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

[Volker Simonis]

Hi Vladimir,

this one is still pending (you only pushed "8166317:
InterpreterCodeSize should be computed").

Could you please also sponsor this one?

The latest version is here:

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

Thank you and best regards,
Volker

On Tue, Sep 5, 2017 at 6:35 PM, Vladimir Kozlov
<vladimir.kozlov at oracle.com> wrote:
> 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
>>>>
>>>
>