RFR (L, tedious again, sorry) 8189610: Reconcile jvm.h and all jvm_md.h between java.base and hotspot

[coleen.phillimore at oracle.com]

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
>>>>>>
>>>>>
>>>>
>>