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

[Vladimir Kozlov]

Thank you, Ioi

Thanks
Vladimir

> On Mar 21, 2020, at 11:21 AM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> 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
>>>>> 
>>> 
>