RFR (L, tedious again, sorry) 8189610: Reconcile jvm.h and all jvm_md.h between java.base and hotspot
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Tue Oct 31 12:53:17 UTC 2017
On 10/30/17 8:21 PM, David Holmes wrote:
> On 31/10/2017 12:48 AM, coleen.phillimore at oracle.com 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, coleen.phillimore at oracle.com wrote:
>>>
>>>
>>> On 10/30/17 8:17 AM, David Holmes wrote:
>>>> On 30/10/2017 10:13 PM, coleen.phillimore at oracle.com 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, coleen.phillimore at oracle.com wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 10/27/17 9:37 AM, David Holmes wrote:
>>>>>>>> On 27/10/2017 10:13 PM, coleen.phillimore at oracle.com 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, coleen.phillimore at oracle.com wrote:
>>>>>>>>>>> Hi Magnus,
>>>>>>>>>>>
>>>>>>>>>>> Thank you for reviewing this. I have a new version that
>>>>>>>>>>> takes out the hack in globalDefinitions.hpp and adds casts
>>>>>>>>>>> to src/hotspot/share/opto/type.cpp instead.
>>>>>>>>>>>
>>>>>>>>>>> Also some fixes from Martin at SAP.
>>>>>>>>>>>
>>>>>>>>>>> open webrev at
>>>>>>>>>>> http://cr.openjdk.java.net/~coleenp/8189610.02/webrev
>>>>>>>>>>>
>>>>>>>>>>> see below.
>>>>>>>>>>>
>>>>>>>>>>> On 10/26/17 5:57 AM, Magnus Ihse Bursie wrote:
>>>>>>>>>>>> Coleen,
>>>>>>>>>>>>
>>>>>>>>>>>> Thank you for addressing this!
>>>>>>>>>>>>
>>>>>>>>>>>> On 2017-10-25 18:49, coleen.phillimore at oracle.com wrote:
>>>>>>>>>>>>> Summary: removed hotspot version of jvm*h and jni*h files
>>>>>>>>>>>>>
>>>>>>>>>>>>> Mostly used sed to remove prims/jvm.h and move #include
>>>>>>>>>>>>> "jvm.h" after precompiled.h, so if you have repetitive
>>>>>>>>>>>>> stress wrist issues don't click on most of these files.
>>>>>>>>>>>>>
>>>>>>>>>>>>> There were more issues to resolve, however. The JDK
>>>>>>>>>>>>> windows jni_md.h file defined jint as long and the hotspot
>>>>>>>>>>>>> windows jni_x86.h as int. I had to choose the jdk version
>>>>>>>>>>>>> since it's the public version, so there are changes to the
>>>>>>>>>>>>> hotspot files for this. Generally I changed the code to
>>>>>>>>>>>>> use 'int' rather than 'jint' where the surrounding API
>>>>>>>>>>>>> didn't insist on consistently using java types. We should
>>>>>>>>>>>>> mostly be using C++ types within hotspot except in
>>>>>>>>>>>>> interfaces to native/JNI code. There are a couple of hacks
>>>>>>>>>>>>> in places where adding multiple jint casts was too painful.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Tested with JPRT and tier2-4 (in progress).
>>>>>>>>>>>>>
>>>>>>>>>>>>> open webrev at
>>>>>>>>>>>>> http://cr.openjdk.java.net/~coleenp/8189610.01/webrev
>>>>>>>>>>>>
>>>>>>>>>>>> Looks great!
>>>>>>>>>>>>
>>>>>>>>>>>> Just a few comments:
>>>>>>>>>>>>
>>>>>>>>>>>> * src/java.base/unix/native/include/jni_md.h:
>>>>>>>>>>>>
>>>>>>>>>>>> I don't think the externally_visible attribute should be
>>>>>>>>>>>> there for arm. I know this was the case for the
>>>>>>>>>>>> corresponding hotspot file for arm, but that was techically
>>>>>>>>>>>> incorrect. The proper dependency here is that
>>>>>>>>>>>> externally_visible should be in all JNIEXPORT if and only
>>>>>>>>>>>> if we're building with JVM feature "link-time-opt".
>>>>>>>>>>>> Traditionally, that feature been enabled when building
>>>>>>>>>>>> arm32 builds, and only then, so there's been a
>>>>>>>>>>>> (coincidentally) connection here. Nowadays, Oracle does not
>>>>>>>>>>>> care about the arm32 builds, and I'm not sure if anyone
>>>>>>>>>>>> else is building them with link-time-opt enabled.
>>>>>>>>>>>>
>>>>>>>>>>>> It does seem wrong to me to export this behavior in the
>>>>>>>>>>>> public jni_md.h file, though. I think the correct way to
>>>>>>>>>>>> solve this, if we should continue supporting link-time-opt
>>>>>>>>>>>> is to make sure this attribute is set for exported hotspot
>>>>>>>>>>>> functions. If it's still needed, that is. A quick googling
>>>>>>>>>>>> seems to indicate that visibility("default") might be
>>>>>>>>>>>> enough in modern gcc's.
>>>>>>>>>>>>
>>>>>>>>>>>> A third option is to remove the support for link-time-opt
>>>>>>>>>>>> entirely, if it's not really used.
>>>>>>>>>>>
>>>>>>>>>>> I didn't know how to change this since we are still building
>>>>>>>>>>> ARM with the jdk10/hs repository, and ARM needed this
>>>>>>>>>>> change. I could wait until we bring down the jdk10/master
>>>>>>>>>>> changes that remove the ARM build and remove this
>>>>>>>>>>> conditional before I push. Or we could file an RFE to remove
>>>>>>>>>>> link-time-opt (?) and remove it then?
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> * src/java.base/unix/native/include/jvm_md.h and
>>>>>>>>>>>> src/java.base/windows/native/include/jvm_md.h:
>>>>>>>>>>>>
>>>>>>>>>>>> These files define a public API, and contain non-trivial
>>>>>>>>>>>> changes. I suspect you should file a CSR request. (Even
>>>>>>>>>>>> though I realize you're only matching the header file with
>>>>>>>>>>>> the reality.)
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I filed the CSR. Waiting for the next steps.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Coleen
>>>>>>>>>>>
>>>>>>>>>>>> /Magnus
>>>>>>>>>>>>
>>>>>>>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8189610
>>>>>>>>>>>>>
>>>>>>>>>>>>> I have a script to update copyright files on commit.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks to Magnus and ErikJ for the makefile changes.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> Coleen
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>>
More information about the hotspot-dev
mailing list