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

[Vladimir Kozlov]

Okay, as you suggested.

Methods JVMCIVMStructs::init() and jvmci_vmStructs_init() declaration in jvmci.cpp use NOT_DEBUG_RETURN.
The same for vmStructs_init() in init.cpp. I also added NOT_VM_STRUCTS_RETURN_(0) to methods in VMStructs because 
vmStructs.cpp file is excluded from compilation when vm-structs is excluded in configure.

http://cr.openjdk.java.net/~kvn/8237497/webrev.01/

Thanks,
Vladimir

On 3/20/20 1:00 PM, Ioi Lam wrote:
> 
> 
> 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
>>>
>