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

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

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

>> Something like the below, perhaps. (Caveat: when I test with LLVM 17 and
>> LDFLAGS="-fuse-ld=lld -Wl,--no-undefined-version" there is no failure to
>> link libctf with trunk binutils, though ld building does fail:
>> ld.lld: error: undefined symbol: ldlex_backup
>>>>> referenced by ldgram.y:860 (../../ld/ldgram.y:860)
>>>>>                ldgram.o:(yyparse)
>>>>> referenced by ldgram.y:1125 (../../ld/ldgram.y:1125)
>>>>>                ldgram.o:(yyparse)
>>>>> referenced by ldgram.y:1146 (../../ld/ldgram.y:1146)
>>>>>                ldgram.o:(yyparse)
>>>>> referenced 1 more times
>> ld.lld: error: undefined symbol: ldlex_wild
>> so this is not really tested and all I can really say is that clang and
>> lld are still happy to link. Don't trust what I wrote here, please test
>> it out -- and obviously it still needs things like the removal of the
>> ctf_label_set symbols that genuinely don't exist, as well. That patch is
>> fine.)
>
> I haven't seen that issue and I've been testing with clang and ld.lld this entire time.
>
> I wonder if this is caused by flex/bison (or lex/yacc). For the record,
> I'm using flex-2.6.4 (Gentoo revision r6) and bison-3.8.2 (Gentoo revision r2).

flex 2.6.4-57 here, but otherwise identical. ldlex_backup is of course a
symbol in ld, and is definitely defined, as is ldlex_wild. Weird.

This is with LLVM 17.0.6 (release, from git).

-- 
NULL && (void)