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

[coleen.phillimore at oracle.com]

  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?

>
> * 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
>>
>