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
