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

Wed Nov 1 17:12:31 UTC 2017

I submitted pre-integration testing. Will push after that.

Vladimir

On 10/30/17 12:34 PM, Volker Simonis wrote:
> 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
>>>>>
>>>>
>>