RFR(S): 8187091: ReturnBlobToWrongHeapTest fails because of problems in CodeHeap::contains_blob()

[Volker Simonis]

On Fri, Sep 1, 2017 at 6:00 PM, Vladimir Kozlov
<vladimir.kozlov at oracle.com> wrote:
> Checking type is emulation of virtual call ;-)

I agree :)  But it is only a bimorphic dispatch in this case which
should be still faster than a normal virtual call.

> But I agree that it is simplest solution - one line change (excluding
> comment - comment is good BTW).
>

Thanks.

> You can also add guard AOT_ONLY() around aot specific code:
>
>    const void* start = AOT_ONLY( (code_blob_type() == CodeBlobType::AOT) ?
> blob->code_begin() : ) (void*)blob;
>
> because we do have builds without AOT.
>

Done. Please find the new webrev here:

http://cr.openjdk.java.net/~simonis/webrevs/2017/8187091.v1/

Could you please sponsor the change once jdk10-hs opens again?

Thanks,
Volker

PS: one thing which is still unclear to me is why you haven't caught
this issue before? Isn't
test/compiler/codecache/stress/ReturnBlobToWrongHeapTest.java part of
JPRT and/or your regular tests?


> Thanks,
> Vladimir
>
>
> On 9/1/17 8:42 AM, Volker Simonis wrote:
>>
>> Hi,
>>
>> can I please have a review and sponsor for the following small fix:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8187091/
>> https://bugs.openjdk.java.net/browse/JDK-8187091
>>
>> We see failures in
>> test/compiler/codecache/stress/ReturnBlobToWrongHeapTest.java which
>> are cause by problems in CodeHeap::contains_blob() for corner cases
>> with CodeBlobs of zero size:
>>
>> # A fatal error has been detected by the Java Runtime Environment:
>> #
>> # Internal Error (heap.cpp:248), pid=27586, tid=27587
>> # guarantee((char*) b >= _memory.low_boundary() && (char*) b <
>> _memory.high()) failed: The block to be deallocated 0x00007fffe6666f80
>> is not within the heap starting with 0x00007fffe6667000 and ending
>> with 0x00007fffe6ba000
>>
>> The problem is that JDK-8183573 replaced
>>
>>    virtual bool contains_blob(const CodeBlob* blob) const { return
>> low_boundary() <= (char*) blob && (char*) blob < high(); }
>>
>> by:
>>
>>    bool contains_blob(const CodeBlob* blob) const { return
>> contains(blob->code_begin()); }
>>
>> But that my be wrong in the corner case where the size of the
>> CodeBlob's payload is zero (i.e. the CodeBlob consists only of the
>> 'header' - i.e. the C++ object itself) because in that case
>> CodeBlob::code_begin() points right behind the CodeBlob's header which
>> is a memory location which doesn't belong to the CodeBlob anymore.
>>
>> This exact corner case is exercised by ReturnBlobToWrongHeapTest which
>> allocates CodeBlobs of size zero (i.e. zero 'payload') with the help
>> of sun.hotspot.WhiteBox.allocateCodeBlob() until the CodeCache fills
>> up. The test first fills the 'non-profiled nmethods' CodeHeap. If the
>> 'non-profiled nmethods' CodeHeap is full, the VM automatically tries
>> to allocate from the 'profiled nmethods' CodeHeap until that fills up
>> as well. But in the CodeCache the 'profiled nmethods' CodeHeap is
>> located right before the non-profiled nmethods' CodeHeap. So if the
>> last CodeBlob allocated from the 'profiled nmethods' CodeHeap has a
>> payload size of zero and uses all the CodeHeaps remaining size, we
>> will end up with a CodeBlob whose code_begin() address will point
>> right behind the actual CodeHeap (i.e. it will point right at the
>> beginning of the adjacent, 'non-profiled nmethods' CodeHeap). This
>> will result in the above guarantee to fire, when we will try to free
>> the last allocated CodeBlob (with
>> sun.hotspot.WhiteBox.freeCodeBlob()).
>>
>> In a previous mail thread
>>
>> (http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-August/028175.html)
>> Vladimir explained why JDK-8183573 was done:
>>
>>> About contains_blob(). The problem is that AOTCompiledMethod allocated in
>>> CHeap and not in aot code section (which is RO):
>>>
>>>
>>> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/8acd232fb52a/src/share/vm/aot/aotCompiledMethod.hpp#l124
>>>
>>> It is allocated in CHeap after AOT library is loaded. Its code_begin()
>>> points to AOT code section but AOTCompiledMethod*
>>> points outside it (to normal malloced space) so you can't use (char*)blob
>>> address.
>>
>>
>> and proposed these two fixes:
>>
>>> There are 2 ways to fix it, I think.
>>> One is to add new field to CodeBlobLayout and set it to blob* address for
>>> normal CodeCache blobs and to code_begin for
>>> AOT code.
>>> Second is to use contains(blob->code_end() - 1) assuming that AOT code is
>>> never zero.
>>
>>
>> I came up with a slightly different solution - just use
>> 'CodeHeap::code_blob_type()' whether to use 'blob->code_begin()' (for
>> the AOT case) or '(void*)blob' (for all other blobs) as input for the
>> call to 'CodeHeap::contain()'. It's simple and still much cheaper than
>> a virtual call. What do you think?
>>
>> I've also updated the documentation of the CodeBlob class hierarchy in
>> codeBlob.hpp. Please let me know if I've missed something.
>>
>> Thank you and best regards,
>> Volker
>>
>