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 12:38:23 UTC 2017
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 build-dev
mailing list