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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Volker Simonis
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
Reply | Threaded
Open this post in threaded view
|

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

Vladimir Kozlov
Checking type is emulation of virtual call ;-)
But I agree that it is simplest solution - one line change (excluding comment - comment is good BTW).

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.

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
>
Reply | Threaded
Open this post in threaded view
|

Re: 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
<[hidden email]> 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
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: 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
> <[hidden email]> 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
>>>
>>
Reply | Threaded
Open this post in threaded view
|

Re: 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
<[hidden email]> wrote:

> On 9/4/17 10:23 AM, Volker Simonis wrote:
>>
>> On Fri, Sep 1, 2017 at 6:00 PM, Vladimir Kozlov
>> <[hidden email]> 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
>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

Vladimir Kozlov
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
> <[hidden email]> wrote:
>> On 9/4/17 10:23 AM, Volker Simonis wrote:
>>>
>>> On Fri, Sep 1, 2017 at 6:00 PM, Vladimir Kozlov
>>> <[hidden email]> 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
>>>>>
>>>>
>>
Reply | Threaded
Open this post in threaded view
|

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

Volker Simonis
Thanks Vladimir!

On Wed, Nov 1, 2017 at 10:12 AM, Vladimir Kozlov
<[hidden email]> 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
>> <[hidden email]> wrote:
>>>
>>> On 9/4/17 10:23 AM, Volker Simonis wrote:
>>>>
>>>>
>>>> On Fri, Sep 1, 2017 at 6:00 PM, Vladimir Kozlov
>>>> <[hidden email]> 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
>>>>>>
>>>>>
>>>
>