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

Fri Jun 8 15:37:10 UTC 2018

Looks good.

/Erik

On 2018-06-08 01:50, Magnus Ihse Bursie wrote:
> 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
>>>
>>
>