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

classic Classic list List threaded Threaded
28 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Magnus Ihse Bursie
Coleen,

Thank you for addressing this!

On 2017-10-25 18:49, [hidden email] 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.

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

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

Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung


On 10/26/17 2:57 AM, Magnus Ihse Bursie wrote:
> A third option is to remove the support for link-time-opt entirely, if
> it's not really used.
>
> * 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.)

jvm.h and jvm_md.h are not public API and they are not copied to the
$JAVA_HOME/includes directly.  This does raise the question that jvm*.h
may belong to other location than src/java.base/{share,$OS}/native/include.

Mandy
Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore


On 10/26/17 2:47 PM, mandy chung wrote:

>
>
> On 10/26/17 2:57 AM, Magnus Ihse Bursie wrote:
>> A third option is to remove the support for link-time-opt entirely,
>> if it's not really used.
>>
>> * 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.)
>
> jvm.h and jvm_md.h are not public API and they are not copied to the
> $JAVA_HOME/includes directly.  This does raise the question that
> jvm*.h may belong to other location than
> src/java.base/{share,$OS}/native/include.

I'm not sure where else it would go honestly, but it could be moved
outside this changeset.  The good thing about where it is, is that the
-I directives in the makefiles find both jni.h and jvm.h.
thanks,
Coleen
>
> Mandy

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
In reply to this post by Magnus Ihse Bursie
  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, [hidden email] 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
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung
In reply to this post by coleen.phillimore


On 10/26/17 1:34 PM, [hidden email] wrote:

>
>
> On 10/26/17 2:47 PM, mandy chung wrote:
>>
>>
>> On 10/26/17 2:57 AM, Magnus Ihse Bursie wrote:
>>> A third option is to remove the support for link-time-opt entirely,
>>> if it's not really used.
>>>
>>> * 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.)
>>
>> jvm.h and jvm_md.h are not public API and they are not copied to the
>> $JAVA_HOME/includes directly.  This does raise the question that
>> jvm*.h may belong to other location than
>> src/java.base/{share,$OS}/native/include.
>
> I'm not sure where else it would go honestly, but it could be moved
> outside this changeset.  The good thing about where it is, is that the
> -I directives in the makefiles find both jni.h and jvm.h.

I agree we should keep this location for this change (the location is a
separate issue).  I reviewed the change that looks good to me.

Mandy
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by coleen.phillimore
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:

make/hotspot/lib/CompileLibjsig.gmk
src/java.base/solaris/native/libjsig/jsig.c

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.

---

src/hotspot/cpu/arm/interp_masm_arm.cpp

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

---

src/hotspot/os/windows/os_windows.cpp.

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()) {

---

src/hotspot/share/c1/c1_Canonicalizer.hpp

   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?

---

src/hotspot/share/c1/c1_LinearScan.cpp

  ConstantIntValue((jint)0);

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.

---

src/hotspot/share/ci/ciReplay.cpp

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

why should this be jint?

---

src/hotspot/share/classfile/altHashing.cpp

Okay this looks more consistent with jint.

---

src/hotspot/share/code/debugInfo.hpp

These changes seem wrong. We have:

ConstantLongValue(jlong value)
ConstantDoubleValue(jdouble value)

so we should have:

ConstantIntValue(jint value)

---

src/hotspot/share/code/relocInfo.cpp

Change seems unnecessary - int32_t is fine

---

src/hotspot/share/compiler/compileBroker.cpp
src/hotspot/share/compiler/compileBroker.hpp

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

---

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp

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;

??

---

src/hotspot/share/jvmci/jvmciRuntime.cpp

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.

---

src/hotspot/share/opto/mulnode.cpp

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

---

src/hotspot/share/opto/parse3.cpp

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!

---

src/hotspot/share/opto/phaseX.cpp

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.

---

src/hotspot/share/opto/type.cpp

1505 int TypeInt::hash(void) const {
1506   return java_add(java_add(_lo, _hi), java_add((jint)_widen,
(jint)Type::Int));
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.

---

src/hotspot/share/prims/jni.cpp

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

---

src/hotspot/share/prims/jvm.cpp

is_attachable is the terminology used in the JDK code.

---

src/hotspot/share/prims/jvmtiEnvBase.cpp
src/hotspot/share/prims/jvmtiImpl.cpp

Are you making parameters consistent with the fields they initialize?

---

src/hotspot/share/prims/jvmtiTagMap.cpp

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,

---

src/hotspot/share/runtime/perfData.cpp

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

---

src/hotspot/share/runtime/perfMemory.cpp
src/hotspot/share/runtime/perfMemory.hpp

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

---

src/java.base/share/native/include/jvm.h

Not clear why the jio functions are not also JNICALL ?

---

src/java.base/unix/native/include/jni_md.h

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

---

src/java.base/unix/native/include/jvm_md.h

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
different
   59 //       Linux releases. Here we define JVM_MAXPATHLEN to be
MAXPATHLEN + 1,
   60 //       so buffers declared in VM are always >= 4096.
   61 #define JVM_MAXPATHLEN MAXPATHLEN + 1

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.

---

src/java.base/windows/native/include/jvm_md.h

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! :)

Thanks,
David
-----


On 27/10/2017 6:44 AM, [hidden email] 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, [hidden email] 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
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
In reply to this post by Mandy Chung
Thank you for reviewing this, Mandy!
Coleen

On 10/26/17 5:27 PM, mandy chung wrote:

>
>
> On 10/26/17 1:34 PM, [hidden email] wrote:
>>
>>
>> On 10/26/17 2:47 PM, mandy chung wrote:
>>>
>>>
>>> On 10/26/17 2:57 AM, Magnus Ihse Bursie wrote:
>>>> A third option is to remove the support for link-time-opt entirely,
>>>> if it's not really used.
>>>>
>>>> * 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.)
>>>
>>> jvm.h and jvm_md.h are not public API and they are not copied to the
>>> $JAVA_HOME/includes directly.  This does raise the question that
>>> jvm*.h may belong to other location than
>>> src/java.base/{share,$OS}/native/include.
>>
>> I'm not sure where else it would go honestly, but it could be moved
>> outside this changeset.  The good thing about where it is, is that
>> the -I directives in the makefiles find both jni.h and jvm.h.
>
> I agree we should keep this location for this change (the location is
> a separate issue).  I reviewed the change that looks good to me.
>
> Mandy

Reply | Threaded
Open this post in threaded view
|

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

Magnus Ihse Bursie
In reply to this post by coleen.phillimore

On 2017-10-26 22:44, [hidden email] 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, [hidden email] 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.

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

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
In reply to this post by David Holmes


On 10/27/17 3:23 AM, David Holmes wrote:

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

Maybe someone can make that decision and change at a later date. The
point of this change is that there is now only one of these files that
is shared.  I don't think jvm.h and the jvm_md.h belong on the hotspot
sources for the jdk to find them in some random prims and os dependent
directories.

I'm happy to withdraw the CSR.  We generally use the CSR process to add
and remove JVM_ interfaces even though they're a private interface in
case some other JVM/JDK combination relies on them. The changes to these
files are very minor though and not likely to cause any even theoretical
incompatibility, so I'll withdraw it.

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

Yes, it does even though int and long are the same representation.

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

I didn't make any change unless there was was an error.  I have 100
failed JPRT jobs to confirm!  I eventually got a Windows system to
compile and test this on.   Actually some of the changes came out
better.  Cases where we use jint as a bool simply turned to int.  We do
not have an overload for bool for cmpxchg.

>
> Looking through I have a quite a few queries/comments - apologies in
> advance as I know how tedious this is:
>
> make/hotspot/lib/CompileLibjsig.gmk
> src/java.base/solaris/native/libjsig/jsig.c
>
> 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.

Can I patch up jsig in a separate RFE?  I don't remember why this broke
so I simply moved JSIG #define.  Is jsig obsolete?  Removing JVM_*
definitions generally requires a CSR.
>
> ---
>
> src/hotspot/cpu/arm/interp_masm_arm.cpp
>
> Why did you need to add the jvm.h include?
>

   tbz(Raccess_flags, JVM_ACC_SYNCHRONIZED_BIT, unlocked);

