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

Wed Nov 1 22:16:03 UTC 2017

Thanks Vladimir!

On Wed, Nov 1, 2017 at 10:12 AM, Vladimir Kozlov
<vladimir.kozlov at oracle.com> wrote:
> 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
>>>>>>
>>>>>
>>>
>