Re: [PATCH 2/3] libctf: Add comment for conditionally def'd sym

[Nick Alcock <nick.alcock@oracle.com>]

On 5 Mar 2024, Nicholas Vinson spake thusly:

> On 3/4/24 08:57, Nick Alcock wrote:
>> On 3 Mar 2024, Nicholas Vinson said:
>> 
>>> On 3/3/24 10:10, Nick Alcock wrote:
>>>> On 2 Mar 2024, Nicholas Vinson uttered the following:
>>>>
>>>>> diff --git a/libctf/configure.ac b/libctf/configure.ac
>>>>> index e4e430615bd..0494a537e78 100644
>>>>> --- a/libctf/configure.ac
>>>>> +++ b/libctf/configure.ac
>>>>> @@ -258,7 +258,8 @@ AC_CACHE_CHECK([for linker versioning flags], [ac_cv_libctf_version_script],
>>>>>       CFLAGS="$CFLAGS -fPIC"
>>>>>       AC_LINK_IFELSE([AC_LANG_SOURCE([[int ctf_foo (void) { return 0; }
>>>>>    				    int main (void) { return ctf_foo(); }]])],
>>>>> -		  [ac_cv_libctf_version_script="-Wl,--version-script='$srcdir/libctf.ver'"],
>>>>> +		  [ac_cv_libctf_version_script="-Wl,--version-script"
>>>>> +		   decommented_version_script=t],
>>>>>    		  [])
>>>>>       LDFLAGS="$old_LDFLAGS"
>>>> Not quite. The stanza you changed there is meant for GNU ld, which
>>>> supports both undefined symbols in version scripts and /* comments */
>>>> and needs none of this hackery.
>>>> We have a three-case case statement here, with the final last-ditch
>>>> attempt being -export-symbols-regex=, and we need to add another case
>>>> for "no -B local, doesn't like nonexistent symbols, supports
>>>> --version-script".
>>>
>>> The three cases are not fully independent. The first case defines conftest.ver and conftest.c in such a way that when the test is
>>> run both ld.bfd and ld.lld pass. As a result, ac_cv_libctf_version_script is set.
>> Yep.
>> 
>>> Then the next check is if ac_cv_libctf_version_script is empty. It's not, so the check with '-Wl,-B,local' is never run.
>> That's why I added 'nonexistent' to the version script at that point, to
>> ensure that if the linker objects to nonexistent symbols in the version
>> script, we fail that test, leaving ac_cv_libctf_version_script unset. So
>> the recent-lld setup fails that, cascades into the Solaris version,
>> fails that, then hits the next case, freshly added, and passes it.
>> 
>>> Even though ld.bfd allows undefined symbols in the symbol version map,
>>> I believe it excludes them in the libctf symbol table. If that's
>>> correct, then I request this change be reconsidered because it's a
>>> much simpler change that supports both ld.bfd and ld.lld.
>> The *test* is simpler, but it expands use of the horrendous hack which
>> involves sed-transforming the version script. I was trying to minimize
>> the number of cases in which we have to use that... but I guess if it's
>> increasingly a lost cause to do so we could move to doing the
>> sed-transformation everywhere that supports version scripts at all. It
>> would certainly simplify things a bit (though not completely simplify
>> this away, since Solaris still needs different flags as well).
>
> I really think using the "horrendous hack" is the better choice. ld.bfd and ld.lld both support the --undefined-version flag which
> also solves the problem. Unfortunately, only recent versions of ld.bfd support the flag. This means, of course, configure.ac would
> need a check to see if the linker supports the flag before binutils tries to use it.
>
> Furthermore, I don't have any way of testing my changes against Solaris linker(s), so I would like to minimize the changes I make to
> Solaris-specific checks.

OK, I'll whip that up shortly -- as soon as I've got some *other*
backlogged stuff in, and also figured out the cause of that ldlex_wild
problem I believe you ran into earlier, since it's now biting all my
environments and seems to be fairly easy to trigger. :(