> ---
>
> src/hotspot/os/windows/os_windows.cpp.
>
> 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()) {

Yes, that's better to change process_exiting to a DWORD.  It needs a
DWORD cast to 0 in the cmpxchg.

         Atomic::cmpxchg(GetCurrentThreadId(), &process_exiting, (DWORD)0);

These templates are picky.

>
> ---
>
> src/hotspot/share/c1/c1_Canonicalizer.hpp
>
>   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?

No, they are not the same types even though they have the same
representation!

>
> ---
>
> src/hotspot/share/c1/c1_LinearScan.cpp
>
>  ConstantIntValue((jint)0);
>
> 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.

Yes, it caused an ambiguity.  0 matches 'int' but it doesn't match
'long' better than any pointer type.  So this cast is needed.
>
> ---
>
> src/hotspot/share/ci/ciReplay.cpp
>
> 793         jint* dims = NEW_RESOURCE_ARRAY(jint, rank);
>
> why should this be jint?

To avoid a cast from int* to jint* in the line below:

          value = kelem->multi_allocate(rank, dims, CHECK);


>
> ---
>
> src/hotspot/share/classfile/altHashing.cpp
>
> Okay this looks more consistent with jint.

Yes.  I translated this from some native code iirc.

>
> ---
>
> src/hotspot/share/code/debugInfo.hpp
>
> These changes seem wrong. We have:
>
> ConstantLongValue(jlong value)
> ConstantDoubleValue(jdouble value)
>
> so we should have:
>
> ConstantIntValue(jint value)

Again, there are multiple call sites with '0', which match int trivially
but are confused with long.  It's less consistent I agree but better to
not cast all the call sites.
>
> ---
>
> src/hotspot/share/code/relocInfo.cpp
>
> Change seems unnecessary - int32_t is fine
>

No, int32_t doesn't match the calls below it.  They all assume _lo and
_hi are jint.
> ---
>
> src/hotspot/share/compiler/compileBroker.cpp
> src/hotspot/share/compiler/compileBroker.hpp
>
> I see a complete mix of int and jint in this class, so why make the
> one change you did ??

This is another case of using jint as a flag with cmpxchg.  The
templates for cmpxchg want the types to match and 0 and 1 are
essentially 'int'.  This is a lot cleaner this way.

>
> ---
>
> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
>
> 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;
>

There's a conversion from O_BUFLEN from int to long in 1701 and 1702.  
MIN2 is a template that wants the types to match exactly.

> ??
>
> ---
>
> src/hotspot/share/jvmci/jvmciRuntime.cpp
>
> 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.

I can't leave it alone because it doesn't compile that way.  This was
the minimal change and yea, does look a bit inconsistent.
>
> ---
>
> src/hotspot/share/opto/mulnode.cpp
>
> Okay TypeInt has jint parts, so the remaining int32_t declarations (A,
> B, C, D) should also be jint.

Yes.  c2 uses jint types.

>
> ---
>
> src/hotspot/share/opto/parse3.cpp
>
> 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!

Everything in globals.hpp is intx.  That's a thread that I don't want to
pull on!

Changed dim_con to int.
>
> ---
>
> src/hotspot/share/opto/phaseX.cpp
>
> 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.

huh?  really?

>
> ---
>
> src/hotspot/share/opto/type.cpp
>
> 1505 int TypeInt::hash(void) const {
> 1506   return java_add(java_add(_lo, _hi), java_add((jint)_widen,
> (jint)Type::Int));
> 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'm not messing with this, this is the minimal in type fixing that I'm
going to do here.
>
> ---
>
> src/hotspot/share/prims/jni.cpp
>
> I think vm_created should be a bool. In fact all the fields you
> changed are logically bools - do Atomics work for bool now?

No, they do not.   I had thought bool would be better originally too.
>
> ---
>
> src/hotspot/share/prims/jvm.cpp
>
> is_attachable is the terminology used in the JDK code.

Well the JDK version had is_attach_supported() as the flag name so I
used that in this one place.
>
> ---
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp
> src/hotspot/share/prims/jvmtiImpl.cpp
>
> Are you making parameters consistent with the fields they initialize?

They're consistent with the declarations now.

>
> ---
>
> src/hotspot/share/prims/jvmtiTagMap.cpp
>
> 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,

Right for consistency with the declarations.
>
> ---
>
> src/hotspot/share/runtime/perfData.cpp
>
> Callers pass both jint and int, so param type seems arbitrary.

They are, but importantly they match the declarations.
>
> ---
>
> src/hotspot/share/runtime/perfMemory.cpp
> src/hotspot/share/runtime/perfMemory.hpp
>
> PerfMemory::_initialized should ideally be a bool - can OrderAccess
> handle that now?

Nope.
>
> ---
>
> src/java.base/share/native/include/jvm.h
>
> Not clear why the jio functions are not also JNICALL ?

They are now.  The JDK version didn't have JNICALL.  JVM needs JNICALL. 
I can't tell you why JDK didn't need JNICALL linkage.
>
> ---
>
> src/java.base/unix/native/include/jni_md.h
>
> There is no need to special case ARM. The differences in the existing
> code were for LTO support and that is now irrelevant.

See discussion with Magnus.   We still build ARM for jdk10/hs so I
needed this conditional or of course I wouldn't have added it.  We can
remove it with LTO support.

>
> ---
>
> src/java.base/unix/native/include/jvm_md.h
>
> 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
> different
>   59 //       Linux releases. Here we define JVM_MAXPATHLEN to be
> MAXPATHLEN + 1,
>   60 //       so buffers declared in VM are always >= 4096.
>   61 #define JVM_MAXPATHLEN MAXPATHLEN + 1
>
> 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.

All of the unix platforms had MAXPATHLEN+1.  I'll leave it for now and
we can investigate that further.

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

Ok.  I'll add this.
>
> ---
>
> src/java.base/windows/native/include/jvm_md.h
>
> Given the differences between the two versions either something has
> been broken or "extern C" declarations are not needed :)

Well, they are needed for Hotspot to build and do not prevent jdk from
building.  I don't know what was broken.
>
> ---
>
> That was a really painful way to spend most of my Friday. TGIF! :)

Thanks for going through it.  See comments inline for changes.
Generating a webrev takes hours so I'm not going to do that unless you
insist.

Thanks,
Coleen


>
> Thanks,
> David
> -----
>
>
> On 27/10/2017 6:44 AM, [hidden email] 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, [hidden email] 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
>>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
In reply to this post by Magnus Ihse Bursie


On 10/27/17 7:44 AM, Magnus Ihse Bursie wrote:

>
> On 2017-10-26 22:44, [hidden email] 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, [hidden email] 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
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by Magnus Ihse Bursie
Magnus,

LTO is irrelevant now.

David

On 27/10/2017 9:44 PM, Magnus Ihse Bursie wrote:

>
> On 2017-10-26 22:44, [hidden email] 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, [hidden email] 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.



> /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
>>>>
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by coleen.phillimore
On 27/10/2017 10:13 PM, [hidden email] wrote:

>
>
> On 10/27/17 3:23 AM, David Holmes wrote:
>> 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.
>
> Maybe someone can make that decision and change at a later date. The
> point of this change is that there is now only one of these files that
> is shared.  I don't think jvm.h and the jvm_md.h belong on the hotspot
> sources for the jdk to find them in some random prims and os dependent
> directories.

The one file that is needed is a hotspot file - jvm.h defines the
interface that hotspot exports via jvm.cpp.

