RFR: JDK-8199608 Clean up LDFLAGS for libfontmanager

Thu Apr 5 09:31:54 UTC 2018

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