RFR: JDK-8199608 Clean up LDFLAGS for libfontmanager

Thu Apr 5 13:48:58 UTC 2018

Looks good.

/Erik

On 2018-04-05 02:31, Magnus Ihse Bursie wrote:
> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>