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

[Erik Joelsson]

Hello Magnus,

Very nice refactoring!

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.

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
>