On 10/30/17 8:17 AM, David Holmes wrote:
On 30/10/2017 10:13 PM, coleen.phillim...@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.
I don't want to wait for time so I'm investigating.
It's one use is:
Java_java_io_UnixFileSystem_canonicalize0(JNIEnv *env, jobject this,
...
char canonicalPath[JVM_MAXPATHLEN];
if (canonicalize((char *)path,
canonicalPath, JVM_MAXPATHLEN) < 0) {
JNU_ThrowIOExceptionWithLastError(env, "Bad pathname");
Which goes to:
canonicalize_md.c
canonicalize(char *original, char *resolved, int len)
if (len < PATH_MAX) {
errno = EINVAL;
return -1;
}
So this should fail every time.
sys/param.h:# define MAXPATHLEN PATH_MAX
I haven't found any tests for it.
I don't know why Java_java_io_UnixFileSystem uses JVM_MAXPATHLEN since
it's not calling the JVM interface as far as I can tell. I think it
should be changed to PATH_MAX.
?
Coleen
David
thanks,
Coleen
Thanks,
David
On 28/10/2017 12:08 AM, coleen.phillim...@oracle.com wrote:
On 10/27/17 9:37 AM, David Holmes wrote:
On 27/10/2017 10:13 PM, coleen.phillim...@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.phillim...@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.phillim...@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
