Resend: Request for reviews (M): 6993125: runThese crashes with assert(Thread::current()->on_local_stack((address)this))

[Vladimir Kozlov]

Thank you, David

David Holmes wrote:
> Hi Vladimir,
> 
> I guess no-one wanted to touch this :)

I also thought so :)

> 
> So if I understand this right one debug field to try and catch when 
> garbage impersonates an object is not good enough so you are adding a 

Correct.

> second. Seems reasonable, but why not simply turn the current field into 
> an array and set two magic numbers in the entries (inverted this 
> pointer, this pointer word-swapped) etc. ?

I will convert it to array as you suggested:
  uintptr_t _allocation[2];

> 
> It also strikes me as odd that set_allocation_type does:
> 
>  87     if (type != STACK_OR_EMBEDDED) {
>  88       // This method is called from operator new(), mark allocation.
> 
> why doesn't operator new() set this debug field directly?

There are 3 new() operators in ResourceObj so I would need
to modify code in all of them. I decided to do it in one place.
I can add new method set_verification_type() to set second
fiels and it which will be called only from new() operators.

> 
> The logic in the ResourceObj constructors is getting pretty convoluted - 
> I couldn't quite work out all the permutations.

I have to agree, I just realized that I should remove next assert
which could be triggered by garbage and it is useless since we
ignore second debug field if the garbage in first field looks
like valid STACK_OR_EMBEDDED value:

  assert(!is_type_set(),

> 
> But if this fixes the bug and passes all tests then it would seem to be ok.

Thanks,
Vladimir

> 
> David
> 
> 
> Vladimir Kozlov said the following on 12/10/10 08:38:
>> Resending since nobody responded to this request I sent week ago (lost?).
>>
>> Vladimir
>>
>> http://cr.openjdk.java.net/~kvn/6993125/webrev
>>
>> Fixed 6993125: runThese crashes with 
>> assert(Thread::current()->on_local_stack((address)this))
>>
>> This code is used to check that allocation space of
>> a GrowableArray object is matching allocation space
>> of its array. It is also check that operator
>> ResourceObj::delete() is called only for C heap
>> allocated objects.
>> To do that operator ResourceObj::new() stores an
>> allocation type into ResourceObj debug field.
>> But new() is not called for stack allocated and
>> embedded objects and ResourceObj() constructor
>> does not know if new() was called.
>> So the constructor is trying to guess it by
>> looking on _allocation value which could be
>> a garbage resembling a valid value.
>>
>> In this bug case the garbage was a valid value for
>> an embedded object and not for a stack allocated
>> object this is why the assert is failed.
>> In the 6994834 case the garbage was a valid value
>> for C heap allocated object and it was embedded
>> object (funny fact: garbage value was 0xf1f1f1f1
>> which is zap value for malloc memory and the
>> embedded object address was 0x0e0e0e0c so that
>> ~(0x0e0e0e0c + 0x2) == 0xf1f1f1f1).
>>
>> The only small solution for this problem I found is
>> to add another ResourceObj debug field and set it
>> in operator ResourceObj::new().
>> I think it should provide much less probability that
>> garbage in these two fields together will match
>> valid values. Unfortunately the probability is not 0.
>> An other solution is totally remove this code or put
>> it under a flag and test it only sometimes.
>>
>> Thanks,
>> Vladimir