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

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

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

Incremental webrev:

http://cr.openjdk.java.net/~coleenp/8189610.incr.01/webrev/index.html

thanks,
Coleen

On 10/27/17 11:13 AM, [hidden email] wrote:

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

David Holmes
On 28/10/2017 6:20 AM, [hidden email] wrote:
>
> Incremental webrev:
>
> http://cr.openjdk.java.net/~coleenp/8189610.incr.01/webrev/index.html

That all looks fine - thanks.

If I get a chance I'll look deeper into why the VS compiler needs 0 to
be cast to jint (aka long) to avoid ambiguity with it being a NULL
pointer. I could understand if it always needed the cast, but not only
needing it for long, but not int.

Thanks,
David

> thanks,
> Coleen
>
> On 10/27/17 11:13 AM, [hidden email] wrote:
>>
>>
>> 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

coleen.phillimore


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

> On 28/10/2017 6:20 AM, [hidden email] wrote:
>>
>> Incremental webrev:
>>
>> http://cr.openjdk.java.net/~coleenp/8189610.incr.01/webrev/index.html
>
> That all looks fine - thanks.
>
> If I get a chance I'll look deeper into why the VS compiler needs 0 to
> be cast to jint (aka long) to avoid ambiguity with it being a NULL
> pointer. I could understand if it always needed the cast, but not only
> needing it for long, but not int.

Thanks,  Kim can probably tell you where in the spec this is.

Coleen

>
> Thanks,
> David
>
>> thanks,
>> Coleen
>>
>> On 10/27/17 11:13 AM, [hidden email] wrote:
>>>
>>>
>>> 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

coleen.phillimore
In reply to this post by coleen.phillimore

http://cr.openjdk.java.net/~coleenp/8189610.incr.02/webrev/index.html

Changed JDK file to use PATH_MAX.  Retested jdk tier1 tests.

thanks,
Coleen

On 10/30/17 8:38 AM, [hidden email] wrote:

