Proposal: Use new JDK_EXPORT decorator instead of JNIEXPORT

[dean.long at oracle.com]

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