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
Fri Oct 27 13:37:42 UTC 2017
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'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; }
>>
>> ---
>>
>> 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?
>>
>> ---
>>
>> 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".
>>
>> ---
>>
>> 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).
>>
>> 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. ??
>>
>> ---
>>
>> 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. :)
If you can reasonably produce an incremental webrev once you've settled
on all the comments/issues that would be good.
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