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

[Vladimir Kozlov]

On 9/4/17 10:23 AM, Volker Simonis wrote:
> 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/

Looks good. Thank you for updated CodeBlob description comment.

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

We have to wait when jdk10 "consolidation" is finished. It may take 2 weeks.

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

test/compiler/codecache/stress are excluded from JPRT runs:

https://bugs.openjdk.java.net/browse/JDK-8069021

Also these tests are marked with @key stress. Originally it was only 2 
tests and ReturnBlobToWrongHeapTest.java was added later:

https://bugs.openjdk.java.net/browse/JDK-8069021

I am trying to find which testing tier runs them. I will follow this.

Thanks,
Vladimir

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