If we are adding JDK_EXPORT, does it make sense to add JDK_CALL as well?
dl
On 12/16/18 4:57 PM, David Holmes wrote:
Hi Magnus,
Thanks for explaining how addition of JNIEXPORT may have started this
problem.
One follow up:
This will also need a CSR request due to the change in linking
behaviour.
I'm not sure what you mean by this..? My entire intention here is
that we should make no changes to documented interfaces; this is
strictly an internal change, so no CSR should be needed. Also, I
don't understand what you mean by "linking behavior"?
A CSR request is also required for behavioural changes - which this
is. Someone linking an "internal only" function today may get an error
if JNICALL is removed tomorrow. This may be acceptable but that is
what the CSR request establishes.
Thanks,
David
On 13/12/2018 8:37 pm, Magnus Ihse Bursie wrote:
On 2018-12-12 13:17, David Holmes wrote:
Okay I went away and did some homework ...
Let me back up a bit and see if I have the evolution of this correctly.
The native implementation of Java methods should be declared and
defined with JNIEXPORT and JNICALL.
JNIEXPORT controls the export visibility so they can be linked.
JNICALL controls the calling convention and is needed so that the
JVM's calling convention matches the native code. [This part was
unclear to me.]
Yes. And JNICALL is empty on all platforms except Windows 32, that's
why we're only seeing issues about mismatch there.
Other native methods exported from the same (or different) native
libraries may also be decorated with JNIEXPORT and JNICALL. But in
places there is a mismatch between the declaration in the header and
the definition leading to errors.
Yes; this mismatch has typically happened when the linking has not
been done by simply including the relevant header file, but either
duplicating the definition, or looking up the symbol dynamically.
Otherwise things should basically work out.
There are two main types of such native functions:
a) those publicly defined in the various native APIs: JNI itself
(jni.h), JVM TI (jvmti.h), AWT (jawt.h) ...
b) those intended only for use by other native code within the JDK
libraries (JLI_* - though I note Alan's comment re javafxpackager,
others??)
and a possible third type are callback functions like the JPLISAgent
event handlers (e.g. eventHandlerVMInit).
I'm not sure I understand what the third type is, but if it is a
publically defined API (which, at a first look, the JPLISAgent API
seems to be), then it's part of catagory a, otherwise it's part of
category b.
I must also state that my initial proposal didn't separate these two
cases. I had in mind only the b cases -- I have no intention of
changing public specifications, but I did not express this in my
proposal. That might have been one source of confusion. I apologize
for this omission.
There is a view that no native method that isn't the native-half of
a Java method should use JNICALL. [Okay I can see how that makes
sense - regardless of the actual calling convention used marking
such methods as "must use the same calling convention as the VM
native method call" is somewhat meaningless. Yet obviously the
public native APIs do specify JNICALL so this is not a hard and fast
rule. Further the callbacks must also follow a convention known to
the VM doing the calling!]
Yes, or rather the rule is "only native half Java methods should use
JNICALL, and also all public APIs where so is specified".
Where we have (introduced?) a discrepancy between the definition and
declaration the approach has been (because of the previous view) to
remove JNICALL. [This should only relate to methods of kind (b) above.]
Actually, it's not so much as we have *removed* JNICALL, as that we
have *introduced* JNIEXPORT. When I removed the map files, I also
removed the .def files and /export command lines for Windows. As a
result, I needed to add JNIEXPORT to a lot of places. Initially, I
followed the rule of adding JNICALL to those calls -- but I can
surely have missed a couple of places, since things will work fine
anyway, as long as caller and callee agree on the calling convention;
and a mismatch can only happen on Windows 32, which we do not build
and test. So there is a risk that even with the initial patch, not
all newly added JNIEXPORTs had JNICALL.
Then, it turned out, that a lot of this code did not compile with
Windows 32. With no deeper analysis of the flaw, the blame was put on
the absence or presence of JNICALL, and a series of patches were made
where JNICALL was more or less arbitrarily added or removed, until
things "worked". This should have been a warning sign, and I was
increasingly uneasy about all these changes, but I hadn't spent
enough time to realize what the core issue was or how to resolve it
properly.
That leaves those methods with JNIEXPORT only.
That seems wrong to you because they are not "JNI methods", so you
want to replace with JDK_EXPORT to make it clear. [Ok - this seems
reasonable.]
Yes. And given the fact that we have a bunch of "non-JNI methods"
that use the JNIEXPORT...JNICALL pattern, another way to interpret
the JDK_EXPORT semantics is that this function is exported for usage
*in the JDK*, but not for public consumption.
If JNICALL is removed from those native methods and they are
currently linked by external applications, those applications may
stop working. But this only affects win32 (as its the only platform
where JNICALL is different to the default C/C++ calling convention?)
so your position is this is acceptable breakage - and would only
affect unsupported/undocumented APIs.
Yes. In fact, it's possible that at least some of the breakage that
occurred was due to new *addition* of JNICALL, which was not present
before. We might have had something like (I'm making this specific
example up) a function "void ZIP_OpenFile(char* name)" with no
decoration at all, and a "/export:ZIP_OpenFile" on the command line,
and a ZIP_OpenFile entry in the unix map file. And I converted this
to "JNIEXPORT void JNICALL ZIP_OpenFile(char* name)", which de facto
-- although I didn't fully realize this at the time, changed the
calling convention and name decoration on Windows 32. When something
broke, perhaps because the user of ZIP_OpenFile did not include the
proper header file with the JNICALL modifier, the solution was to
remove the JNICALL part.
And of all the bug reports I've seen on this, the issues has been
internal linking only, i.e. problems building the JDK, not complaints
that external tools tried to use internal interfaces and failed. So
yes, my position is if this should break things, tough shit. That, of
courses, requires that we do not change public APIs, so we need to be
careful not to mess with those.
Does that sum it up?
Yep, I think so.
We still need to be sure that we're only changing functions of type
(b) above.
Yes, definitely.
And I note that this has been driven by the choice to remove JNICALL
where there was a discrepancy - that leads to the visibility of the
two kinds of methods. If it had been chosen to add JNICALL where
missing instead, then we may not have been having this conversation.
Actually, as I said, this has more been the result of a lot of added
JNICALL where previously there was none.
An alternative course of action is the make sure that *all* calls use
the JNIEXPORT...JNICALL pattern, even type b ones, and that we fix
all parts of code to actually accept the decorated names on Windows
32. This will lead to a lot more code changes, like the fix for
JDK-8214122 (JDWP is broken on 32 bit Windows: transport library
missing onLoad entry). And all this special case handling will be
needed only on Windows 32, which is a platform we do not want to
spend to much time or effort on. And finally, I think doing so would
make us miss out on an opportunity to make the semantics clearer.
This will also need a CSR request due to the change in linking
behaviour.
I'm not sure what you mean by this..? My entire intention here is
that we should make no changes to documented interfaces; this is
strictly an internal change, so no CSR should be needed. Also, I
don't understand what you mean by "linking behavior"?
/Magnus
Cheers,
David
-----
On 12/12/2018 9:03 pm, Magnus Ihse Bursie wrote:
On 2018-12-11 23:47, David Holmes wrote:
On 12/12/2018 12:34 am, Magnus Ihse Bursie wrote:
On 2018-12-11 00:23, David Holmes wrote:
Hi Magnus,
On 10/12/2018 11:19 pm, Magnus Ihse Bursie wrote:
I propose that we introduce a new define, available to all JDK
native files (Hotspot included), called JDK_EXPORT. The
behavior of this symbol will be very similar (as of now, in
fact identical) to JNIEXPORT; however, the semantics will not.
Currently, we "mis-use" the JNIEXPORT define to mark a function
for exporting from the library. The problem with this is that
JNIEXPORT is part of the JNI interface, and is supposed to be
used when C programs interact with Java. And, when doing this,
the function should be fully decorated like this: "JNIEXPORT
foo JNICALL".
I've seen a lot of the emails on this issue and I don't fully
understand what has been going wrong. But the intent is
obviously the JNIEXPORT represents what is needed to export this
function for use by JNI, while JNICALL defines the calling
convention. I agree there may be some mistmatch when functions
are actually not intended for general export outside the JDK but
are only for internal JDK use.
We do have many such JNI exports in our native libraries, but
we also have a lot of other, non-JNI exports, where one native
library just provides an interface to other libraries. In these
cases, we have still used JNIEXPORT for the functionality of
getting the function exported, but we have not been consistent
in our use of JNICALL. This has caused us way too much trouble
for something that should Just Work<tm>.
Are you suggesting that the interface between different
libraries in the JDK should not be a JNI interface? Is this
because you think the functions in these libraries are only for
JDK internal use or ... ??
I therefore propose that we define "JDK_EXPORT", with the same
behavior as JNIEXPORT (that is, flagging the function for
external visibility in the resulting native library), but which
is *not* supposed to be exported to Java code using JNI, nor
supposed to be decorated with
Just a clarification there. JNI functions are not exported to
Java code, they are exported to native code. Java code can
declare native methods and those native methods must be written
as JNI functions, but that's not what we are discussing.
Libraries expose a JNI interface (a set of functions in the
library) that can be called by application native code, using JNI.
We're apparently looking at "JNI" and "exporting" from two
opposite sides here. :-) Just to make everything clear: If I have
a Java class
class MyClass {
public static void native myNativeFunc();
}
then I have one half of the JNI function, the Java half. This
must be matched by a corresponding implementation in native, like
this:
JNIEXPORT void JNICALL
Java_MyClass_myNativeFunc(void) {
// ... do stuff
}
And this is the native half of the JNI function. Right? Let's
leave aside which side is "exporting" to the other for now. :-)
This way of setting up native functions that can be called from
Java is what I refer to as JNI. And when you declare a native JNI
function, you *must* use both JNIEXPORT and JNICALL. Alright?
We do have a lot of those functions in our native libraries. And
they are correct just the way they are.
Yep all well and good. A function declared native in Java must
have an implementation as you describe. But not all native
functions exist to provide the native-half of a Java native function!
However, we also have *other* native functions, that are
exported, not as JNI functions that should be called from Java,
but as normal native library functions that should be called by
other native code. Okay so far? And *those* functions have been
problematic in how we decorate
But there are again two cases. Those functions exported from a
library that are expected to be called from external code using
the JNI interface mechanism - such as all the JNI functions and
JVM TI functions we export from the JVM - and those "exported" for
access between libraries within the JDK (such as all the JVM_*
functions in libjvm).
I think it is only the second group that should be addressed by
your JDK_EXPORT proposal - though I'm not completely clear exactly
how to identify them.
Alright, now I think we're on the same page again. :)
Yes, this is what I'm saying. I'm not proposing to modify public APIs.
You identify external APIs by the fact that they are documented.
And if you are unsure, you ask Jon. ;-)
them. My proposal is that we *refrain* from using JNIEXPORT for
those functions, and instead use JDK_EXPORT as name for the macro
that decorates them as exported. That way, we can clearly see
that a function like this:
JDK_EXPORT void
JLI_ReadEnv(char* env);
is correctly declared, and will be exported to other native
libraries, but not to Java.
The issue is not whether it is "exported to Java"** but whether it
is exported using the JNI mechanism such that other native code
calls it using the JNI mechanism.
** There is no way to write a native method declaration in Java
such that it would be linked to the JLI_ReadEnv function. The
naming is all wrong, as is the signature.
Just to clarify, this has nothing to do with if this is a
officially supported API or not. In general though, I assume that
most (if not all?) of our exported functions (apart from the
JNI_* stuff) is supposed to be consumed by other libraries in the
JDK, and is not a public API.
I think it varies library by library. You may need native
application code that can call directly into native JDK libraries.
JLI is the Java Launcher Interface - I think it was introduced to
make it easier for other launchers to be created. Native agents
may need access to libmanagement or libjdwp functions. Native
graphics code may need access to the JDK graphics library. Some of
these accesses may be unsupported and undocumented, but I don't
think you can just cut them all off.
If they are unsupported and undocumented, I sure as h*ll can cut
them all off! :-)
Besides, and let me re-iterate this, the only change that will
happen by removing JNICALL, is on Windows 32. No other platform
will be affected. There is no official support for Windows 32
anymore. There's some low-effort community work done on keeping
Windows 32 alive, but it's not backed by any major player. Right
now, they are taking a lot of hits *due to the fact* that we have
not sorted this out, and waste a lot of their precious effort
trying to fix this piecemeal. If we do fix things according to this
proposal, then it's at least clear how things *should* work. If it
turns out that there's some code out there in the wild, running on
Windows 32, that uses an undocumented and unsupported function
call, and it breaks -- well, sucks to be them. They'll just have to
do what everyone does who uses an undocumented interface that
suddenly changes: update their code.
/Magnus
David
/Magnus
JNICALL. All current instances of JNIEXPORT which is not pure
JNI native functions should be changed to use JDK_EXPORT instead.
I further propose that this macro should reside in a new file
"jdk.h", placed in the new directory
src/java.base/share/native/include/internal. This header file
path will automatically be provided to all native libraries,
but not copied to the JDK being built. (The existence of a
"include/internal" directory with this behavior has been
discussed before. There are more files that ought to be moved
there, if/when it is created.) I believe in many cases the
#include "jni.h" can be just modified to #include "#jdk.h",
since most native code will not require "jni.h" unless actually
doing JNI calls -- most have included this file to get the
JNIEXPORT macro, which would explain the pervasive use of
#include "jni.h" in our code base.
jni.h also defines all of the types used by the JNI. Those types
are pervsive to the native code used throughout the JDK.
Thoughts?
I think we need to understand the problems on Windows that
prompted all this. Then I think we need to look at exactly how
jni.h and JNIEXPORT etc are being used and understand whether
this is truly an exported interface or not.
Cheers,
David
/Magnus