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

[Vladimir Kozlov]

Thank you, Dave

On 8/2/10 11:23 PM, David Holmes wrote:
> Hi Vladimir,
>
> I always get nervous when people start tweaking these fundamental allocation classes - there are always unforeseen
> interactions. :)

I hate to touch it also. But my patience was broken when I found this problem.

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

Yes, you are right. But CodeBuffer is never allocated on C heap and ResourceObj::delete(p)
should be called only for allocation on C_HEAP. So this code should never be executed,
I will replace it with ShouldNotCallThis() in CodeBuffer::delete(void* p).
Which leaves the original problem: I need to find how to zap CodeBuffer after ResourceObj destructor
which is called after CodeBuffer destructor.
Note: the problem here is not CodeBuffer but its field: OopRecorder  _default_oop_recorder;

>
> src/share/vm/utilities/growableArray.hpp
>
> + // on stack or ebedded into an other object.
>
> Typo: embedded

OK.

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

Constructor and destructor are special methods. Super class's default destructor are always called from
subclass's destructor. The only case when you need to declare it as virtual if subclass object is assigned to
local/field of super class type and compiler does not know what the original subclass was.

ResourceObj *f = new OopRecorder();
...
delete f;

We don't use such constructions.

>
> src/share/vm/memory/allocation.cpp
>
> + #ifdef ASSERT
> + ((ResourceObj *)p)->_allocation = badHeapOopVal;
> + #endif // ASSERT
>
> For consistency this should be a DEBUG_ONLY(...)

OK. I had problems recently with some macros when I passed expressions with nested () so I was too conservative here.

>
> + assert((allocation & allocation_mask) == 0, "");
> + assert(type <= allocation_mask, "");
>
> Please give meaningful messages to assertion failures.

OK. I will update webrev today (during work hours :) ).

Thanks,
Vladimir

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