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

[Nicholas Vinson]

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.

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.

conftest.ver and/or conftest.c could be updated so the test passes with 
ld.bfd and not ld.lld, in which case another block is added for ld.lld.


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.

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

> 
> diff --git a/libctf/configure.ac b/libctf/configure.ac
> index e4e430615bd..28f63792826 100644
> --- a/libctf/configure.ac
> +++ b/libctf/configure.ac
> @@ -251,7 +251,7 @@ AC_SUBST(HAVE_TCL_TRY)
>   # Use a version script, if possible, or an -export-symbols-regex otherwise.
>   decommented_version_script=
>   AC_CACHE_CHECK([for linker versioning flags], [ac_cv_libctf_version_script],
> -  [echo 'FOO { global: mai*; local: ctf_fo*; };' > conftest.ver
> +  [echo 'FOO { global: mai*; nonexistent; local: ctf_fo*; };' > conftest.ver
>      old_LDFLAGS="$LDFLAGS"
>      old_CFLAGS="$CFLAGS"
>      LDFLAGS="$LDFLAGS -shared -Wl,--version-script=conftest.ver"
> @@ -262,7 +262,10 @@ AC_CACHE_CHECK([for linker versioning flags], [ac_cv_libctf_version_script],
>   		  [])
>      LDFLAGS="$old_LDFLAGS"
>   
> +   # Solaris: -B local, nonexistent symbols prohibited: use preprocessed
> +   # version scripts
>      if test -z "$ac_cv_libctf_version_script"; then
> +     echo 'FOO { global: mai*; local: ctf_fo*; };' > conftest.ver
>        LDFLAGS="$LDFLAGS -shared -Wl,-B,local -Wl,-z,gnu-version-script=conftest.ver"
>        AC_LINK_IFELSE([AC_LANG_SOURCE([[int ctf_foo (void) { return 0; }
>   				      int main (void) { return ctf_foo(); }]])],
> @@ -273,6 +276,19 @@ AC_CACHE_CHECK([for linker versioning flags], [ac_cv_libctf_version_script],
>      fi
>      CFLAGS="$old_CFLAGS"
>   
> +   # LLD with --no-undefined-version on by default: no -B local, nonexistent
> +   # symbols prohibited: same solution as Solaris
> +   if test -z "$ac_cv_libctf_version_script"; then
> +     LDFLAGS="$LDFLAGS -shared -Wl,--version-script=conftest.ver"
> +     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"
> +		     decommented_version_script=t],
> +		    [])
> +     LDFLAGS="$old_LDFLAGS"
> +   fi
> +   CFLAGS="$old_CFLAGS"
> +
>      if test -z "$ac_cv_libctf_version_script"; then
>        ac_cv_libctf_version_script='-export-symbols-regex ctf_.*'
>      fi
> diff --git a/libctf/libctf.ver b/libctf/libctf.ver
> index c59847d012b..474852c2c84 100644
> --- a/libctf/libctf.ver
> +++ b/libctf/libctf.ver
> @@ -139,7 +139,6 @@ LIBCTF_1.0 {
>   
>   	ctf_arc_write;
>   	ctf_arc_write_fd;
> -	ctf_arc_open;
>   	ctf_arc_bufopen;
>   	ctf_arc_close;
>   	ctf_arc_open_by_name;
> @@ -165,6 +164,7 @@ LIBCTF_1.0 {
>   	ctf_link_shuffle_syms;
>   	ctf_link_write;
>   
> +	ctf_arc_open;                           /* libctf only.  */
>   	ctf_fdopen;                             /* libctf only.  */
>   	ctf_open;                               /* libctf only.  */
>   	ctf_bfdopen;                            /* libctf only.  */
> 
> base-commit: 90f8d97c8efa75f7f019b868eca9c626bc35203d