Proposal: Use new JDK_EXPORT decorator instead of JNIEXPORT

[David Holmes]

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

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.

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

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

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

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

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.

Does that sum it up?

We still need to be sure that we're only changing functions of type (b) 
above.

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.

This will also need a CSR request due to the change in linking behaviour.

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