RFR (L, tedious again, sorry) 8189610: Reconcile jvm.h and all jvm_md.h between java.base and hotspot
David Holmes
david.holmes at oracle.com
Mon Oct 30 12:17:38 UTC 2017
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.
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