RFR(M): 8166317: InterpreterCodeSize should be computed

[Volker Simonis]

On Thu, Aug 31, 2017 at 6:05 PM, Vladimir Kozlov
<vladimir.kozlov at oracle.com> wrote:
> Very good change. Thank you, Volker.
>
> 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.
>

Thanks for the explanation - now I got it.

> 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'll give it a try tomorrow and will send out a new webrev.

Regards,
Volker

> Thanks,
> Vladimir
>
>
> On 8/31/17 5:43 AM, Volker Simonis wrote:
>>
>> 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