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

David Holmes david.holmes at oracle.com
Fri Oct 27 07:23:34 UTC 2017

Hi Coleen,

Thanks for tackling this.

> Summary: removed hotspot version of jvm*h and jni*h files

Can you update the bug synopsis to show it covers both sets of files please.

I hate to start with this (and it took me quite a while to realize it) 
but as Mandy pointed out jvm.h is not an exported interface from the JDK 
to the outside world (so not subject to CSR review), but is a private 
interface between the JVM and the JDK libraries. So I think really jvm.h 
belongs in the hotspot sources where it was, while jni.h belongs in the 
exported JDK sources. In which case the bulk of your changes to the 
hotspot files would not be needed - sorry.

Moving on ...

First to address the initial comments/query you had:

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

On Windows int and long are always the same as it uses ILP32 or LLP64 
(not LP64 like *nix platforms). So either choice should be fine. That 
said there are some odd casting issues I comment on below. Does the VS 
compiler complain about mixing int and long in expressions?

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

I think you pulled too hard on a few threads here and things are 
starting to unravel. There are numerous cases I refer to below where 
either the cast seems unnecessary/inappropriate or else highlights a 
bunch of additional changes that also need to be made. The fan out from 
this could be horrendous. Unless you actually get some kind of error - 
and I'd like to understand the details of those - I would not suggest 
making these changes as part of this work.

Looking through I have a quite a few queries/comments - apologies in 
advance as I know how tedious this is:


Took a while to figure out why the include was needed. :) As a follow up 
I suggest just deleting the -I include directive, delete the 
Solaris-only definition of JSIG_VERSION_1_4_1, and delete everything to 
do with JVM_get_libjsig_version. It is all obsolete.



Why did you need to add the jvm.h include?



The type of process_exiting should be uint to match the DWORD of 
GetCurrentThreadID. Then you should need any casts. Also you missed this 
jint cast:

3796         process_exiting != (jint)GetCurrentThreadId()) {



   43 #ifdef _WINDOWS
   44   // jint is defined as long in jni_md.h, so convert from int to jint
   45   void set_constant(int x)                       { 
set_constant((jint)x); }
   46 #endif

Why is this necessary? int and long are the same on Windows. The whole 
point is that jint hides the underlying type, so where does this go wrong?




why is this cast needed? what causes the ambiguity? (If this was a 
template I'd understand ;-) ). Also didn't you change that constructor 
to take an int anyway - not that I think it should - see below.



793         jint* dims = NEW_RESOURCE_ARRAY(jint, rank);

why should this be jint?



Okay this looks more consistent with jint.



These changes seem wrong. We have:

ConstantLongValue(jlong value)
ConstantDoubleValue(jdouble value)

so we should have:

ConstantIntValue(jint value)



Change seems unnecessary - int32_t is fine



I see a complete mix of int and jint in this class, so why make the one 
change you did ??



1700     tty->write((char*) start, MIN2(length, (jint)O_BUFLEN));

why did you need to add the jint cast? It's used without any cast on the 
next two lines:

1701     length -= O_BUFLEN;
1702     offset += O_BUFLEN;




Looking around this code it seems very confused about types - eg the 
previous function is declared jboolean yet returns a jint on one path! 
It isn't clear to me if the return type is what should be changed or the 
parameter type? I would just leave this alone.



Okay TypeInt has jint parts, so the remaining int32_t declarations (A, 
B, C, D) should also be jint.



I agree with the changes you made, but then:

  419     jint dim_con = find_int_con(length[j], -1);

should also be changed.

And obviously MultiArrayExpandLimit should be defined as int not intx!



I can see that intcon(jint i) is consistent with longcon(jlong l), but 
the use of "i" in the code is more consistent with int than jint.



1505 int TypeInt::hash(void) const {
1506   return java_add(java_add(_lo, _hi), java_add((jint)_widen, 
1507 }

I can see that the (jint) casts you added make sense, but then the whole 
function should be returning jint not int. Ditto the other hash functions.



I think vm_created should be a bool. In fact all the fields you changed 
are logically bools - do Atomics work for bool now?



is_attachable is the terminology used in the JDK code.



Are you making parameters consistent with the fields they initialize?



There is a mix of int and jint for slot in this code. You fixed some, 
but this remains:

2440 inline bool CallbackInvoker::report_stack_ref_root(jlong thread_tag,
2441                                                    jlong tid,
2442                                                    jint depth,
2443                                                    jmethodID method,
2444                                                    jlocation bci,
2445                                                    jint slot,



Callers pass both jint and int, so param type seems arbitrary.



PerfMemory::_initialized should ideally be a bool - can OrderAccess 
handle that now?



Not clear why the jio functions are not also JNICALL ?



There is no need to special case ARM. The differences in the existing 
code were for LTO support and that is now irrelevant.



I know you've just copied this across, but it seems wrong to me:

  57 // Hack: MAXPATHLEN is 4095 on some Linux and 4096 on others. This may
   58 //       cause problems if JVM and the rest of JDK are built on 
   59 //       Linux releases. Here we define JVM_MAXPATHLEN to be 
   60 //       so buffers declared in VM are always >= 4096.

It doesn't make sense to me to define an internal "max path length" that 
can _exceed_ the platform max!

That aside there's no support for building different parts of the JDK on 
different platforms and then bringing them together. And in any case I 
would think the real problem would be building on a platform that uses 
4096 and running on one that uses 4095!

But that aside this is a Linux hack and should be guarded by ifdef 
LINUX. (I doubt BSD needs it, the bsd file is just a copy of the linux 
one - the JDK macosx version does the right thing). Solaris and AIX 
should stay as-is at MAXPATHLEN.

  86 #define ASYNC_SIGNAL     SIGJVM2

This only exists on Solaris so I think should be in #ifdef SOLARIS, to 
make that clear.



Given the differences between the two versions either something has been 
broken or "extern C" declarations are not needed :)


That was a really painful way to spend most of my Friday. TGIF! :)


On 27/10/2017 6:44 AM, 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?
>> * 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

More information about the hotspot-dev mailing list