RFR: JDK-8190484 Move jvm.h, jmm.h et al to hotspot/*/include

[David Holmes]

On 5/12/2017 9:59 AM, Magnus Ihse Bursie wrote:
> On 2017-12-05 00:28, David Holmes wrote:
>> Hi Magnus,
>>
>> There may be a further wart here to resolve. 
> But of course...
> 
>> From the "bump the classfile version to 54" thread on jdk-dev:
>>
>> ---
>>
>> >>>> Don't you also need to update:
>> >>>>
>> >>>> jdk/src/java.base/share/native/include/classfile_constants.h
>> >>>>
>> >>>> #define JVM_CLASSFILE_MAJOR_VERSION 53
>> >>>>
>> >>> I cannot find any usages for this constant, nor 
>> JVM_CLASSFILE_MINOR_VERSION. I will remove them.
>> >>
>> >> Okay. I don't know the history or use of this file, other than it 
>> gets included into jvm.h to export the jvm interface to the jdk.
>> >>
>> >
>>> And classfile_constants.h is also distributed with the image. I am
>>> unsure of the intent/history. To play it safe i will just bump the
>>> number.
>> Hmmm - that seems wrong. jvm.h is not an exported external interface 
>> AFAIK. And we just moved it so I don't think it will get distributed 
>> any more. Hmm that also suggests that classfile_constants.h may be in 
>> the wrong place ... I'll take this up elsewhere.
>>
>> ---
>>
>> So is classfile_constants.h also in the wrong place? And should it be 
>> in the image ??
> 
> It sounds like it. :( It's only included from jvm.h (and 
> src/java.base/share/native/libverify/check_code.c) as far as I can tell. 
> So it should probably move with jvm.h.
> 
> The bad news is that I just pushed JDK-8190484. (To jdk/hs; Jesper 
> promised me to make sure it ended up in jdk/jdk before RDP1). The good 
> news is that just moving classfile_constants.h is probably simple, I 
> assume all include paths are already correct.
> 
> Can you open a new bug (and/or handle it all by yourself)?

No need - Mandy just clarified this:

 > classfile_constants.h was added to ${java.home}/include in JDK 6 [1].
 > It was intended for BCI native agent to use.
 >
 > Mandy
 > [1] https://bugs.openjdk.java.net/browse/JDK-5107600

so that file should be co-located with jni.h and distributed in the image.

Thanks,
David

> /Magnus
> 
>>
>> David
>>
>> On 5/12/2017 6:40 AM, Magnus Ihse Bursie wrote:
>>> On 2017-12-04 19:17, mandy chung wrote:
>>>>
>>>>
>>>> On 12/4/17 9:33 AM, Erik Joelsson wrote:
>>>>> Hello Magnus,
>>>>>
>>>>> The <module>-copy targets are currently only being generated for 
>>>>> modules that have make/copy/Copy-<module>.gmk makefiles present. By 
>>>>> removing make/copy/Copy-jdk.accessibility.gmk and 
>>>>> make/copy/Copy-jdk.jdwp.agent.gmk, those targets are no longer 
>>>>> created so the logic in CopyCommon will not be executed.
>>>>>
>>>>> This can be solved in two ways. Either generate <module>-copy rules 
>>>>> for all modules or leave the files there with just include 
>>>>> CopyCommon.gmk as the only contents. I would recommend the latter 
>>>>> for now. Most modules do not need to copy anything.
>>>>
>>>> Is it possible to generate <module>-copy rules for module where 
>>>> src/<module>/{share, $OS}/include directory or 
>>>> make/copy/Copy-<module>.gmk is present?
>>> Technically, it's of course possible. But it does not fit very well 
>>> with the current DeclareRecipesForPhase. I agree with Erik, that for 
>>> now the reasonable approach is to have files that only include 
>>> CopyCommon. We can consider for future updates if it's worth 
>>> generalizing this.
>>>
>>> Updated webrev that restores the removed Copy-$MODULE.gmk files:
>>> http://cr.openjdk.java.net/~ihse/JDK-8190484-move-hotspot-exported-includes/webrev.02 
>>>
>>>
>>>>>
>>>>> Another minor note, when ordering include directories, I usually 
>>>>> put the most specific dir first, so that any platform specific 
>>>>> header file with the same name would override a more general one. 
>>>>> We don't have that situation in this case, but I still think it's 
>>>>> good practice.
>>>>>
>>>>> Regarding where to push this. IMO, if it depends on a change 
>>>>> currently in hs, push it to hs. If it ends up in JDK 10 or 11 
>>>>> doesn't really matter that much.
>>>>>
>>>>
>>>> I would love this in JDK 10 if time permits and I am happy to see 
>>>> Coleen retarget it to 10.  This is a really nice clean up that shows 
>>>> the benefit from JEP 201 w.r.t. exported native header files.   But 
>>>> this is not a must for JDK 10 and if it can't make 10, it's okay for 
>>>> 11.
>>>
>>> Ok. I'll try to get it into jdk 10. Will push this to jdk/jdk as soon 
>>> as the needed fixes are integrated from jdk/hs.
>>>
>>> /Magnus
>>>
>>>>
>>>> Mandy
>>>>
>>>>
>>>>> /Erik
>>>>>
>>>>>
>>>>> On 2017-12-04 03:06, Magnus Ihse Bursie wrote:
>>>>>> JDK-8190484 was created as a follow-up bug to the unification of 
>>>>>> the duplicated jvm.h, jvm_md.h and jmm.h, to determine the proper 
>>>>>> location of these files. This has now been decided to be 
>>>>>> hotspot/share/include and hotspot/os/$OS/include, respectively.
>>>>>>
>>>>>> This patch moves the relevant files there, but since this also 
>>>>>> frees up the src/$MODULE/native/include directories for the 
>>>>>> original purpose, it also unifies and simplifies the build logic 
>>>>>> for these directories, so that common code is executed for all 
>>>>>> modules to just copy any exported header files from these 
>>>>>> directories, should they exist.
>>>>>>
>>>>>> I'm intending to push this to jdk-hs.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8190484
>>>>>> WebRev: 
>>>>>> http://cr.openjdk.java.net/~ihse/JDK-8190484-move-hotspot-exported-includes/webrev.01 
>>>>>>
>>>>>>
>>>>>> /Magnus
>>>>>
>>>>
>>>
>