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

[Ioi Lam]

Looks good.

Thanks
- Ioi

On 3/20/20 5:47 PM, Vladimir Kozlov wrote:
> 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
>>>>
>>