Re: [PATCH] Better supported way to set symbol versions for legacy syscalls

[Mike Frysinger]

On 28 May 2022 22:42, Alexander Miller wrote:

i'm not anti this patch, but some of the commit message claims seem ...
inaccurate and/or misleading.

> Using a linker script to set symbol versions is an undocumented
> hack and doesn't work reliably in many cases.

which part is undocumented ?

https://sourceware.org/binutils/docs-2.39/ld/Implicit-Linker-Scripts.html
> If you specify a linker input file which the linker can not recognize as an
> object file or an archive file, it will try to read the file as a linker
> script. If the file can not be parsed as a linker script, the linker will
> report an error.

https://sourceware.org/binutils/docs-2.39/ld/Assignments.html
> You may assign a value to a symbol in a linker script. This will define
> the symbol and place it into the symbol table with a global scope.

https://sourceware.org/binutils/docs-2.39/ld/Simple-Assignments.html
> You may assign to a symbol using any of the C assignment operators:
> symbol = expression ;
> ...
> The first case will define symbol to the value of expression.
> ...
> You may write symbol assignments as commands in their own right, ...

https://sourceware.org/binutils/docs-2.39/ld/VERSION.html
this talks indirectly about defined symbols and how they're bound to versions.
there isn't explicit mention of where/how symbols must be defined, but i think
it's reasonable to extrapolate that a symbol defined using the @ syntax will be
processed as such.

> It works (to some degree) with the bfd linker, but fails with gold or lld.

older lld versions had bugs in how it processed linker scripts & symbol
assignments, including with quotes, but they fixed those.

gold's processing has been reported as a bug.  it processes the symbols
correctly in the linker script, but then seems to apply incorrect
visibility via the version script.

> And even with bfd it can break when using --gc-sections or LTO.

if these functions are culling symbols referenced via linker scripts,
you should report them as bugs to the respective projects.

sounds like the LTO bug has already been fixed.

> --- a/libattr/syscalls.c
> +++ b/libattr/syscalls.c
> @@ -26,6 +26,27 @@
>  #include <sys/syscall.h>
>  #include <sys/xattr.h>
> 
> +/*
> + * Versioning of compat symbols:
> + * prefer symver attribute if available (since gcc 10),
> + * fall back to traditional .symver asm directive otherwise.
> + */
> +#ifdef __has_attribute
> +# if __has_attribute(symver)
> +#  define SYMVER(cn, vn) __typeof(cn) cn __attribute__((symver(vn)))
> +# elif __has_attribute(no_reorder)
> +   /*
> +    * Avoid wrong partitioning with older gcc and LTO. May not work reliably
> +    * with all versions; use -flto-partition=none if you encounter problems.
> +    */
> +#  define SYMVER(cn, vn) __typeof(cn) cn __attribute__((noreorder)); \
> +                      __asm__(".symver " #cn "," vn)

attribute names should use the __ scoped values like you've already done with
the attribute & asm directives.  this avoids namespace clashes.  while not a
likely issue currently, it's standard good hygiene.
-mike

signature.asc
Description: PGP signature