<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 2018-06-07 23:20, Erik Joelsson
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:eea6cb0b-dc41-7fd3-eb32-45740982b847@oracle.com">Hello
Magnus,
<br>
<br>
Very nice refactoring!
<br>
</blockquote>
Thanks!<br>
<br>
<blockquote type="cite"
cite="mid:eea6cb0b-dc41-7fd3-eb32-45740982b847@oracle.com">
<br>
JdkNativeCompilation.gmk
<br>
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.
<br>
</blockquote>
<br>
Yeah, I agree, addprefix is better. I just forgot about it; foreach
is a nice allround tool.<br>
<br>
Updated webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~ihse/JDK-8204572-autodetect-SRC-and-headers-dirs/webrev.02/">http://cr.openjdk.java.net/~ihse/JDK-8204572-autodetect-SRC-and-headers-dirs/webrev.02/</a><br>
(Only changes is in JdkNativeCompilation.gmk, as per above
comments).<br>
<br>
/Magnus<br>
<br>
<blockquote type="cite"
cite="mid:eea6cb0b-dc41-7fd3-eb32-45740982b847@oracle.com">
<br>
Otherwise looks good.
<br>
<br>
/Erik
<br>
<br>
<br>
On 2018-06-07 13:22, Magnus Ihse Bursie wrote:
<br>
<blockquote type="cite">This change needs some background
information.
<br>
<br>
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.
<br>
<br>
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.
<br>
<br>
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.
<br>
<br>
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.
<br>
<br>
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.)
<br>
<br>
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.
<br>
<br>
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:
<br>
<br>
* Windows-x64:
<br>
<br>
No differences at all.
<br>
<br>
* Solaris:
<br>
<br>
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...)
<br>
<br>
* MacOS:
<br>
<br>
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.
<br>
<br>
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.
<br>
<br>
For libmlib_image.dylib, the link order had changed causing the
noted binary difference.
<br>
<br>
* Linux:
<br>
<br>
On linux, the compare script noted differences for libextnet,
libjava, libmlib_image, libprefs, libsaproc, libsplashscreen and
libsunec.
<br>
<br>
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.
<br>
<br>
libsaproc and libjava changes was caused by closed source
changes, and is therefore not relevant to OpenJDK.
<br>
<br>
For libmlib_image.dylib, the link order had changed causing the
noted binary difference, as on MacOS.
<br>
<br>
Bug: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8204572">https://bugs.openjdk.java.net/browse/JDK-8204572</a>
<br>
WebRev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~ihse/JDK-8204572-autodetect-SRC-and-headers-dirs/webrev.01">http://cr.openjdk.java.net/~ihse/JDK-8204572-autodetect-SRC-and-headers-dirs/webrev.01</a><br>
<br>
/Magnus
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>