Proposal: Use new JDK_EXPORT decorator instead of JNIEXPORT
dean.long at oracle.com
dean.long at oracle.com
Mon Dec 17 02:51:48 UTC 2018
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
>>>>>>>>
>>>>>>
>>>>
>>
More information about the core-libs-dev
mailing list