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 07:23:34 UTC 2017
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.
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?
> 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.
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.
---
src/hotspot/cpu/arm/interp_masm_arm.cpp
Why did you need to add the jvm.h include?
---
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()) {
---
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?
---
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.
---
src/hotspot/share/ci/ciReplay.cpp
793 jint* dims = NEW_RESOURCE_ARRAY(jint, rank);
why should this be jint?
---
src/hotspot/share/classfile/altHashing.cpp
Okay this looks more consistent with jint.
---
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)
---
src/hotspot/share/code/relocInfo.cpp
Change seems unnecessary - int32_t is fine
---
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 ??
---
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;
??
---
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.
---
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.
---
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!
---
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.
---
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.
---
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?
---
src/hotspot/share/prims/jvm.cpp
is_attachable is the terminology used in the JDK code.
---
src/hotspot/share/prims/jvmtiEnvBase.cpp
src/hotspot/share/prims/jvmtiImpl.cpp
Are you making parameters consistent with the fields they initialize?
---
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,
---
src/hotspot/share/runtime/perfData.cpp
Callers pass both jint and int, so param type seems arbitrary.
---
src/hotspot/share/runtime/perfMemory.cpp
src/hotspot/share/runtime/perfMemory.hpp
PerfMemory::_initialized should ideally be a bool - can OrderAccess
handle that now?
---
src/java.base/share/native/include/jvm.h
Not clear why the jio functions are not also JNICALL ?
---
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.
---
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.
86 #define ASYNC_SIGNAL SIGJVM2
This only exists on Solaris so I think should be in #ifdef SOLARIS, to
make that clear.
---
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 :)
---
That was a really painful way to spend most of my Friday. TGIF! :)
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