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
>