>
>
> On 10/30/17 8:17 AM, David Holmes wrote:
>> On 30/10/2017 10:13 PM, [hidden email] wrote:
>>> 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?
>>
>> Yes - thanks. It preserves existing behaviour on the VM side at
>> least. Time will tell if it messes anything up on the JDK side for
>> Linux/OSX.
>
> I don't want to wait for time so I'm investigating.
>
> It's one use is:
>
> Java_java_io_UnixFileSystem_canonicalize0(JNIEnv *env, jobject this,
> ...
>         char canonicalPath[JVM_MAXPATHLEN];
>         if (canonicalize((char *)path,
>                          canonicalPath, JVM_MAXPATHLEN) < 0) {
>             JNU_ThrowIOExceptionWithLastError(env, "Bad pathname");
>
> Which goes to:
>
> canonicalize_md.c
>
> canonicalize(char *original, char *resolved, int len)
>     if (len < PATH_MAX) {
>         errno = EINVAL;
>         return -1;
>     }
>
>
> So this should fail every time.
>
> sys/param.h:# define MAXPATHLEN    PATH_MAX
>
> I haven't found any tests for it.
>
> I don't know why Java_java_io_UnixFileSystem uses JVM_MAXPATHLEN since
> it's not calling the JVM interface as far as I can tell. I think it
> should be changed to PATH_MAX.
>
> ?
> Coleen
>>
>> David
>>
>>> 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
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>

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
On 31/10/2017 12:48 AM, [hidden email] wrote:
>
> http://cr.openjdk.java.net/~coleenp/8189610.incr.02/webrev/index.html
>
> Changed JDK file to use PATH_MAX.  Retested jdk tier1 tests.

Why PATH_MAX instead of MAXPATHLEN? They appear to be the same on Linux
and Solaris, but I don't know if that is true for AIX and Mac OS / BSD.

Does UnixFileSystem_md.c still need the jvm.h include now?

Thanks,
David

> thanks,
> Coleen
>
> On 10/30/17 8:38 AM, [hidden email] wrote:
>>
>>
>> On 10/30/17 8:17 AM, David Holmes wrote:
>>> On 30/10/2017 10:13 PM, [hidden email] wrote:
>>>> 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?
>>>
>>> Yes - thanks. It preserves existing behaviour on the VM side at
>>> least. Time will tell if it messes anything up on the JDK side for
>>> Linux/OSX.
>>
>> I don't want to wait for time so I'm investigating.
>>
>> It's one use is:
>>
>> Java_java_io_UnixFileSystem_canonicalize0(JNIEnv *env, jobject this,
>> ...
>>         char canonicalPath[JVM_MAXPATHLEN];
>>         if (canonicalize((char *)path,
>>                          canonicalPath, JVM_MAXPATHLEN) < 0) {
>>             JNU_ThrowIOExceptionWithLastError(env, "Bad pathname");
>>
>> Which goes to:
>>
>> canonicalize_md.c
>>
>> canonicalize(char *original, char *resolved, int len)
>>     if (len < PATH_MAX) {
>>         errno = EINVAL;
>>         return -1;
>>     }
>>
>>
>> So this should fail every time.
>>
>> sys/param.h:# define MAXPATHLEN    PATH_MAX
>>
>> I haven't found any tests for it.
>>
>> I don't know why Java_java_io_UnixFileSystem uses JVM_MAXPATHLEN since
>> it's not calling the JVM interface as far as I can tell. I think it
>> should be changed to PATH_MAX.
>>
>> ?
>> Coleen
>>>
>>> David
>>>
>>>> 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
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
>
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 30/10/2017 10:15 PM, [hidden email] wrote:

> On 10/28/17 3:58 AM, David Holmes wrote:
>> On 28/10/2017 6:20 AM, [hidden email] wrote:
>>>
>>> Incremental webrev:
>>>
>>> http://cr.openjdk.java.net/~coleenp/8189610.incr.01/webrev/index.html
>>
>> That all looks fine - thanks.
>>
>> If I get a chance I'll look deeper into why the VS compiler needs 0 to
>> be cast to jint (aka long) to avoid ambiguity with it being a NULL
>> pointer. I could understand if it always needed the cast, but not only
>> needing it for long, but not int.
>
> Thanks,  Kim can probably tell you where in the spec this is.

Now I get it.

Given:

void x(int i) { ...}
void x(Foo* p) { ... }

a call x(0) is a call to x(int) because 0 is an int. No conversion needed.

But given:

void x(long i) { ...}
void x(Foo* p) { ... }

a call x(0) has no direct match (no int version) so standard conversions
apply and IIUC conversion to long and conversion to Foo* have the same
rank, so neither is preferred and the call is ambiguous.

David

> Coleen
>
>>
>> Thanks,
>> David
>>
>>> thanks,
>>> Coleen
>>>
>>> On 10/27/17 11:13 AM, [hidden email] wrote:
>>>>
>>>>
>>>> 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

coleen.phillimore
In reply to this post by David Holmes


On 10/30/17 8:21 PM, David Holmes wrote:
> On 31/10/2017 12:48 AM, [hidden email] wrote:
>>
>> http://cr.openjdk.java.net/~coleenp/8189610.incr.02/webrev/index.html
>>
>> Changed JDK file to use PATH_MAX.  Retested jdk tier1 tests.
>
> Why PATH_MAX instead of MAXPATHLEN? They appear to be the same on
> Linux and Solaris, but I don't know if that is true for AIX and Mac OS
> / BSD.

I picked PATH_MAX because canonicalize_md.c uses that constant.
>
> Does UnixFileSystem_md.c still need the jvm.h include now?

No, I will remove it.

Thanks,
Coleen

>
> Thanks,
> David
>
>> thanks,
>> Coleen
>>
>> On 10/30/17 8:38 AM, [hidden email] wrote:
>>>
>>>
>>> On 10/30/17 8:17 AM, David Holmes wrote:
>>>> On 30/10/2017 10:13 PM, [hidden email] wrote:
>>>>> 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?
>>>>
>>>> Yes - thanks. It preserves existing behaviour on the VM side at
>>>> least. Time will tell if it messes anything up on the JDK side for
>>>> Linux/OSX.
>>>
>>> I don't want to wait for time so I'm investigating.
>>>
>>> It's one use is:
>>>
>>> Java_java_io_UnixFileSystem_canonicalize0(JNIEnv *env, jobject this,
>>> ...
>>>         char canonicalPath[JVM_MAXPATHLEN];
>>>         if (canonicalize((char *)path,
>>>                          canonicalPath, JVM_MAXPATHLEN) < 0) {
>>>             JNU_ThrowIOExceptionWithLastError(env, "Bad pathname");
>>>
>>> Which goes to:
>>>
>>> canonicalize_md.c
>>>
>>> canonicalize(char *original, char *resolved, int len)
>>>     if (len < PATH_MAX) {
>>>         errno = EINVAL;
>>>         return -1;
>>>     }
>>>
>>>
>>> So this should fail every time.
>>>
>>> sys/param.h:# define MAXPATHLEN    PATH_MAX
>>>
>>> I haven't found any tests for it.
>>>
>>> I don't know why Java_java_io_UnixFileSystem uses JVM_MAXPATHLEN
>>> since it's not calling the JVM interface as far as I can tell. I
>>> think it should be changed to PATH_MAX.
>>>
>>> ?
>>> Coleen
>>>>
>>>> David
>>>>
>>>>> 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
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>>