RFR (XL): JDK-8204572 SetupJdkLibrary should setup SRC and -I flags automatically

Fri Jun 8 08:50:37 UTC 2018

On 2018-06-07 23:20, Erik Joelsson wrote:
> Hello Magnus,
>
> Very nice refactoring!
Thanks!

>
> JdkNativeCompilation.gmk
> line 126-127 looks a bit long. There is an extra space on 126. Also, 
> why not addprefix for adding -I instead of clunky foreach? Not that I 
> care greatly, but I usually prefer that construct.

Yeah, I agree, addprefix is better. I just forgot about it; foreach is a 
nice allround tool.

Updated webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8204572-autodetect-SRC-and-headers-dirs/webrev.02/
(Only changes is in JdkNativeCompilation.gmk, as per above comments).

/Magnus

>
> Otherwise looks good.
>
> /Erik
>
>
> On 2018-06-07 13:22, Magnus Ihse Bursie wrote:
>> This change needs some background information.
>>
>> I've been working on simplifying and streamlining the compilation of 
>> native libraries of the JDK. Previously, I introduced the 
>> SetupJdkLibrary function, and started to get a better control of 
>> compiler flags. This patch continues on both paths.
>>
>> The original intent of this change, which I naively thought was going 
>> to be much simpler than it turned out :-) was to separate the -I 
>> flags (location of header files) from other flags, and to generate 
>> these automatically, wherever possible. Since we have a standard way 
>> of locating native code, and most libraries just want to have an -I 
>> path to their own source base and the generated Java header, we 
>> should be able to provide this in SetupJdkLibrary.
>>
>> This turned out to be closely related to SetupJdkLibrary being able 
>> to discover the location of the SRC directories itself, using 
>> "convention over configuration" and assuming that the library 
>> "libfoo" for "java.module" would be located in 
>> java.module/*/native/libfoo.
>>
>> While this sounds simple in theory, when actually trying to implement 
>> this I of course ran into all the places where some special handling 
>> was indeed needed. So even if like 90% of all libraries were simple 
>> to get to build using automated discovery of source and header 
>> directories, the 10% that did not caused me much more headaches than 
>> I had anticipated. On the other hand, now that I've sorted out all 
>> those places, the few remaining odd solutions is clearly documented 
>> and not just something that "just happens" due to strange 
>> configurations.
>>
>> One file deserves mentioning specifically: Awt2dLibraries.gmk. The 
>> java.desktop libraries are unfortunately quite entangled with each 
>> other, and do not readily follow the patterns that are used elsewhere 
>> in the code base. So it might just look like the file has just gone 
>> from one state of messiness, to another, which would hardly be an 
>> improvement. :-( I would still argue that the new messiness is 
>> better: It is now much clearer in what ways the libraries diverge 
>> from our standard assumption, and what course of action needs to be 
>> taken to minimize these differences. (Which is something I believe 
>> should be done -- these issues are not just cosmetic but is the root 
>> of most of the issues we always see for these libraries, when 
>> upgrading compilers, etc.)
>>
>> During this change, I noticed that not all native libraries include 
>> the proper generated header file. This is a dangerous coding 
>> practice, since a change in the Java part of the interface might not 
>> get picked up properly in the native part. I've added the missing 
>> includes that I've detected, and due to these changes, I'm also 
>> including the component teams in what is really only a build change. 
>> As can be seen for jdk.crypto.mscapi; there had indeed been changes 
>> that needed correcting.
>>
>> Since this is (basically) a pure build change, my gold standard here 
>> has been the build compare script. In essence, the build output prior 
>> to my change and with this change are 100% identical. In truth, this 
>> is a bit too strong a claim. A few changes has occurred, but none of 
>> them should matter. Here's a breakdown of the compare.sh results:
>>
>> * Windows-x64:
>>
>> No differences at all.
>>
>> * Solaris:
>>
>> Two libraries are reported to differ: libsaproc.so and 
>> libfontmanager.so, both with a disass diff on ~700 bytes. Analyzing 
>> this, I found that the object files used to link these two libraries 
>> has no disass differences. They have a slight binary difference and a 
>> difference in size, due to the include paths being different (and 
>> this is stored in the .o file, which makes it different). Somehow 
>> this apparently triggers the linker to generate a slightly different 
>> code in a few places, using a different register or so. (Weird...)
>>
>> * MacOS:
>>
>> Two libraries are reported to differ: libjava.dylib and 
>> libmlib_image.dylib, both of them just reported as a binary diff (no 
>> symbol, disass or fulldump differences). This is not really 
>> unsuspected, but I analyzed it anyway.
>>
>> I found that for libjava.dylib, a single .o file was different. This 
>> one was actually picked up from closed sources, and are not really 
>> relevant for OpenJDK. Anyway, the reason for the difference was the 
>> same as for the Solaris libs; the include paths had changes, which 
>> caused a binary diff.
>>
>> For libmlib_image.dylib, the link order had changed causing the noted 
>> binary difference.
>>
>> * Linux:
>>
>> On linux, the compare script noted differences for libextnet, 
>> libjava, libmlib_image, libprefs, libsaproc, libsplashscreen and 
>> libsunec.
>>
>> The differences for libextnet, libprefs, libsplashscreen and libsunec 
>> turned out to be caused by the added #include of the generated Java 
>> headers. This caused binary differences (reasonably), and for some 
>> odd reason also a symbol difference in java_awt_SplashScreen.o 
>> (clazz.10057 and mid.10058 were replaced by clazz.10015 and 
>> mid.10016). I can't claim to understand this, but I'm assuming it's 
>> some kind of generated code.
>>
>> libsaproc and libjava changes was caused by closed source changes, 
>> and is therefore not relevant to OpenJDK.
>>
>> For libmlib_image.dylib, the link order had changed causing the noted 
>> binary difference, as on MacOS.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8204572
>> WebRev: 
>> http://cr.openjdk.java.net/~ihse/JDK-8204572-autodetect-SRC-and-headers-dirs/webrev.01
>>
>> /Magnus
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20180608/32ad80e6/attachment.htm>