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

[David Holmes]

Hi Vladimir,

I guess no-one wanted to touch this :)

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 
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. ?

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?

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

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

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