RFR(M): 8166317: InterpreterCodeSize should be computed

[Vladimir Kozlov]

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.

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.

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