On 10/27/17 7:44 AM, Magnus Ihse Bursie wrote:
On 2017-10-26 22:44, coleen.phillim...@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.phillim...@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.
Thanks Magnus. Let me know when you push this change and I'll update my
change to remove this #ifdef ARM code. Please push to the hs repo though.
Thanks!
Coleen
/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
