Unification of jni_<platform>.h

[Doerr, Martin]

Hi,

we only support 64 bit on s390. Seems like the code could be cleaned up or updated. Thanks for looking into it.

Best regards,
Martin


-----Original Message-----
From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of David Holmes
Sent: Montag, 23. Oktober 2017 05:47
To: Magnus Ihse Bursie <magnus.ihse.bursie at oracle.com>; hotspot-runtime-dev at openjdk.java.net; build-dev <build-dev at openjdk.java.net>
Subject: Re: Unification of jni_<platform>.h

Hi Magnus,

On 18/10/2017 11:10 PM, Magnus Ihse Bursie wrote:
> In the context of JDK-8167078 (Duplicate header files in hotspot and 
> jdk), I was looking at unifying the platform specific extensions to 
> jni.h, the jni_md.h files between hotspot and java.base.
> 
> The main difference here is that the hotspot jni_* files are divided 
> into individual files based on cpu, while the java.base versions are 
> divided according to operating system (which, in turn, implicates 
> compiler). On a second look, however, it turns out to not be so 
> problematic -- a hotspot file like jni_x86.h contains basically
> #ifdef WIN32
> ... // do windows stuff
> #else
> ... // do posix/macos stuff
> #endif
> 
> and the two blocks match very well what the java.base versions are doing 
> for the different operating systems.
> 
> In fact, the OS (and compiler) division seems more natural, since there 
> is much redundancy in the hotspot files, and the java.base OS-based 
> versions are much simpler.
> 
> I'm proposing to remove the hotspot CPU-based files and just replace 
> them with the java.base versions. However, a few differences crept out. 
> I'd like to discuss them before proceeding.
> 
> For jni_aarch64.h: There's a windows (or rather, "not unix") part of the 
> jni_aarch64.h that I do not believe have ever been used. Nevertheless, 
> it contains "typedef int jint" rather than "typedef long jint", which 
> the java.base windows version uses. Since I've never heard of aarch64 
> building on Windows, I presume this is irrelevant.

Likely just copied from the x86 version at some point. There is no 
Windows-aarch64 support in the OpenJDK.

> For jni_aarch64.h and jni_sparc.h: The unix version will match with the 
> java.base version as long as _LP64 is defined. This should be just fine, 
> since we define that when building hotspot/the JDK, and the java.base 
> version that is exported by the JDK on linux/aarch64 and solaris/sparc 
> has always required the _LP64 define.

Yep 64-bit only.

> (In fact, the macos and unix versions in java.base only contain trivial 
> differences. The macos version should probably be removed in favor of a 
> single unix version.)
> 
> For jni_x86.h: There windows part suffers the same problem as for 
> aarch64, but here it's potentially problematic. The hotspot version uses 
> "typedef int jint" while the java.base version uses "typedef long jint". 
> If this *really* were a problem, we would probably have gotten reports 
> from JNI code unable to work on Windows, so I assume this turns out to 
> be the same datatype in the end. Don't know if it's due to luck, 
> compiler flags or by definition.

Windows is either ILP32 or LLP64 - in both cases int and long are the 
same and 32-bits.

> 
> For jni_s390.h: Here's the most problematic version. The exported 
> java.base version uses:
> #ifdef _LP64
> typedef long jlong;
> #else
> typedef long long jlong;
> #endif
> but s390 uses:
> typedef long int jlong;
> 
> My best assumption here is that we're only really using s390x (the 
> 64-bit version) and that "long int" is functionally equivalent to 
> "long". Or that jni is broken on s390 and nobody really noticed.
> 
> Also, s390 uses a simplified version of the gcc attribute dance used for 
> JNIEXPORT/JNIIMPORT. I think the s390 version is just not really 
> updated, and that using the newer in java.base is harmless.

Can't comment on s390.

> For jni_arm.h: Finally, the jni_arm.h (the 32-bit formerly closed Oracle 
> port), the JNIEXPORT/JNIIMPORT is different, by defining 
> __attribute__((externally_visible)). This might have been relevant due 
> to compile/link time symbol processing for that platform, but 
> technically it should probably not have been connected to the platform 
> per se, but rather to the compilation procedure. Since the arm-32 
> platform is kind of abandoned right now, I propose to modify the 
> compilation to be more standard, if this is required, rather than 
> keeping these attributes.

As Dean stated this likely related to LTO. And unfortunately attention 
was not paid to the comment:

// Note: please do not change these without also changing jni_md.h in 
the JDK
// repository

possibly due to open/closed issues back in time. But I'd say we would 
want the hotspot version so that nothing that would previously work is 
now broken.

Thanks,
David

> 
> /Magnus