[15] RFR(M) 8237497: vmStructs_jvmci.cpp does not check that the correct field type is specified

[Ioi Lam]

On 3/20/20 12:32 PM, Vladimir Kozlov wrote:
> Thank you, Ioi, for looking on changes.
>
> On 3/20/20 11:56 AM, Ioi Lam wrote:
>> Hi Vladimir,
>>
>> Why do you need jvmci_vmStructs_init()? Is it possible to call 
>> JVMCIVMStructs::init() directly?
>
> For that I have to #include vmStructs_jvmci.hpp which will pull other 
> headers as well. Which would increase parsing time of this C++ code 
> even for product build. And I followed what we do for vmStruct - it 
> calls vmStructs_init().
>

OK.

>>
>> Also, I think you can change the declaration as
>>
>> class JVMCIVMStructs {
>>     void init() NOT_DEBUG_RETURN;
>> }
>>
>> So the caller can avoid using DEBUG_ONLY().
>
> I don't like hiding that a called code is executed only in debug more.
> And why do we need to keep empty methods in product builds?
>
The "XXX_RETURN;" pattern is widely used. E.g.,

class nmethod : public CompiledMethod {
   void print_dependencies()                       PRODUCT_RETURN;
}

These functions are all inlined and elided by the C++ compiler. They 
don't take up space in the product build.

$ nm -S debug/images/jdk/lib/server/libjvm.so | c++filt | grep 
nmethod::print_dependencies
000000000173fdb2 000000000000017f t nmethod::print_dependencies()

$ nm -S product/images/jdk/lib/server/libjvm.so | c++filt | grep 
nmethod::print_dependencies | wc
       0       0       0

We only use this style of declaration for class member functions, 
though. I believe it's not strictly compliant to have a stray ";" in the 
global scope until some newer version of C++ that we have not adopted yet.

Thanks
- Ioi

> It takes a lot of time to compile HotSpot VM.
>
>>
>> The rest looks good to me.
>
> Thank you,
> Vladimir K
>
>>
>> Thanks
>> - Ioi
>>
>> On 3/20/20 10:01 AM, Vladimir Kozlov wrote:
>>> http://cr.openjdk.java.net/~kvn/8237497/webrev.00/
>>> https://bugs.openjdk.java.net/browse/JDK-8237497
>>>
>>> JVMCI missed checks for declarations in vmStructs_jvmci.cpp. I 
>>> copied check logic from vmStruct to fix that. It will be called 
>>> during JVMCI initialization in debug build (as for vmStruct). I 
>>> can't call it from vmStructs_init() because vmStruct code is guarded 
>>> by INCLUDE_VM_STRUCTS variable which can be switched off with 
>>> configure option --with-jvm-features=-vm-structs. On other hand 
>>> JVMCIVMStruct have to be initialized always when JVMCI is used - it 
>>> is how Graal accesses HotSpot's internal data.
>>>
>>> The fix found several (4) cases of incorrect types in JVMCI 
>>> declarations. 2 of them require update Graal code as well which I 
>>> will upstream later.
>>>
>>> I also did cleanup in both, vmStructs_jvmci and vmStructs.
>>> In vmStructs_jvmci I removed macros which are not used (we can add 
>>> them back if they needed).
>>> In vmStructs I moved all check code under #ifdef ASSERT because it 
>>> is called only in debug build.
>>> I removed VMStructs::test() declaration which we forgot to remove 
>>> when its body was removed in JDK-8171090.
>>> I removed duplicated lines which existed from "day one" (before 
>>> Mercurial).
>>>
>>> I did local build and testing with and without 
>>> --with-jvm-features=-vm-structs to make sure everything (inluding 
>>> Graal) worked. I ran tier1 (which runs HotSpot gtests), hs-tier2, 
>>> hs-tier3-graal.
>>>
>>> Thanks,
>>> Vladimir
>>