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
