[Fwd: Request for reviews (M): 6973963: SEGV in ciBlock::start_bci() with EA]

[David Holmes]

Hi Vladimir,

I always get nervous when people start tweaking these fundamental 
allocation classes - there are always unforeseen interactions. :)

This is only a partial review.

src/share/vm/asm/codeBuffer.cpp

+ void CodeBuffer::operator delete(void* p) {
+   ResourceObj::operator delete(p);
   #ifdef ASSERT
!   Copy::fill_to_bytes(p, sizeof(CodeBuffer), badResourceValue);
   #endif
   }

You moved the copy from the destructor to operator delete, but now you 
access p after you have deleted it.

src/share/vm/utilities/growableArray.hpp

+     // on stack or ebedded into an other object.

Typo: embedded

src/share/vm/memory/allocation.hpp

!   ~ResourceObj();

You've added a destructor, but subclasses define their own destructors - 
so doesn't this need to be virtual?

src/share/vm/memory/allocation.cpp

+ #ifdef ASSERT
+   ((ResourceObj *)p)->_allocation = badHeapOopVal;
+ #endif // ASSERT

For consistency this should be a DEBUG_ONLY(...)

+     assert((allocation & allocation_mask) == 0, "");
+     assert(type <= allocation_mask, "");

Please give meaningful messages to assertion failures.

David
-----

Vladimir Kozlov said the following on 08/03/10 15:45:
> I updated webrev:
> 
> http://cr.openjdk.java.net/~kvn/6973963/webrev.01
> 
> Additional changes:
> 
> 1. Used new option StressRecompilation instead of CompileTheWorld.
>    I am still not sure if it is OK since we never test our stress options.
> 
> 2. Added new allocation type: STACK_OR_EMBEDDED for cases when new() is 
> not called.
> 
> 3. Added ResourceObj destructor to zap _allocation field.
> 
> 4. Added assert into get_allocation_type() to check that 'this' address 
> is still encoded in _allocation.
>    Found several cases where it was not true, have to add copy 
> constructor and assignment operator.
> 
> 5. Moved all new methods with asserts into allocation.cpp.
> 
> 6. The added assert failed for CodeBuffer since it destroys itself 
> inside destructor before ResourceObj destructor called.
>    Moved Copy::fill_to_bytes(badResourceValue) into CodeBuffer::operator 
> delete().
> 
> 7. Replaced PhaseCFG::_node_latency field with pointer since it is valid 
> only inside resource mark in GlobalCodeMotion().
> 
> Thanks,
> Vladimir
> 
> Vladimir Kozlov wrote:
>> Thank you, Tom
>>
>> RESOURCE_AREA case combines 3 cases:
>>
>> GrowableArray<int> *foo = new GrowableArray<int>(size); // resource 
>> array allocation
>>
>> GrowableArray<int> foo; // stack allocation
>>
>> class A : public ResourceObj {
>>   GrowableArray<int> foo; // embedding allocation
>> }
>>
>> In all this cases GrowableArray::_data array is allocated in thread local
>> resource area. GrowableArray calls all these cases as stack allocation
>> and I followed this naming.
>>
>> But you are right I can separate 2 last cases into "stack" (stack or 
>> embedding)
>> allocation, I still have 1 bit in allocation_type :)
>>
>> Thanks,
>> Vladimir
>>
>> Tom Rodriguez wrote:
>>> So doesn't this triple the amount of time taken by CTW?  That seems 
>>> kind of extreme for the default mode.  Maybe it should be under a 
>>> flag?  The allocation.hpp changes look a little sketchy. +     if 
>>> (~(_allocation | allocation_mask) != (uintptr_t)this) {
>>> +       set_allocation_type((address)this, RESOURCE_AREA);
>>>
>>> Why is this case called RESOURCE_AREA?  Isn't this the stack or 
>>> embedding case?
>>>
>>> This doesn't make sense either.
>>>
>>> !   bool allocated_on_stack()     { return get_allocation_type() == 
>>> RESOURCE_AREA; }
>>>
>>> Anyway, the existing logic around this seemed sketchy so I can't 
>>> quite say whether this is better or not.  I'll have to leave that to 
>>> someone else.
>>>
>>> The GrowableArray changes themselves look fine.
>>>
>>> tom
>>>
>>> On Aug 2, 2010, at 3:01 PM, Vladimir Kozlov wrote:
>>>
>>>> Forwarding to GC and Runtime groups since it is common code.
>>>>
>>>> Vladimir
>>>>
>>>> -------- Original Message --------
>>>> Subject: Request for reviews (M): 6973963: SEGV in 
>>>> ciBlock::start_bci() with EA
>>>> Date: Mon, 02 Aug 2010 14:57:25 -0700
>>>> From: Vladimir Kozlov <vladimir.kozlov at oracle.com>
>>>> To: hotspot compiler <hotspot-compiler-dev at openjdk.java.net>
>>>>
>>>> http://cr.openjdk.java.net/~kvn/6973963/webrev
>>>>
>>>> Fixed 6973963: SEGV in ciBlock::start_bci() with EA
>>>>
>>>> I added stress recompilation during CompileTheWorld and found this 
>>>> case.
>>>> It is similar to 6968368. BCEscapeAnalyzer::do_analysis() calls
>>>> ciMethod::get_method_blocks() which calls constructor ciMethodBlocks.
>>>> This constructor allocates GrowableArray elements on stack (thread
>>>> local resource area). As result when the method recompiled without EA
>>>> _blocks->_data is NULL.
>>>>
>>>> Solution:
>>>> Added stress recompilation during CompileTheWorld: recompile with
>>>> subsume_loads = false and do_escape_analysis = false.
>>>> Added more checks into ResourceObj and growableArray to verify 
>>>> correctness
>>>> of allocation. I have to relax the new assert in GrowableArray when
>>>> elements are allocated on arena to allow allocattion of 
>>>> GrowableArray object
>>>> as a part of an other object (for example, in ConnectionGraph and 
>>>> SuperWord).
>>>>
>>>> Tested with failed cases, CTW.
>>>