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
Mon Oct 30 14:48:32 UTC 2017
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, 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