RFR (L, tedious again, sorry) 8189610: Reconcile jvm.h and all jvm_md.h between java.base and hotspot

[David Holmes]

Magnus,

LTO is irrelevant now.

David

On 27/10/2017 9:44 PM, Magnus Ihse Bursie wrote:
> 
> On 2017-10-26 22:44, coleen.phillimore at oracle.com wrote:
>>  Hi Magnus,
>>
>> Thank you for reviewing this.   I have a new version that takes out 
>> the hack in globalDefinitions.hpp and adds casts to 
>> src/hotspot/share/opto/type.cpp instead.
>>
>> Also some fixes from Martin at SAP.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8189610.02/webrev
>>
>> see below.
>>
>> On 10/26/17 5:57 AM, Magnus Ihse Bursie wrote:
>>> Coleen,
>>>
>>> Thank you for addressing this!
>>>
>>> On 2017-10-25 18:49, coleen.phillimore at oracle.com wrote:
>>>> Summary: removed hotspot version of jvm*h and jni*h files
>>>>
>>>> Mostly used sed to remove prims/jvm.h and move #include "jvm.h" 
>>>> after precompiled.h, so if you have repetitive stress wrist issues 
>>>> don't click on most of these files.
>>>>
>>>> There were more issues to resolve, however.  The JDK windows 
>>>> jni_md.h file defined jint as long and the hotspot windows jni_x86.h 
>>>> as int.  I had to choose the jdk version since it's the public 
>>>> version, so there are changes to the hotspot files for this. 
>>>> Generally I changed the code to use 'int' rather than 'jint' where 
>>>> the surrounding API didn't insist on consistently using java types. 
>>>> We should mostly be using C++ types within hotspot except in 
>>>> interfaces to native/JNI code. There are a couple of hacks in places 
>>>> where adding multiple jint casts was too painful.
>>>>
>>>> Tested with JPRT and tier2-4 (in progress).
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8189610.01/webrev
>>>
>>> Looks great!
>>>
>>> Just a few comments:
>>>
>>> * src/java.base/unix/native/include/jni_md.h:
>>>
>>> I don't think the externally_visible attribute should be there for 
>>> arm. I know this was the case for the corresponding hotspot file for 
>>> arm, but that was techically incorrect. The proper dependency here is 
>>> that externally_visible should be in all JNIEXPORT if and only if 
>>> we're building with JVM feature "link-time-opt". Traditionally, that 
>>> feature been enabled when building arm32 builds, and only then, so 
>>> there's been a (coincidentally) connection here. Nowadays, Oracle 
>>> does not care about the arm32 builds, and I'm not sure if anyone else 
>>> is building them with link-time-opt enabled.
>>>
>>> It does seem wrong to me to export this behavior in the public 
>>> jni_md.h file, though. I think the correct way to solve this, if we 
>>> should continue supporting link-time-opt is to make sure this 
>>> attribute is set for exported hotspot functions. If it's still 
>>> needed, that is. A quick googling seems to indicate that 
>>> visibility("default") might be enough in modern gcc's.
>>>
>>> A third option is to remove the support for link-time-opt entirely, 
>>> if it's not really used.
>>
>> I didn't know how to change this since we are still building ARM with 
>> the jdk10/hs repository, and ARM needed this change.  I could wait 
>> until we bring down the jdk10/master changes that remove the ARM build 
>> and remove this conditional before I push.  Or we could file an RFE to 
>> remove link-time-opt (?) and remove it then?
> 
> I'm looking into the link-time-opt issue right now. I think it boils 
> down to us using an incorrect flag to gcc when linking, -fwhole-program, 
> when -fuse-linker-plugin should have been used. This caused all exported 
> symbols to disappear unless they were attributed with 
> externally_visible, which makes sense for a program but not a shared 
> library. I'm currently trying to verify that -fuse-linker-plugin removes 
> the need for the externally_visible attribute when using link-time-opt. 
> If it does, I'll open a separate bug to fix that, and if I push that 
> first, you can safely delete the externally_visible attributes.



> /Magnus
> 
>>
>>>
>>> * src/java.base/unix/native/include/jvm_md.h and 
>>> src/java.base/windows/native/include/jvm_md.h:
>>>
>>> These files define a public API, and contain non-trivial changes. I 
>>> suspect you should file a CSR request. (Even though I realize you're 
>>> only matching the header file with the reality.)
>>>
>>
>> I filed the CSR.   Waiting for the next steps.
>>
>> Thanks,
>> Coleen
>>
>>> /Magnus
>>>
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8189610
>>>>
>>>> I have a script to update copyright files on commit.
>>>>
>>>> Thanks to Magnus and ErikJ for the makefile changes.
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>
>>
>