Proposal: Use new JDK_EXPORT decorator instead of JNIEXPORT

[David Holmes]

On 17/12/2018 12:51 pm, dean.long at oracle.com wrote:
> If we are adding JDK_EXPORT, does it make sense to add JDK_CALL as well?

It should never be necessary. JNICALL is only needed to ensure caller 
and callee use the same calling convention. For JDK_EXPORT the caller 
and callee are part of the same thing - the JDK - and will always use 
the same (platform default) calling convention.

David

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