RFR(M): 8166317: InterpreterCodeSize should be computed

[Volker Simonis]

On Thu, Aug 31, 2017 at 12:14 PM, Claes Redestad
<claes.redestad at oracle.com> wrote:
>
>
> On 2017-08-31 08:54, Volker Simonis wrote:
>>
>> While working on this, I found another problem which is related to the
>> fix of JDK-8183573 and leads to crashes when executing the JTreg test
>> compiler/codecache/stress/ReturnBlobToWrongHeapTest.java.
>>
>> 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.
>
>
> I recall this change was somehow necessary to allow merging
> AOTCodeHeap::contains_blob and CodeHead::contains_blob into
> one devirtualized method, so you need to ensure all AOT tests
> pass with this change (on linux-x64).
>

All of hotspot/test/aot and hotspot/test/jvmci executed and passed
successful. Are there any other tests I should check?

That said, it is a little hard to follow the stages of your change. It
seems like http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.00/
was reviewed [1] but then finally the slightly changed version from
http://cr.openjdk.java.net/~redestad/scratch/codeheap_contains.01/ was
checked in and linked to the bug report.

The first, reviewed version of the change still had a correct version
of 'CodeHeap::contains_blob(const CodeBlob* blob)' while the second,
checked in version has the faulty version of that method.

I don't know why you finally did that change to 'contains_blob()' but
I don't see any reason why we shouldn't be able to directly use the
blob's address for inclusion checking. From what I understand, it
should ALWAYS be contained in the corresponding CodeHeap so no reason
to mess with 'CodeBlob::code_begin()'.

Please let me know if I'm missing something.

[1] http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-July/026624.html

> I can't help to wonder if we'd not be better served by disallowing
> zero-sized payloads. Is this something that can ever actually
> happen except by abuse of the white box API?
>

The corresponding test (ReturnBlobToWrongHeapTest.java) specifically
wants to allocate "segment sized" blocks which is most easily achieved
by allocation zero-sized CodeBlobs. And I think there's nothing wrong
about it if we handle the inclusion tests correctly.

Thank you and best regards,
Volker

> /Claes