On 2018-03-14 22:17, Magnus Ihse Bursie wrote:


On 2018-03-14 22:05, Phil Race wrote:

>I see we used to link with libawt_headless for solaris, but removed it in >JDK-8194870. Phil remarked in that bug: "We linked against headless before then >but allowed undefined symbols JDK 9 switched this to libawt_headless when >headless apps failed." but I'm not sure I understand what this really means.

It means that there is a long and sordid history.
When we introduced this code it was as it is now.
Some time in JDK 8 (?) it started to link against one of these libs.
I'd have to go back and check which one, but definitely in JDK 9 a bug
popped up with headless because of explicit linking to xawt .. which
pulls in X11. At that time the fix was to switch it to link against headless but that was the wrong fix now as it is then. Somehow it worked OK though
as had the xawt linking so long as you had X11 .. until JDK-8194870 and
we realised the error. Now we are back to how it was intended in JDK 1.5 (I think).
Thank you for the explanation!

I will think about this fix for a while, and see what I can do about it. For now, consider the review request cancelled.

I still need to think about how to handle the solaris case, but at least I can salvage the macosx cleanup from this patch.

I have verified functionality using "java -jar SwingSet2.jar" on a macosx machine.

Updated webrev:
http://cr.openjdk.java.net/~ihse/JDK-8199608-clean-up-LDFLAGS-for-libfontmanager/webrev.02

/Magnus



/Magnus


> Question to Phil: Would it be possible to rewrite the code to load and reference the symbols dynamically

Whilst it probably is possible, I really don't want to do that it is not
really in the spirit of what is intended here as well as being a fair amount of work. This is not an "optional" library. It has to be there. We just don't know which one
until runtime.
What I'd really like here is a build linker option that at build time verifies that the dependencies are actually satisfied by each of xawt and headless in turn but in neither case writes that library into the produced image as a dependency.

For the macos changes, I am supposing you mean you had to add this
+ LIBS_macosx := -lawt_lwawt -framework CoreText -framework CoreFoundation \

because you removed this

- LDFLAGS_macosx := -undefined dynamic_lookup, \

If it builds *and runs* its probably fine :-)

Only Solaris + Linux have separate libraries for headful + headless.

-phil.

On 03/14/2018 01:22 PM, Magnus Ihse Bursie wrote:
On 2018-03-14 15:59, Erik Joelsson wrote:
This change is unfortunately not correct for Linux and Solaris. We cannot link libfontmanager explicitly against either libawt_xawt or libawt_headless. See https://bugs.openjdk.java.net/browse/JDK-8196516 for my suggestion on a better fix than we currently have. I was hoping for Severin to check it out and pick it up, but he is away for a bit so that hasn't happened.

The reason we cannot link explicitly is that we need to decide at runtime whether to pull in the headful or headless libraries. If one or the other is already pulled in, and we explicitly load the other, the runtime linker will lookup the common symbols in one or the other in an unpredictable way. Some users get the correct behavior, some get the wrong behavior. We recently had discussions around this on this list if you want to dive deeper into it.

Also see https://bugs.openjdk.java.net/browse/JDK-8194870 for one of the consequences of explicit linking here.

I think the mac part is ok though, but Phil has to have a look. For Linux and Solaris, if you could remove the -lawt_headless and add "-Xlinker --unresolved-symbols=ignore-all" to LDFLAGS for linux I think we should be good.
Oh, this seems like a fine mess. :-(

I see we used to link with libawt_headless for solaris, but removed it in JDK-8194870. Phil remarked in that bug: "We linked against headless before then but allowed undefined symbols JDK 9 switched this to libawt_headless when headless apps failed." but I'm not sure I understand what this really means.

I agree it seems likely that the macosx changes is correct. I could probably add -Wl,--unresolved-symbols=ignore-all for gcc on linux, but that is unlikely to work on solstudio. :-( So I'm afraid we're still stuck with the need to remove -z defs for some builds. :-(

Question to Phil: Would it be possible to rewrite the code to load and reference the symbols dynamically, using libdl? The problem here is that the functions are declared extern, instead of loaded into a function pointer at runtime. That's how you typically use this kind of runtime-determined dynamic loading.

/Magnus

/Erik

On 2018-03-14 04:45, Magnus Ihse Bursie wrote:
This is the final LDFLAGS cleanup, which required some more work to resolve.

Libfontmanager had previously explicitely disabled -z defs, with the result that linking did not complain about missing libraries. To fix this, I had to provide the proper libraries to link with. For linux and solaris, this was libawt_headless. For macosx, this was libawt_lwawt, but also three system frameworks.

Note that this patch has a merge conflict with JDK-8199606. The end result of both patches are shown in the patch (that is, with -lc removed). I will make sure to resolve the conflicts properly when committing this patch.

I have run COMPARE_BUILDS, with expected results. That is, no changes for Windows, and a deps change for macosx, linux and solaris. I also got a symbol change for linux, since the symbols from libawt_headless changed from e.g. "AWTCharAdvance" to "AWTCharAdvance@@SUNWprivate_1.1". And of course, when you do linking changes, you also end up getting binary/disasm changes.

Bug: https://bugs.openjdk.java.net/browse/JDK-8199608
WebRev: http://cr.openjdk.java.net/~ihse/JDK-8199608-clean-up-LDFLAGS-for-libfontmanager/webrev.01






Reply via email to