If you leave jvm.h in hotspot/prims then a very large chunk of your
boilerplate changes are not needed. The JDK code doesn't care what the
name of the directory is - whatever it is just gets added as a -I
directive (the JDK code will include "jvm.h" not "prims/jvm.h" the way
hotspot sources do.

This isn't something we want to change back or move again later.
Whatever we do now we live with.

> I'm happy to withdraw the CSR.  We generally use the CSR process to add
> and remove JVM_ interfaces even though they're a private interface in
> case some other JVM/JDK combination relies on them. The changes to these
> files are very minor though and not likely to cause any even theoretical
> incompatibility, so I'll withdraw it.
>>
>> 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?
>
> Yes, it does even though int and long are the same representation.

And what an absolute mess that makes. :(

>>
>>> 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.
>
> I didn't make any change unless there was was an error.  I have 100
> failed JPRT jobs to confirm!  I eventually got a Windows system to
> compile and test this on.   Actually some of the changes came out
> better.  Cases where we use jint as a bool simply turned to int.  We do
> not have an overload for bool for cmpxchg.

That's unfortunate - ditto for OrderAccess.

>>
>> Looking through I have a quite a few queries/comments - apologies in
>> advance as I know how tedious this is:
>>
>> make/hotspot/lib/CompileLibjsig.gmk
>> src/java.base/solaris/native/libjsig/jsig.c
>>
>> 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.
>
> Can I patch up jsig in a separate RFE?  I don't remember why this broke
> so I simply moved JSIG #define.  Is jsig obsolete?  Removing JVM_*
> definitions generally requires a CSR.

I did say "As a follow up". jsig is not obsolete but the jsig versioning
code, only used by Solaris, is.

>>
>> ---
>>
>> src/hotspot/cpu/arm/interp_masm_arm.cpp
>>
>> Why did you need to add the jvm.h include?
>>
>
>    tbz(Raccess_flags, JVM_ACC_SYNCHRONIZED_BIT, unlocked);

Okay. I'm not going to try and figure out how this code found this before.

>> ---
>>
>> src/hotspot/os/windows/os_windows.cpp.
>>
>> 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()) {
>
> Yes, that's better to change process_exiting to a DWORD.  It needs a
> DWORD cast to 0 in the cmpxchg.
>
>          Atomic::cmpxchg(GetCurrentThreadId(), &process_exiting, (DWORD)0);
>
> These templates are picky.

Yes - their inability to deal with literals is extremely frustrating.

>>
>> ---
>>
>> src/hotspot/share/c1/c1_Canonicalizer.hpp
>>
>>   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?
>
> No, they are not the same types even though they have the same
> representation!

This is truly unfortunate.

>>
>> ---
>>
>> src/hotspot/share/c1/c1_LinearScan.cpp
>>
>>  ConstantIntValue((jint)0);
>>
>> 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.
>
> Yes, it caused an ambiguity.  0 matches 'int' but it doesn't match
> 'long' better than any pointer type.  So this cast is needed.

But you changed the constructor to take an int!

  class ConstantIntValue: public ScopeValue {
   private:
-  jint _value;
+  int _value;
   public:
-  ConstantIntValue(jint value)         { _value = value; }
+  ConstantIntValue(int value)          { _value = value; }


>>
>> ---
>>
>> src/hotspot/share/ci/ciReplay.cpp
>>
>> 793         jint* dims = NEW_RESOURCE_ARRAY(jint, rank);
>>
>> why should this be jint?
>
> To avoid a cast from int* to jint* in the line below:
>
>           value = kelem->multi_allocate(rank, dims, CHECK);
>
>
>>
>> ---
>>
>> src/hotspot/share/classfile/altHashing.cpp
>>
>> Okay this looks more consistent with jint.
>
> Yes.  I translated this from some native code iirc.
>>
>> ---
>>
>> src/hotspot/share/code/debugInfo.hpp
>>
>> These changes seem wrong. We have:
>>
>> ConstantLongValue(jlong value)
>> ConstantDoubleValue(jdouble value)
>>
>> so we should have:
>>
>> ConstantIntValue(jint value)
>
> Again, there are multiple call sites with '0', which match int trivially
> but are confused with long.  It's less consistent I agree but better to
> not cast all the call sites.

This is really making a mess of the APIs - they should be a jint but we
declare them int because of a 0 casting problem. Can't we just use 0L?

>>
>> ---
>>
>> src/hotspot/share/code/relocInfo.cpp
>>
>> Change seems unnecessary - int32_t is fine
>>
>
> No, int32_t doesn't match the calls below it.  They all assume _lo and
> _hi are jint.
>> ---
>>
>> src/hotspot/share/compiler/compileBroker.cpp
>> src/hotspot/share/compiler/compileBroker.hpp
>>
>> I see a complete mix of int and jint in this class, so why make the
>> one change you did ??
>
> This is another case of using jint as a flag with cmpxchg.  The
> templates for cmpxchg want the types to match and 0 and 1 are
> essentially 'int'.  This is a lot cleaner this way.

<sigh>

>>
>> ---
>>
>> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
>>
>> 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;
>>
>
> There's a conversion from O_BUFLEN from int to long in 1701 and 1702.  
> MIN2 is a template that wants the types to match exactly.

$%^%$! templates!

>> ??
>>
>> ---
>>
>> src/hotspot/share/jvmci/jvmciRuntime.cpp
>>
>> 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.
>
> I can't leave it alone because it doesn't compile that way.  This was
> the minimal change and yea, does look a bit inconsistent.
>>
>> ---
>>
>> src/hotspot/share/opto/mulnode.cpp
>>
>> Okay TypeInt has jint parts, so the remaining int32_t declarations (A,
>> B, C, D) should also be jint.
>
> Yes.  c2 uses jint types.
>>
>> ---
>>
>> src/hotspot/share/opto/parse3.cpp
>>
>> 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!
>
> Everything in globals.hpp is intx.  That's a thread that I don't want to
> pull on!

We still have that limitation? <double sigh>

>
> Changed dim_con to int.
>>
>> ---
>>
>> src/hotspot/share/opto/phaseX.cpp
>>
>> 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.
>
> huh?  really?
>>
>> ---
>>
>> src/hotspot/share/opto/type.cpp
>>
>> 1505 int TypeInt::hash(void) const {
>> 1506   return java_add(java_add(_lo, _hi), java_add((jint)_widen,
>> (jint)Type::Int));
>> 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'm not messing with this, this is the minimal in type fixing that I'm
> going to do here.

<sigh>

>>
>> ---
>>
>> src/hotspot/share/prims/jni.cpp
>>
>> I think vm_created should be a bool. In fact all the fields you
>> changed are logically bools - do Atomics work for bool now?
>
> No, they do not.   I had thought bool would be better originally too.
>>
>> ---
>>
>> src/hotspot/share/prims/jvm.cpp
>>
>> is_attachable is the terminology used in the JDK code.
>
> Well the JDK version had is_attach_supported() as the flag name so I
> used that in this one place.
>>
>> ---
>>
>> src/hotspot/share/prims/jvmtiEnvBase.cpp
>> src/hotspot/share/prims/jvmtiImpl.cpp
>>
>> Are you making parameters consistent with the fields they initialize?
>
> They're consistent with the declarations now.
>>
>> ---
>>
>> src/hotspot/share/prims/jvmtiTagMap.cpp
>>
>> 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,
>
> Right for consistency with the declarations.
>>
>> ---
>>
>> src/hotspot/share/runtime/perfData.cpp
>>
>> Callers pass both jint and int, so param type seems arbitrary.
>
> They are, but importantly they match the declarations.
>>
>> ---
>>
>> src/hotspot/share/runtime/perfMemory.cpp
>> src/hotspot/share/runtime/perfMemory.hpp
>>
>> PerfMemory::_initialized should ideally be a bool - can OrderAccess
>> handle that now?
>
> Nope.
>>
>> ---
>>
>> src/java.base/share/native/include/jvm.h
>>
>> Not clear why the jio functions are not also JNICALL ?
>
> They are now.  The JDK version didn't have JNICALL.  JVM needs JNICALL.  
> I can't tell you why JDK didn't need JNICALL linkage.

?? JVM currently does not have JNICALL. But they are declared as "extern C".

>>
>> ---
>>
>> src/java.base/unix/native/include/jni_md.h
>>
>> There is no need to special case ARM. The differences in the existing
>> code were for LTO support and that is now irrelevant.
>
> See discussion with Magnus.   We still build ARM for jdk10/hs so I
> needed this conditional or of course I wouldn't have added it.  We can
> remove it with LTO support.

Those builds are gone - this is obsolete. But yes all LTO can be removed
later if you wish. Just trying to simplify things now.

>>
>> ---
>>
>> src/java.base/unix/native/include/jvm_md.h
>>
>> 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
>> different
>>   59 //       Linux releases. Here we define JVM_MAXPATHLEN to be
>> MAXPATHLEN + 1,
>>   60 //       so buffers declared in VM are always >= 4096.
>>   61 #define JVM_MAXPATHLEN MAXPATHLEN + 1
>>
>> 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.
>
> All of the unix platforms had MAXPATHLEN+1.  I'll leave it for now and
> we can investigate that further.

I see the following existing code:

src/java.base/unix/native/include/jvm_md.h:

#define JVM_MAXPATHLEN MAXPATHLEN

src/java.base/macosx/native/include/jvm_md.h

#define JVM_MAXPATHLEN MAXPATHLEN

src/hotspot/os/aix/jvm_aix.h

#define JVM_MAXPATHLEN MAXPATHLEN

src/hotspot/os/bsd/jvm_bsd.h

#define JVM_MAXPATHLEN MAXPATHLEN + 1  // blindly copied from Linux version

src/hotspot/os/linux/jvm_linux.h

#define JVM_MAXPATHLEN MAXPATHLEN + 1

src/hotspot/os/solaris/jvm_solaris.h

#define JVM_MAXPATHLEN MAXPATHLEN

This is a linux only hack (if you ignore the blind copy from linux into
the BSD code in the VM).

>>
>>  86 #define ASYNC_SIGNAL     SIGJVM2
>>
>> This only exists on Solaris so I think should be in #ifdef SOLARIS, to
>> make that clear.
>
> Ok.  I'll add this.
>>
>> ---
>>
>> src/java.base/windows/native/include/jvm_md.h
>>
>> Given the differences between the two versions either something has
>> been broken or "extern C" declarations are not needed :)
>
> Well, they are needed for Hotspot to build and do not prevent jdk from
> building.  I don't know what was broken.

We really need to understand this better. Maybe related to the map files
that expose the symbols. ??

>>
>> ---
>>
>> That was a really painful way to spend most of my Friday. TGIF! :)
>
> Thanks for going through it.  See comments inline for changes.
> Generating a webrev takes hours so I'm not going to do that unless you
> insist.

An incremental webrev shouldn't take long - right? You're a mq maestro
now. :)

If you can reasonably produce an incremental webrev once you've settled
on all the comments/issues that would be good.

Thanks,
David

> Thanks,
> Coleen
>
>
>>
>> Thanks,
>> David
>> -----
>>
>>
>> On 27/10/2017 6:44 AM, [hidden email] 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, [hidden email] 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
>>>>>
>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore


On 10/27/17 9:37 AM, David Holmes wrote:

> On 27/10/2017 10:13 PM, [hidden email] wrote:
>>
>>
>> On 10/27/17 3:23 AM, David Holmes wrote:
>>> 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.
>>
>> Maybe someone can make that decision and change at a later date. The
>> point of this change is that there is now only one of these files
>> that is shared.  I don't think jvm.h and the jvm_md.h belong on the
>> hotspot sources for the jdk to find them in some random prims and os
>> dependent directories.
>
> The one file that is needed is a hotspot file - jvm.h defines the
> interface that hotspot exports via jvm.cpp.
>
> If you leave jvm.h in hotspot/prims then a very large chunk of your
> boilerplate changes are not needed. The JDK code doesn't care what the
> name of the directory is - whatever it is just gets added as a -I
> directive (the JDK code will include "jvm.h" not "prims/jvm.h" the way
> hotspot sources do.
>
> This isn't something we want to change back or move again later.
> Whatever we do now we live with.

I think it belongs with jni.h and I think the core libraries group would
agree.   It seems more natural there than buried in the hotspot prims
directory.  I guess this is on hold while we have this debate.   Sigh.

Actually with -I directives, changing to jvm.h from prims/jvm.h would
still work.   Maybe we should change the name to jvm.hpp since it's
jvm.cpp though?   Or maybe just have two divergent copies and close this
as WNF.

>
>> I'm happy to withdraw the CSR.  We generally use the CSR process to
>> add and remove JVM_ interfaces even though they're a private
>> interface in case some other JVM/JDK combination relies on them. The
>> changes to these files are very minor though and not likely to cause
>> any even theoretical incompatibility, so I'll withdraw it.
>>>
>>> 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?
>>
>> Yes, it does even though int and long are the same representation.
>
> And what an absolute mess that makes. :(
>
>>>
>>>> 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.
>>
>> I didn't make any change unless there was was an error.  I have 100
>> failed JPRT jobs to confirm!  I eventually got a Windows system to
>> compile and test this on.   Actually some of the changes came out
>> better.  Cases where we use jint as a bool simply turned to int.  We
>> do not have an overload for bool for cmpxchg.
>
> That's unfortunate - ditto for OrderAccess.
>
>>>
>>> Looking through I have a quite a few queries/comments - apologies in
>>> advance as I know how tedious this is:
>>>
>>> make/hotspot/lib/CompileLibjsig.gmk
>>> src/java.base/solaris/native/libjsig/jsig.c
>>>
>>> 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.
>>
>> Can I patch up jsig in a separate RFE?  I don't remember why this
>> broke so I simply moved JSIG #define.  Is jsig obsolete? Removing
>> JVM_* definitions generally requires a CSR.
>
> I did say "As a follow up". jsig is not obsolete but the jsig
> versioning code, only used by Solaris, is.
>
>>>
>>> ---
>>>
>>> src/hotspot/cpu/arm/interp_masm_arm.cpp
>>>
>>> Why did you need to add the jvm.h include?
>>>
>>
>>    tbz(Raccess_flags, JVM_ACC_SYNCHRONIZED_BIT, unlocked);
>
> Okay. I'm not going to try and figure out how this code found this
> before.
>
>>> ---
>>>
>>> src/hotspot/os/windows/os_windows.cpp.
>>>
>>> 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()) {
>>
>> Yes, that's better to change process_exiting to a DWORD.  It needs a
>> DWORD cast to 0 in the cmpxchg.
>>
>>          Atomic::cmpxchg(GetCurrentThreadId(), &process_exiting,
>> (DWORD)0);
>>
>> These templates are picky.
>
> Yes - their inability to deal with literals is extremely frustrating.
>
>>>
>>> ---
>>>
>>> src/hotspot/share/c1/c1_Canonicalizer.hpp
>>>
>>>   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?
>>
>> No, they are not the same types even though they have the same
>> representation!
>
> This is truly unfortunate.
>
>>>
>>> ---
>>>
>>> src/hotspot/share/c1/c1_LinearScan.cpp
>>>
>>>  ConstantIntValue((jint)0);
>>>
>>> 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.
>>
>> Yes, it caused an ambiguity.  0 matches 'int' but it doesn't match
>> 'long' better than any pointer type.  So this cast is needed.
>
> But you changed the constructor to take an int!
>
>  class ConstantIntValue: public ScopeValue {
>   private:
> -  jint _value;
> +  int _value;
>   public:
> -  ConstantIntValue(jint value)         { _value = value; }
> +  ConstantIntValue(int value)          { _value = value; }
>
>

Okay I removed this cast.

>>> ---
>>>
>>> src/hotspot/share/ci/ciReplay.cpp
>>>
>>> 793         jint* dims = NEW_RESOURCE_ARRAY(jint, rank);
>>>
>>> why should this be jint?
>>
>> To avoid a cast from int* to jint* in the line below:
>>
>>           value = kelem->multi_allocate(rank, dims, CHECK);
>>
>>
>>>
>>> ---
>>>
>>> src/hotspot/share/classfile/altHashing.cpp
>>>
>>> Okay this looks more consistent with jint.
>>
>> Yes.  I translated this from some native code iirc.
>>>
>>> ---
>>>
>>> src/hotspot/share/code/debugInfo.hpp
>>>
>>> These changes seem wrong. We have:
>>>
>>> ConstantLongValue(jlong value)
>>> ConstantDoubleValue(jdouble value)
>>>
>>> so we should have:
>>>
>>> ConstantIntValue(jint value)
>>
>> Again, there are multiple call sites with '0', which match int
>> trivially but are confused with long.  It's less consistent I agree
>> but better to not cast all the call sites.
>
> This is really making a mess of the APIs - they should be a jint but
> we declare them int because of a 0 casting problem. Can't we just use 0L?

There aren't that many casts.  You're right, that would have been better
in some places.

>>>
>>> ---
>>>
>>> src/hotspot/share/code/relocInfo.cpp
>>>
>>> Change seems unnecessary - int32_t is fine
>>>
>>
>> No, int32_t doesn't match the calls below it.  They all assume _lo
>> and _hi are jint.
>>> ---
>>>
>>> src/hotspot/share/compiler/compileBroker.cpp
>>> src/hotspot/share/compiler/compileBroker.hpp
>>>
>>> I see a complete mix of int and jint in this class, so why make the
>>> one change you did ??
>>
>> This is another case of using jint as a flag with cmpxchg.  The
>> templates for cmpxchg want the types to match and 0 and 1 are
>> essentially 'int'.  This is a lot cleaner this way.
>
> <sigh>
>
>>>
>>> ---
>>>
>>> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
>>>
>>> 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;
>>>
>>
>> There's a conversion from O_BUFLEN from int to long in 1701 and
>> 1702.   MIN2 is a template that wants the types to match exactly.
>
> $%^%$! templates!
>
>>> ??
>>>
>>> ---
>>>
>>> src/hotspot/share/jvmci/jvmciRuntime.cpp
>>>
>>> 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.
>>
>> I can't leave it alone because it doesn't compile that way. This was
>> the minimal change and yea, does look a bit inconsistent.
>>>
>>> ---
>>>
>>> src/hotspot/share/opto/mulnode.cpp
>>>
>>> Okay TypeInt has jint parts, so the remaining int32_t declarations
>>> (A, B, C, D) should also be jint.
>>
>> Yes.  c2 uses jint types.
>>>
>>> ---
>>>
>>> src/hotspot/share/opto/parse3.cpp
>>>
>>> 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!
>>
>> Everything in globals.hpp is intx.  That's a thread that I don't want
>> to pull on!
>
> We still have that limitation? <double sigh>
>>
>> Changed dim_con to int.
>>>
>>> ---
>>>
>>> src/hotspot/share/opto/phaseX.cpp
>>>
>>> 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.
>>
>> huh?  really?
>>>
>>> ---
>>>
>>> src/hotspot/share/opto/type.cpp
>>>
>>> 1505 int TypeInt::hash(void) const {
>>> 1506   return java_add(java_add(_lo, _hi), java_add((jint)_widen,
>>> (jint)Type::Int));
>>> 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'm not messing with this, this is the minimal in type fixing that
>> I'm going to do here.
>
> <sigh>
>
>>>
>>> ---
>>>
>>> src/hotspot/share/prims/jni.cpp
>>>
>>> I think vm_created should be a bool. In fact all the fields you
>>> changed are logically bools - do Atomics work for bool now?
>>
>> No, they do not.   I had thought bool would be better originally too.
>>>
>>> ---
>>>
>>> src/hotspot/share/prims/jvm.cpp
>>>
>>> is_attachable is the terminology used in the JDK code.
>>
>> Well the JDK version had is_attach_supported() as the flag name so I
>> used that in this one place.
>>>
>>> ---
>>>
>>> src/hotspot/share/prims/jvmtiEnvBase.cpp
>>> src/hotspot/share/prims/jvmtiImpl.cpp
>>>
>>> Are you making parameters consistent with the fields they initialize?
>>
>> They're consistent with the declarations now.
>>>
>>> ---
>>>
>>> src/hotspot/share/prims/jvmtiTagMap.cpp
>>>
>>> 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,
>>
>> Right for consistency with the declarations.
>>>
>>> ---
>>>
>>> src/hotspot/share/runtime/perfData.cpp
>>>
>>> Callers pass both jint and int, so param type seems arbitrary.
>>
>> They are, but importantly they match the declarations.
>>>
>>> ---
>>>
>>> src/hotspot/share/runtime/perfMemory.cpp
>>> src/hotspot/share/runtime/perfMemory.hpp
>>>
>>> PerfMemory::_initialized should ideally be a bool - can OrderAccess
>>> handle that now?
>>
>> Nope.
>>>
>>> ---
>>>
>>> src/java.base/share/native/include/jvm.h
>>>
>>> Not clear why the jio functions are not also JNICALL ?
>>
>> They are now.  The JDK version didn't have JNICALL.  JVM needs
>> JNICALL.  I can't tell you why JDK didn't need JNICALL linkage.
>
> ?? JVM currently does not have JNICALL. But they are declared as
> "extern C".

This was a compilation error on Windows with JDK.   Maybe the C code in
the JDK doesn't complain about linkage differences.  I'll have to go
back and figure this out then.

>
>>>
>>> ---
>>>
>>> src/java.base/unix/native/include/jni_md.h
>>>
>>> There is no need to special case ARM. The differences in the
>>> existing code were for LTO support and that is now irrelevant.
>>
>> See discussion with Magnus.   We still build ARM for jdk10/hs so I
>> needed this conditional or of course I wouldn't have added it.  We
>> can remove it with LTO support.
>
> Those builds are gone - this is obsolete. But yes all LTO can be
> removed later if you wish. Just trying to simplify things now.
>
>>>
>>> ---
>>>
>>> src/java.base/unix/native/include/jvm_md.h
>>>
>>> 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
>>> different
>>>   59 //       Linux releases. Here we define JVM_MAXPATHLEN to be
>>> MAXPATHLEN + 1,
>>>   60 //       so buffers declared in VM are always >= 4096.
>>>   61 #define JVM_MAXPATHLEN MAXPATHLEN + 1
>>>
>>> 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.
>>
>> All of the unix platforms had MAXPATHLEN+1.  I'll leave it for now
>> and we can investigate that further.
>
> I see the following existing code:
>
> src/java.base/unix/native/include/jvm_md.h:
>
> #define JVM_MAXPATHLEN MAXPATHLEN
>
> src/java.base/macosx/native/include/jvm_md.h
>
> #define JVM_MAXPATHLEN MAXPATHLEN
>
> src/hotspot/os/aix/jvm_aix.h
>
> #define JVM_MAXPATHLEN MAXPATHLEN
>
> src/hotspot/os/bsd/jvm_bsd.h
>
> #define JVM_MAXPATHLEN MAXPATHLEN + 1  // blindly copied from Linux
> version
>
> src/hotspot/os/linux/jvm_linux.h
>
> #define JVM_MAXPATHLEN MAXPATHLEN + 1
>
> src/hotspot/os/solaris/jvm_solaris.h
>
> #define JVM_MAXPATHLEN MAXPATHLEN
>
> This is a linux only hack (if you ignore the blind copy from linux
> into the BSD code in the VM).

Oh, thanks, so should I add a bunch of ifdefs then?  Or do you think
having MAXPATHLEN + 1 will really break the other platforms?  Do you
really see this as a problem or are you just pointing out inconsistency?

>
>>>
>>>  86 #define ASYNC_SIGNAL     SIGJVM2
>>>
>>> This only exists on Solaris so I think should be in #ifdef SOLARIS,
>>> to make that clear.
>>
>> Ok.  I'll add this.
>>>
>>> ---
>>>
>>> src/java.base/windows/native/include/jvm_md.h
>>>
>>> Given the differences between the two versions either something has
>>> been broken or "extern C" declarations are not needed :)
>>
>> Well, they are needed for Hotspot to build and do not prevent jdk
>> from building.  I don't know what was broken.
>
> We really need to understand this better. Maybe related to the map
> files that expose the symbols. ??

They're needed because the JDK files are written mostly in C and that
doesn't complain about the linkage difference.  Hotspot files are in C++
which does complain.

>
>>>
>>> ---
>>>
>>> That was a really painful way to spend most of my Friday. TGIF! :)
>>
>> Thanks for going through it.  See comments inline for changes.
>> Generating a webrev takes hours so I'm not going to do that unless
>> you insist.
>
> An incremental webrev shouldn't take long - right? You're a mq maestro
> now. :)

Well I generally trash a repository whenever I use mq but sure.
>
> If you can reasonably produce an incremental webrev once you've
> settled on all the comments/issues that would be good.

Ok, sure.

Coleen

>
> Thanks,
> David
>
>> Thanks,
>> Coleen
>>
>>
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>
>>> On 27/10/2017 6:44 AM, [hidden email] 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, [hidden email] 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
>>>>>>
>>>>>
>>>>
>>

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
In reply to this post by David Holmes


On 10/27/17 9:37 AM, David Holmes wrote:

>>> src/hotspot/share/c1/c1_LinearScan.cpp
>>>
>>>  ConstantIntValue((jint)0);
>>>
>>> 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.
>>
>> Yes, it caused an ambiguity.  0 matches 'int' but it doesn't match
>> 'long' better than any pointer type.  So this cast is needed.
>
> But you changed the constructor to take an int!
>
>  class ConstantIntValue: public ScopeValue {
>   private:
> -  jint _value;
> +  int _value;
>   public:
> -  ConstantIntValue(jint value)         { _value = value; }
> +  ConstantIntValue(int value)          { _value = value; }
>
I changed this back to not take an int and changed c1_LinearScan.cpp to
have the (jint)0 cast and output.cp needed (jint)0 casts.  0L doesn't
work for platforms where jint is an 'int' rather than a long because
it's ambiguous with the functions that take a pointer type.
Probably better to keep the type of ConstantIntValue consistent with j
types.

Thanks,
Coleen
Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung
In reply to this post by coleen.phillimore


On 10/27/17 7:08 AM, [hidden email] wrote:

>
>
> On 10/27/17 9:37 AM, David Holmes wrote:
>>
>> The one file that is needed is a hotspot file - jvm.h defines the
>> interface that hotspot exports via jvm.cpp.
>>
>> If you leave jvm.h in hotspot/prims then a very large chunk of your
>> boilerplate changes are not needed. The JDK code doesn't care what
>> the name of the directory is - whatever it is just gets added as a -I
>> directive (the JDK code will include "jvm.h" not "prims/jvm.h" the
>> way hotspot sources do.
>>
>> This isn't something we want to change back or move again later.
>> Whatever we do now we live with.
>
> I think it belongs with jni.h and I think the core libraries group
> would agree.   It seems more natural there than buried in the hotspot
> prims directory.  I guess this is on hold while we have this debate.  
> Sigh.
>
> Actually with -I directives, changing to jvm.h from prims/jvm.h would
> still work.   Maybe we should change the name to jvm.hpp since it's
> jvm.cpp though?   Or maybe just have two divergent copies and close
> this as WNF.

I also think hotspot/prims is not a good location.
src/java.base/share/include is a well-defined location for native header
files.  Maybe internal header files could be placed in include/internal
but this is a separate issue .  I should create an issue for jvm.h and
jmm.h (I looked at the files under the include directory and jvm.h and
jmm.h are the only two internal header files in the include directory).

I do think removing the duplicated copy of jvm.h is a good change. This
is finally possible with the consolidated repository and we no longer
need to update two copies of jvm.h for any change to the JVM
interface.   This change will work with -I directive setting to the new
location, if changed later.

What do you think?

Mandy
Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore


On 10/27/17 1:47 PM, mandy chung wrote:

>
>
> On 10/27/17 7:08 AM, [hidden email] wrote:
>>
>>
>> On 10/27/17 9:37 AM, David Holmes wrote:
>>>
>>> The one file that is needed is a hotspot file - jvm.h defines the
>>> interface that hotspot exports via jvm.cpp.
>>>
>>> If you leave jvm.h in hotspot/prims then a very large chunk of your
>>> boilerplate changes are not needed. The JDK code doesn't care what
>>> the name of the directory is - whatever it is just gets added as a
>>> -I directive (the JDK code will include "jvm.h" not "prims/jvm.h"
>>> the way hotspot sources do.
>>>
>>> This isn't something we want to change back or move again later.
>>> Whatever we do now we live with.
>>
>> I think it belongs with jni.h and I think the core libraries group
>> would agree.   It seems more natural there than buried in the hotspot
>> prims directory.  I guess this is on hold while we have this
>> debate.   Sigh.
>>
>> Actually with -I directives, changing to jvm.h from prims/jvm.h would
>> still work.   Maybe we should change the name to jvm.hpp since it's
>> jvm.cpp though?   Or maybe just have two divergent copies and close
>> this as WNF.
>
> I also think hotspot/prims is not a good location.
> src/java.base/share/include is a well-defined location for native
> header files.  Maybe internal header files could be placed in
> include/internal but this is a separate issue .  I should create an
> issue for jvm.h and jmm.h (I looked at the files under the include
> directory and jvm.h and jmm.h are the only two internal header files
> in the include directory).
>
> I do think removing the duplicated copy of jvm.h is a good change. 
> This is finally possible with the consolidated repository and we no
> longer need to update two copies of jvm.h for any change to the JVM
> interface.   This change will work with -I directive setting to the
> new location, if changed later.
>
> What do you think?

I agree.  I'm not really bothered by it being in
src/java/base/share/include in the first place though.   Only jni.h and
jni_md.h are copied into the images, so this seems a bit pained to make
jvm.h be in some other directory.  But your call, really.

Thanks,
Coleen

>
> Mandy

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by Mandy Chung
On 28/10/2017 3:47 AM, mandy chung wrote:

> On 10/27/17 7:08 AM, [hidden email] wrote:
>>
>>
>> On 10/27/17 9:37 AM, David Holmes wrote:
>>>
>>> The one file that is needed is a hotspot file - jvm.h defines the
>>> interface that hotspot exports via jvm.cpp.
>>>
>>> If you leave jvm.h in hotspot/prims then a very large chunk of your
>>> boilerplate changes are not needed. The JDK code doesn't care what
>>> the name of the directory is - whatever it is just gets added as a -I
>>> directive (the JDK code will include "jvm.h" not "prims/jvm.h" the
>>> way hotspot sources do.
>>>
>>> This isn't something we want to change back or move again later.
>>> Whatever we do now we live with.
>>
>> I think it belongs with jni.h and I think the core libraries group
>> would agree.   It seems more natural there than buried in the hotspot
>> prims directory.  I guess this is on hold while we have this debate.  
>> Sigh.
>>
>> Actually with -I directives, changing to jvm.h from prims/jvm.h would
>> still work.   Maybe we should change the name to jvm.hpp since it's
>> jvm.cpp though?   Or maybe just have two divergent copies and close
>> this as WNF.
>
> I also think hotspot/prims is not a good location.
> src/java.base/share/include is a well-defined location for native header
> files.  Maybe internal header files could be placed in include/internal
> but this is a separate issue .  I should create an issue for jvm.h and
> jmm.h (I looked at the files under the include directory and jvm.h and
> jmm.h are the only two internal header files in the include directory).

Keeping it in prims avoids the need to touch many hotspot files, and
with no changes needed on the JDK side because we use a -I directive to
set the include path anyway. This is the exported VM interface so it
makes sense to me for it to be located in the VM sources.

But I'm not going to oppose this either way so it's up to Coleen.

> I do think removing the duplicated copy of jvm.h is a good change. This
> is finally possible with the consolidated repository and we no longer
> need to update two copies of jvm.h for any change to the JVM

Unfortunately we did not do this though - hence the divergence between
the two. The use of int versus long for jint is causing a real problem.

Coleen also hit the other issue on the head. The JNI and JVM interfaces
are C interfaces, not C++. The JDK code that uses them is compiled as C
- so all good. But the JVM code that implements them is compiled as C++,
and that is why we are getting issues with differing linkage directives.

David
-----

> interface.   This change will work with -I directive setting to the new
> location, if changed later.
>
> What do you think?
>
> Mandy
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by coleen.phillimore
Hi Coleen,

I've commented on the file location in response to Mandy's email.

The only issue I'm still concerned about is the JVM_MAXPATHLEN issue. I
think it is a bug to define a JVM_MAXPATHLEN that is bigger than the
platform MAXPATHLEN. I also would not want to see any change in
behaviour because of this - so AIX and Solaris should not get a
different JVM_MAXPATHLEN due to this refactoring change. So yes I think
this needs to be ifdef'd for Linux and reluctantly (because it was a
copy error) for OSX/BSD as well.

Thanks,
David

On 28/10/2017 12:08 AM, [hidden email] wrote:

>
>
> On 10/27/17 9:37 AM, David Holmes wrote:
>> On 27/10/2017 10:13 PM, [hidden email] wrote:
>>>
>>>
>>> On 10/27/17 3:23 AM, David Holmes wrote:
>>>> 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.
>>>
>>> Maybe someone can make that decision and change at a later date. The
>>> point of this change is that there is now only one of these files
>>> that is shared.  I don't think jvm.h and the jvm_md.h belong on the
>>> hotspot sources for the jdk to find them in some random prims and os
>>> dependent directories.
>>
>> The one file that is needed is a hotspot file - jvm.h defines the
>> interface that hotspot exports via jvm.cpp.
>>
>> If you leave jvm.h in hotspot/prims then a very large chunk of your
>> boilerplate changes are not needed. The JDK code doesn't care what the
>> name of the directory is - whatever it is just gets added as a -I
>> directive (the JDK code will include "jvm.h" not "prims/jvm.h" the way
>> hotspot sources do.
>>
>> This isn't something we want to change back or move again later.
>> Whatever we do now we live with.
>
> I think it belongs with jni.h and I think the core libraries group would
> agree.   It seems more natural there than buried in the hotspot prims
> directory.  I guess this is on hold while we have this debate.   Sigh.
>
> Actually with -I directives, changing to jvm.h from prims/jvm.h would
> still work.   Maybe we should change the name to jvm.hpp since it's
> jvm.cpp though?   Or maybe just have two divergent copies and close this
> as WNF.
>
>>
>>> I'm happy to withdraw the CSR.  We generally use the CSR process to
>>> add and remove JVM_ interfaces even though they're a private
>>> interface in case some other JVM/JDK combination relies on them. The
>>> changes to these files are very minor though and not likely to cause
>>> any even theoretical incompatibility, so I'll withdraw it.
>>>>
>>>> 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?
>>>
>>> Yes, it does even though int and long are the same representation.
>>
>> And what an absolute mess that makes. :(
>>
>>>>
>>>>> 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.
>>>
>>> I didn't make any change unless there was was an error.  I have 100
>>> failed JPRT jobs to confirm!  I eventually got a Windows system to
>>> compile and test this on.   Actually some of the changes came out
>>> better.  Cases where we use jint as a bool simply turned to int.  We
>>> do not have an overload for bool for cmpxchg.
>>
>> That's unfortunate - ditto for OrderAccess.
>>
>>>>
>>>> Looking through I have a quite a few queries/comments - apologies in
>>>> advance as I know how tedious this is:
>>>>
>>>> make/hotspot/lib/CompileLibjsig.gmk
>>>> src/java.base/solaris/native/libjsig/jsig.c
>>>>
>>>> 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.
>>>
>>> Can I patch up jsig in a separate RFE?  I don't remember why this
>>> broke so I simply moved JSIG #define.  Is jsig obsolete? Removing
>>> JVM_* definitions generally requires a CSR.
>>
>> I did say "As a follow up". jsig is not obsolete but the jsig
>> versioning code, only used by Solaris, is.
>>
>>>>
>>>> ---
>>>>
>>>> src/hotspot/cpu/arm/interp_masm_arm.cpp
>>>>
>>>> Why did you need to add the jvm.h include?
>>>>
>>>
>>>    tbz(Raccess_flags, JVM_ACC_SYNCHRONIZED_BIT, unlocked);
>>
>> Okay. I'm not going to try and figure out how this code found this
>> before.
>>
>>>> ---
>>>>
>>>> src/hotspot/os/windows/os_windows.cpp.
>>>>
>>>> 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()) {
>>>
>>> Yes, that's better to change process_exiting to a DWORD.  It needs a
>>> DWORD cast to 0 in the cmpxchg.
>>>
>>>          Atomic::cmpxchg(GetCurrentThreadId(), &process_exiting,
>>> (DWORD)0);
>>>
>>> These templates are picky.
>>
>> Yes - their inability to deal with literals is extremely frustrating.
>>
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/c1/c1_Canonicalizer.hpp
>>>>
>>>>   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?
>>>
>>> No, they are not the same types even though they have the same
>>> representation!
>>
>> This is truly unfortunate.
>>
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/c1/c1_LinearScan.cpp
>>>>
>>>>  ConstantIntValue((jint)0);
>>>>
>>>> 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.
>>>
>>> Yes, it caused an ambiguity.  0 matches 'int' but it doesn't match
>>> 'long' better than any pointer type.  So this cast is needed.
>>
>> But you changed the constructor to take an int!
>>
>>  class ConstantIntValue: public ScopeValue {
>>   private:
>> -  jint _value;
>> +  int _value;
>>   public:
>> -  ConstantIntValue(jint value)         { _value = value; }
>> +  ConstantIntValue(int value)          { _value = value; }
>>
>>
>
> Okay I removed this cast.
>
>>>> ---
>>>>
>>>> src/hotspot/share/ci/ciReplay.cpp
>>>>
>>>> 793         jint* dims = NEW_RESOURCE_ARRAY(jint, rank);
>>>>
>>>> why should this be jint?
>>>
>>> To avoid a cast from int* to jint* in the line below:
>>>
>>>           value = kelem->multi_allocate(rank, dims, CHECK);
>>>
>>>
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/classfile/altHashing.cpp
>>>>
>>>> Okay this looks more consistent with jint.
>>>
>>> Yes.  I translated this from some native code iirc.
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/code/debugInfo.hpp
>>>>
>>>> These changes seem wrong. We have:
>>>>
>>>> ConstantLongValue(jlong value)
>>>> ConstantDoubleValue(jdouble value)
>>>>
>>>> so we should have:
>>>>
>>>> ConstantIntValue(jint value)
>>>
>>> Again, there are multiple call sites with '0', which match int
>>> trivially but are confused with long.  It's less consistent I agree
>>> but better to not cast all the call sites.
>>
>> This is really making a mess of the APIs - they should be a jint but
>> we declare them int because of a 0 casting problem. Can't we just use 0L?
>
> There aren't that many casts.  You're right, that would have been better
> in some places.
>
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/code/relocInfo.cpp
>>>>
>>>> Change seems unnecessary - int32_t is fine
>>>>
>>>
>>> No, int32_t doesn't match the calls below it.  They all assume _lo
>>> and _hi are jint.
>>>> ---
>>>>
>>>> src/hotspot/share/compiler/compileBroker.cpp
>>>> src/hotspot/share/compiler/compileBroker.hpp
>>>>
>>>> I see a complete mix of int and jint in this class, so why make the
>>>> one change you did ??
>>>
>>> This is another case of using jint as a flag with cmpxchg.  The
>>> templates for cmpxchg want the types to match and 0 and 1 are
>>> essentially 'int'.  This is a lot cleaner this way.
>>
>> <sigh>
>>
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
>>>>
>>>> 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;
>>>>
>>>
>>> There's a conversion from O_BUFLEN from int to long in 1701 and
>>> 1702.   MIN2 is a template that wants the types to match exactly.
>>
>> $%^%$! templates!
>>
>>>> ??
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/jvmci/jvmciRuntime.cpp
>>>>
>>>> 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.
>>>
>>> I can't leave it alone because it doesn't compile that way. This was
>>> the minimal change and yea, does look a bit inconsistent.
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/opto/mulnode.cpp
>>>>
>>>> Okay TypeInt has jint parts, so the remaining int32_t declarations
>>>> (A, B, C, D) should also be jint.
>>>
>>> Yes.  c2 uses jint types.
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/opto/parse3.cpp
>>>>
>>>> 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!
>>>
>>> Everything in globals.hpp is intx.  That's a thread that I don't want
>>> to pull on!
>>
>> We still have that limitation? <double sigh>
>>>
>>> Changed dim_con to int.
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/opto/phaseX.cpp
>>>>
>>>> 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.
>>>
>>> huh?  really?
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/opto/type.cpp
>>>>
>>>> 1505 int TypeInt::hash(void) const {
>>>> 1506   return java_add(java_add(_lo, _hi), java_add((jint)_widen,
>>>> (jint)Type::Int));
>>>> 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'm not messing with this, this is the minimal in type fixing that
>>> I'm going to do here.
>>
>> <sigh>
>>
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/prims/jni.cpp
>>>>
>>>> I think vm_created should be a bool. In fact all the fields you
>>>> changed are logically bools - do Atomics work for bool now?
>>>
>>> No, they do not.   I had thought bool would be better originally too.
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/prims/jvm.cpp
>>>>
>>>> is_attachable is the terminology used in the JDK code.
>>>
>>> Well the JDK version had is_attach_supported() as the flag name so I
>>> used that in this one place.
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/prims/jvmtiEnvBase.cpp
>>>> src/hotspot/share/prims/jvmtiImpl.cpp
>>>>
>>>> Are you making parameters consistent with the fields they initialize?
>>>
>>> They're consistent with the declarations now.
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/prims/jvmtiTagMap.cpp
>>>>
>>>> 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,
>>>
>>> Right for consistency with the declarations.
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/runtime/perfData.cpp
>>>>
>>>> Callers pass both jint and int, so param type seems arbitrary.
>>>
>>> They are, but importantly they match the declarations.
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/runtime/perfMemory.cpp
>>>> src/hotspot/share/runtime/perfMemory.hpp
>>>>
>>>> PerfMemory::_initialized should ideally be a bool - can OrderAccess
>>>> handle that now?
>>>
>>> Nope.
>>>>
>>>> ---
>>>>
>>>> src/java.base/share/native/include/jvm.h
>>>>
>>>> Not clear why the jio functions are not also JNICALL ?
>>>
>>> They are now.  The JDK version didn't have JNICALL.  JVM needs
>>> JNICALL.  I can't tell you why JDK didn't need JNICALL linkage.
>>
>> ?? JVM currently does not have JNICALL. But they are declared as
>> "extern C".
>
> This was a compilation error on Windows with JDK.   Maybe the C code in
> the JDK doesn't complain about linkage differences.  I'll have to go
> back and figure this out then.
>>
>>>>
>>>> ---
>>>>
>>>> src/java.base/unix/native/include/jni_md.h
>>>>
>>>> There is no need to special case ARM. The differences in the
>>>> existing code were for LTO support and that is now irrelevant.
>>>
>>> See discussion with Magnus.   We still build ARM for jdk10/hs so I
>>> needed this conditional or of course I wouldn't have added it.  We
>>> can remove it with LTO support.
>>
>> Those builds are gone - this is obsolete. But yes all LTO can be
>> removed later if you wish. Just trying to simplify things now.
>>
>>>>
>>>> ---
>>>>
>>>> src/java.base/unix/native/include/jvm_md.h
>>>>
>>>> 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
>>>> different
>>>>   59 //       Linux releases. Here we define JVM_MAXPATHLEN to be
>>>> MAXPATHLEN + 1,
>>>>   60 //       so buffers declared in VM are always >= 4096.
>>>>   61 #define JVM_MAXPATHLEN MAXPATHLEN + 1
>>>>
>>>> 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.
>>>
>>> All of the unix platforms had MAXPATHLEN+1.  I'll leave it for now
>>> and we can investigate that further.
>>
>> I see the following existing code:
>>
>> src/java.base/unix/native/include/jvm_md.h:
>>
>> #define JVM_MAXPATHLEN MAXPATHLEN
>>
>> src/java.base/macosx/native/include/jvm_md.h
>>
>> #define JVM_MAXPATHLEN MAXPATHLEN
>>
>> src/hotspot/os/aix/jvm_aix.h
>>
>> #define JVM_MAXPATHLEN MAXPATHLEN
>>
>> src/hotspot/os/bsd/jvm_bsd.h
>>
>> #define JVM_MAXPATHLEN MAXPATHLEN + 1  // blindly copied from Linux
>> version
>>
>> src/hotspot/os/linux/jvm_linux.h
>>
>> #define JVM_MAXPATHLEN MAXPATHLEN + 1
>>
>> src/hotspot/os/solaris/jvm_solaris.h
>>
>> #define JVM_MAXPATHLEN MAXPATHLEN
>>
>> This is a linux only hack (if you ignore the blind copy from linux
>> into the BSD code in the VM).
>
> Oh, thanks, so should I add a bunch of ifdefs then?  Or do you think
> having MAXPATHLEN + 1 will really break the other platforms?  Do you
> really see this as a problem or are you just pointing out inconsistency?
>>
>>>>
>>>>  86 #define ASYNC_SIGNAL     SIGJVM2
>>>>
>>>> This only exists on Solaris so I think should be in #ifdef SOLARIS,
>>>> to make that clear.
>>>
>>> Ok.  I'll add this.
>>>>
>>>> ---
>>>>
>>>> src/java.base/windows/native/include/jvm_md.h
>>>>
>>>> Given the differences between the two versions either something has
>>>> been broken or "extern C" declarations are not needed :)
>>>
>>> Well, they are needed for Hotspot to build and do not prevent jdk
>>> from building.  I don't know what was broken.
>>
>> We really need to understand this better. Maybe related to the map
>> files that expose the symbols. ??
>
> They're needed because the JDK files are written mostly in C and that
> doesn't complain about the linkage difference.  Hotspot files are in C++
> which does complain.
>
>>
>>>>
>>>> ---
>>>>
>>>> That was a really painful way to spend most of my Friday. TGIF! :)
>>>
>>> Thanks for going through it.  See comments inline for changes.
>>> Generating a webrev takes hours so I'm not going to do that unless
>>> you insist.
>>
>> An incremental webrev shouldn't take long - right? You're a mq maestro
>> now. :)
>
> Well I generally trash a repository whenever I use mq but sure.
>>
>> If you can reasonably produce an incremental webrev once you've
>> settled on all the comments/issues that would be good.
>
> Ok, sure.
>
> Coleen
>>
>> Thanks,
>> David
>>
>>> Thanks,
>>> Coleen
>>>
>>>
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>
>>>> On 27/10/2017 6:44 AM, [hidden email] 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, [hidden email] 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
>>>>>>>
>>>>>>
>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
In reply to this post by David Holmes


On 10/28/17 3:46 AM, David Holmes wrote:

> On 28/10/2017 3:47 AM, mandy chung wrote:
>> On 10/27/17 7:08 AM, [hidden email] wrote:
>>>
>>>
>>> On 10/27/17 9:37 AM, David Holmes wrote:
>>>>
>>>> The one file that is needed is a hotspot file - jvm.h defines the
>>>> interface that hotspot exports via jvm.cpp.
>>>>
>>>> If you leave jvm.h in hotspot/prims then a very large chunk of your
>>>> boilerplate changes are not needed. The JDK code doesn't care what
>>>> the name of the directory is - whatever it is just gets added as a
>>>> -I directive (the JDK code will include "jvm.h" not "prims/jvm.h"
>>>> the way hotspot sources do.
>>>>
>>>> This isn't something we want to change back or move again later.
>>>> Whatever we do now we live with.
>>>
>>> I think it belongs with jni.h and I think the core libraries group
>>> would agree.   It seems more natural there than buried in the
>>> hotspot prims directory.  I guess this is on hold while we have this
>>> debate.   Sigh.
>>>
>>> Actually with -I directives, changing to jvm.h from prims/jvm.h
>>> would still work.   Maybe we should change the name to jvm.hpp since
>>> it's jvm.cpp though?   Or maybe just have two divergent copies and
>>> close this as WNF.
>>
>> I also think hotspot/prims is not a good location.
>> src/java.base/share/include is a well-defined location for native
>> header files.  Maybe internal header files could be placed in
>> include/internal but this is a separate issue .  I should create an
>> issue for jvm.h and jmm.h (I looked at the files under the include
>> directory and jvm.h and jmm.h are the only two internal header files
>> in the include directory).
>
> Keeping it in prims avoids the need to touch many hotspot files, and
> with no changes needed on the JDK side because we use a -I directive
> to set the include path anyway. This is the exported VM interface so
> it makes sense to me for it to be located in the VM sources.
>
> But I'm not going to oppose this either way so it's up to Coleen.

I've already disagreed that this file belongs in
src/hotspot/share/prims, so the include directive without prims is
preferred.  This allows putting jvm.h in a new place if/when that is
agreed upon.

>
>> I do think removing the duplicated copy of jvm.h is a good change.
>> This is finally possible with the consolidated repository and we no
>> longer need to update two copies of jvm.h for any change to the JVM
>
> Unfortunately we did not do this though - hence the divergence between
> the two. The use of int versus long for jint is causing a real problem.
>
> Coleen also hit the other issue on the head. The JNI and JVM
> interfaces are C interfaces, not C++. The JDK code that uses them is
> compiled as C - so all good. But the JVM code that implements them is
> compiled as C++, and that is why we are getting issues with differing
> linkage directives.

Well, there is now one source file for jvm.h and jni.h and their machine
dependent counterparts and 2500 lines of duplicated code is removed with
this change.  The issues with jint and linkages are resolved and tested
as well with this changeset.

Thanks,
Coleen

>
> David
> -----
>
>> interface.   This change will work with -I directive setting to the
>> new location, if changed later.
>>
>> What do you think?
>>
>> Mandy

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
In reply to this post by David Holmes


On 10/28/17 3:50 AM, David Holmes wrote:

> Hi Coleen,
>
> I've commented on the file location in response to Mandy's email.
>
> The only issue I'm still concerned about is the JVM_MAXPATHLEN issue.
> I think it is a bug to define a JVM_MAXPATHLEN that is bigger than the
> platform MAXPATHLEN. I also would not want to see any change in
> behaviour because of this - so AIX and Solaris should not get a
> different JVM_MAXPATHLEN due to this refactoring change. So yes I
> think this needs to be ifdef'd for Linux and reluctantly (because it
> was a copy error) for OSX/BSD as well.

#if defined(AIX) || defined(SOLARIS)
#define JVM_MAXPATHLEN MAXPATHLEN
#else
// Hack: MAXPATHLEN is 4095 on some Linux and 4096 on others. This may
//       cause problems if JVM and the rest of JDK are built on different
//       Linux releases. Here we define JVM_MAXPATHLEN to be MAXPATHLEN + 1,
//       so buffers declared in VM are always >= 4096.
#define JVM_MAXPATHLEN MAXPATHLEN + 1
#endif

Is this ok?

thanks,
Coleen

>
> Thanks,
> David
>
> On 28/10/2017 12:08 AM, [hidden email] wrote:
>>
>>
>> On 10/27/17 9:37 AM, David Holmes wrote:
>>> On 27/10/2017 10:13 PM, [hidden email] wrote:
>>>>
>>>>
>>>> On 10/27/17 3:23 AM, David Holmes wrote:
>>>>> 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.
>>>>
>>>> Maybe someone can make that decision and change at a later date.
>>>> The point of this change is that there is now only one of these
>>>> files that is shared.  I don't think jvm.h and the jvm_md.h belong
>>>> on the hotspot sources for the jdk to find them in some random
>>>> prims and os dependent directories.
>>>
>>> The one file that is needed is a hotspot file - jvm.h defines the
>>> interface that hotspot exports via jvm.cpp.
>>>
>>> If you leave jvm.h in hotspot/prims then a very large chunk of your
>>> boilerplate changes are not needed. The JDK code doesn't care what
>>> the name of the directory is - whatever it is just gets added as a
>>> -I directive (the JDK code will include "jvm.h" not "prims/jvm.h"
>>> the way hotspot sources do.
>>>
>>> This isn't something we want to change back or move again later.
>>> Whatever we do now we live with.
>>
>> I think it belongs with jni.h and I think the core libraries group
>> would agree.   It seems more natural there than buried in the hotspot
>> prims directory.  I guess this is on hold while we have this
>> debate.   Sigh.
>>
>> Actually with -I directives, changing to jvm.h from prims/jvm.h would
>> still work.   Maybe we should change the name to jvm.hpp since it's
>> jvm.cpp though?   Or maybe just have two divergent copies and close
>> this as WNF.
>>
>>>
>>>> I'm happy to withdraw the CSR.  We generally use the CSR process to
>>>> add and remove JVM_ interfaces even though they're a private
>>>> interface in case some other JVM/JDK combination relies on them.
>>>> The changes to these files are very minor though and not likely to
>>>> cause any even theoretical incompatibility, so I'll withdraw it.
>>>>>
>>>>> 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?
>>>>
>>>> Yes, it does even though int and long are the same representation.
>>>
>>> And what an absolute mess that makes. :(
>>>
>>>>>
>>>>>> 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.
>>>>
>>>> I didn't make any change unless there was was an error.  I have 100
>>>> failed JPRT jobs to confirm!  I eventually got a Windows system to
>>>> compile and test this on.   Actually some of the changes came out
>>>> better.  Cases where we use jint as a bool simply turned to int. 
>>>> We do not have an overload for bool for cmpxchg.
>>>
>>> That's unfortunate - ditto for OrderAccess.
>>>
>>>>>
>>>>> Looking through I have a quite a few queries/comments - apologies
>>>>> in advance as I know how tedious this is:
>>>>>
>>>>> make/hotspot/lib/CompileLibjsig.gmk
>>>>> src/java.base/solaris/native/libjsig/jsig.c
>>>>>
>>>>> 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.
>>>>
>>>> Can I patch up jsig in a separate RFE?  I don't remember why this
>>>> broke so I simply moved JSIG #define.  Is jsig obsolete? Removing
>>>> JVM_* definitions generally requires a CSR.
>>>
>>> I did say "As a follow up". jsig is not obsolete but the jsig
>>> versioning code, only used by Solaris, is.
>>>
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/cpu/arm/interp_masm_arm.cpp
>>>>>
>>>>> Why did you need to add the jvm.h include?
>>>>>
>>>>
>>>>    tbz(Raccess_flags, JVM_ACC_SYNCHRONIZED_BIT, unlocked);
>>>
>>> Okay. I'm not going to try and figure out how this code found this
>>> before.
>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/os/windows/os_windows.cpp.
>>>>>
>>>>> 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()) {
>>>>
>>>> Yes, that's better to change process_exiting to a DWORD.  It needs
>>>> a DWORD cast to 0 in the cmpxchg.
>>>>
>>>>          Atomic::cmpxchg(GetCurrentThreadId(), &process_exiting,
>>>> (DWORD)0);
>>>>
>>>> These templates are picky.
>>>
>>> Yes - their inability to deal with literals is extremely frustrating.
>>>
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/c1/c1_Canonicalizer.hpp
>>>>>
>>>>>   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?
>>>>
>>>> No, they are not the same types even though they have the same
>>>> representation!
>>>
>>> This is truly unfortunate.
>>>
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/c1/c1_LinearScan.cpp
>>>>>
>>>>>  ConstantIntValue((jint)0);
>>>>>
>>>>> 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.
>>>>
>>>> Yes, it caused an ambiguity.  0 matches 'int' but it doesn't match
>>>> 'long' better than any pointer type.  So this cast is needed.
>>>
>>> But you changed the constructor to take an int!
>>>
>>>  class ConstantIntValue: public ScopeValue {
>>>   private:
>>> -  jint _value;
>>> +  int _value;
>>>   public:
>>> -  ConstantIntValue(jint value)         { _value = value; }
>>> +  ConstantIntValue(int value)          { _value = value; }
>>>
>>>
>>
>> Okay I removed this cast.
>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/ci/ciReplay.cpp
>>>>>
>>>>> 793         jint* dims = NEW_RESOURCE_ARRAY(jint, rank);
>>>>>
>>>>> why should this be jint?
>>>>
>>>> To avoid a cast from int* to jint* in the line below:
>>>>
>>>>           value = kelem->multi_allocate(rank, dims, CHECK);
>>>>
>>>>
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/classfile/altHashing.cpp
>>>>>
>>>>> Okay this looks more consistent with jint.
>>>>
>>>> Yes.  I translated this from some native code iirc.
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/code/debugInfo.hpp
>>>>>
>>>>> These changes seem wrong. We have:
>>>>>
>>>>> ConstantLongValue(jlong value)
>>>>> ConstantDoubleValue(jdouble value)
>>>>>
>>>>> so we should have:
>>>>>
>>>>> ConstantIntValue(jint value)
>>>>
>>>> Again, there are multiple call sites with '0', which match int
>>>> trivially but are confused with long.  It's less consistent I agree
>>>> but better to not cast all the call sites.
>>>
>>> This is really making a mess of the APIs - they should be a jint but
>>> we declare them int because of a 0 casting problem. Can't we just
>>> use 0L?
>>
>> There aren't that many casts.  You're right, that would have been
>> better in some places.
>>
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/code/relocInfo.cpp
>>>>>
>>>>> Change seems unnecessary - int32_t is fine
>>>>>
>>>>
>>>> No, int32_t doesn't match the calls below it.  They all assume _lo
>>>> and _hi are jint.
>>>>> ---
>>>>>
>>>>> src/hotspot/share/compiler/compileBroker.cpp
>>>>> src/hotspot/share/compiler/compileBroker.hpp
>>>>>
>>>>> I see a complete mix of int and jint in this class, so why make
>>>>> the one change you did ??
>>>>
>>>> This is another case of using jint as a flag with cmpxchg. The
>>>> templates for cmpxchg want the types to match and 0 and 1 are
>>>> essentially 'int'.  This is a lot cleaner this way.
>>>
>>> <sigh>
>>>
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
>>>>>
>>>>> 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;
>>>>>
>>>>
>>>> There's a conversion from O_BUFLEN from int to long in 1701 and
>>>> 1702.   MIN2 is a template that wants the types to match exactly.
>>>
>>> $%^%$! templates!
>>>
>>>>> ??
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/jvmci/jvmciRuntime.cpp
>>>>>
>>>>> 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.
>>>>
>>>> I can't leave it alone because it doesn't compile that way. This
>>>> was the minimal change and yea, does look a bit inconsistent.
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/opto/mulnode.cpp
>>>>>
>>>>> Okay TypeInt has jint parts, so the remaining int32_t declarations
>>>>> (A, B, C, D) should also be jint.
>>>>
>>>> Yes.  c2 uses jint types.
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/opto/parse3.cpp
>>>>>
>>>>> 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!
>>>>
>>>> Everything in globals.hpp is intx.  That's a thread that I don't
>>>> want to pull on!
>>>
>>> We still have that limitation? <double sigh>
>>>>
>>>> Changed dim_con to int.
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/opto/phaseX.cpp
>>>>>
>>>>> 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.
>>>>
>>>> huh?  really?
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/opto/type.cpp
>>>>>
>>>>> 1505 int TypeInt::hash(void) const {
>>>>> 1506   return java_add(java_add(_lo, _hi), java_add((jint)_widen,
>>>>> (jint)Type::Int));
>>>>> 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'm not messing with this, this is the minimal in type fixing that
>>>> I'm going to do here.
>>>
>>> <sigh>
>>>
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/prims/jni.cpp
>>>>>
>>>>> I think vm_created should be a bool. In fact all the fields you
>>>>> changed are logically bools - do Atomics work for bool now?
>>>>
>>>> No, they do not.   I had thought bool would be better originally too.
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/prims/jvm.cpp
>>>>>
>>>>> is_attachable is the terminology used in the JDK code.
>>>>
>>>> Well the JDK version had is_attach_supported() as the flag name so
>>>> I used that in this one place.
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/prims/jvmtiEnvBase.cpp
>>>>> src/hotspot/share/prims/jvmtiImpl.cpp
>>>>>
>>>>> Are you making parameters consistent with the fields they initialize?
>>>>
>>>> They're consistent with the declarations now.
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/prims/jvmtiTagMap.cpp
>>>>>
>>>>> 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,
>>>>
>>>> Right for consistency with the declarations.
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/runtime/perfData.cpp
>>>>>
>>>>> Callers pass both jint and int, so param type seems arbitrary.
>>>>
>>>> They are, but importantly they match the declarations.
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/runtime/perfMemory.cpp
>>>>> src/hotspot/share/runtime/perfMemory.hpp
>>>>>
>>>>> PerfMemory::_initialized should ideally be a bool - can
>>>>> OrderAccess handle that now?
>>>>
>>>> Nope.
>>>>>
>>>>> ---
>>>>>
>>>>> src/java.base/share/native/include/jvm.h
>>>>>
>>>>> Not clear why the jio functions are not also JNICALL ?
>>>>
>>>> They are now.  The JDK version didn't have JNICALL.  JVM needs
>>>> JNICALL.  I can't tell you why JDK didn't need JNICALL linkage.
>>>
>>> ?? JVM currently does not have JNICALL. But they are declared as
>>> "extern C".
>>
>> This was a compilation error on Windows with JDK.   Maybe the C code
>> in the JDK doesn't complain about linkage differences. I'll have to
>> go back and figure this out then.
>>>
>>>>>
>>>>> ---
>>>>>
>>>>> src/java.base/unix/native/include/jni_md.h
>>>>>
>>>>> There is no need to special case ARM. The differences in the
>>>>> existing code were for LTO support and that is now irrelevant.
>>>>
>>>> See discussion with Magnus.   We still build ARM for jdk10/hs so I
>>>> needed this conditional or of course I wouldn't have added it.  We
>>>> can remove it with LTO support.
>>>
>>> Those builds are gone - this is obsolete. But yes all LTO can be
>>> removed later if you wish. Just trying to simplify things now.
>>>
>>>>>
>>>>> ---
>>>>>
>>>>> src/java.base/unix/native/include/jvm_md.h
>>>>>
>>>>> 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 different
>>>>>   59 //       Linux releases. Here we define JVM_MAXPATHLEN to be
>>>>> MAXPATHLEN + 1,
>>>>>   60 //       so buffers declared in VM are always >= 4096.
>>>>>   61 #define JVM_MAXPATHLEN MAXPATHLEN + 1
>>>>>
>>>>> 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.
>>>>
>>>> All of the unix platforms had MAXPATHLEN+1.  I'll leave it for now
>>>> and we can investigate that further.
>>>
>>> I see the following existing code:
>>>
>>> src/java.base/unix/native/include/jvm_md.h:
>>>
>>> #define JVM_MAXPATHLEN MAXPATHLEN
>>>
>>> src/java.base/macosx/native/include/jvm_md.h
>>>
>>> #define JVM_MAXPATHLEN MAXPATHLEN
>>>
>>> src/hotspot/os/aix/jvm_aix.h
>>>
>>> #define JVM_MAXPATHLEN MAXPATHLEN
>>>
>>> src/hotspot/os/bsd/jvm_bsd.h
>>>
>>> #define JVM_MAXPATHLEN MAXPATHLEN + 1  // blindly copied from Linux
>>> version
>>>
>>> src/hotspot/os/linux/jvm_linux.h
>>>
>>> #define JVM_MAXPATHLEN MAXPATHLEN + 1
>>>
>>> src/hotspot/os/solaris/jvm_solaris.h
>>>
>>> #define JVM_MAXPATHLEN MAXPATHLEN
>>>
>>> This is a linux only hack (if you ignore the blind copy from linux
>>> into the BSD code in the VM).
>>
>> Oh, thanks, so should I add a bunch of ifdefs then?  Or do you think
>> having MAXPATHLEN + 1 will really break the other platforms?  Do you
>> really see this as a problem or are you just pointing out inconsistency?
>>>
>>>>>
>>>>>  86 #define ASYNC_SIGNAL     SIGJVM2
>>>>>
>>>>> This only exists on Solaris so I think should be in #ifdef
>>>>> SOLARIS, to make that clear.
>>>>
>>>> Ok.  I'll add this.
>>>>>
>>>>> ---
>>>>>
>>>>> src/java.base/windows/native/include/jvm_md.h
>>>>>
>>>>> Given the differences between the two versions either something
>>>>> has been broken or "extern C" declarations are not needed :)
>>>>
>>>> Well, they are needed for Hotspot to build and do not prevent jdk
>>>> from building.  I don't know what was broken.
>>>
>>> We really need to understand this better. Maybe related to the map
>>> files that expose the symbols. ??
>>
>> They're needed because the JDK files are written mostly in C and that
>> doesn't complain about the linkage difference.  Hotspot files are in
>> C++ which does complain.
>>
>>>
>>>>>
>>>>> ---
>>>>>
>>>>> That was a really painful way to spend most of my Friday. TGIF! :)
>>>>
>>>> Thanks for going through it.  See comments inline for changes.
>>>> Generating a webrev takes hours so I'm not going to do that unless
>>>> you insist.
>>>
>>> An incremental webrev shouldn't take long - right? You're a mq
>>> maestro now. :)
>>
>> Well I generally trash a repository whenever I use mq but sure.
>>>
>>> If you can reasonably produce an incremental webrev once you've
>>> settled on all the comments/issues that would be good.
>>
>> Ok, sure.
>>
>> Coleen
>>>
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>
>>>>> On 27/10/2017 6:44 AM, [hidden email] 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, [hidden email] 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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>

12