RFR: JDK-8199608 Clean up LDFLAGS for libfontmanager

[Magnus Ihse Bursie]

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.

/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
>>>>
>>>
>>
>