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
Fri Oct 27 12:13:12 UTC 2017
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.
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.
>
>> 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.
>
> 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.
>
> ---
>
> 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);
> ---
>
> 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.
>
> ---
>
> 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!
>
> ---
>
> 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.
>
> ---
>
> 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.
>
> ---
>
> 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.
>
> ---
>
> 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.
> ??
>
> ---
>
> 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!
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.
>
> ---
>
> 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.
>
> ---
>
> 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.
>
> ---
>
> 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.
>
> 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.
>
> ---
>
> 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.
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