[PATCH] ld: Add -Bsymbolic-functions tests

[H.J. Lu]

On Sat, May 23, 2020 at 8:56 PM Fangrui Song <maskray@google.com> wrote:
>
> On 2020-05-23, H.J. Lu wrote:
> >On Sat, May 23, 2020 at 7:56 PM Fangrui Song <maskray@google.com> wrote:
> >>
> >> On 2020-05-23, H.J. Lu wrote:
> >> >On Sat, May 23, 2020 at 6:05 PM Fangrui Song <maskray@google.com> wrote:
> >> >>
> >> >> On 2020-05-23, H.J. Lu wrote:
> >> >> >On Sat, May 23, 2020 at 5:11 PM Fangrui Song <maskray@google.com> wrote:
> >> >> >>
> >> >> >> On 2020-05-23, Fangrui Song wrote:
> >> >> >> >On 2020-05-23, H.J. Lu wrote:
> >> >> >> >>On Fri, May 22, 2020 at 8:56 AM Fangrui Song <maskray@google.com> wrote:
> >> >> >> >>>
> >> >> >> >>>
> >> >> >> >>>On 2020-05-22, H.J. Lu wrote:
> >> >> >> >>>>On Wed, May 20, 2020 at 6:48 PM Fangrui Song via Binutils
> >> >> >> >>>><binutils@sourceware.org> wrote:
> >> >> >> >>>>>
> >> >> >> >>>>> They aren't used together in reality, so it is safe to change the
> >> >> >> >>>>> semantics. --dynamic-list is refined -Bsymbolic. -Bsymbolic-functions
> >> >> >> >>>>> should be considered as a subset of -Bsymbolic, so --dynamic-list
> >> >> >> >>>>> overridding -Bsymbolic implies that --dynamic-list overridding
> >> >> >> >>>>> -Bsymbolic-functions.
> >> >> >> >>>>
> >> >> >> >>>>--dynamic-list=DYNAMIC-LIST-FILE'
> >> >> >> >>>>     Specify the name of a dynamic list file to the linker.  This is
> >> >> >> >>>>     typically used when creating shared libraries to specify a list of
> >> >> >> >>>>     global symbols whose references shouldn't be bound to the
> >> >> >> >>>>     definition within the shared library, or creating dynamically
> >> >> >> >>>>     linked executables to specify a list of symbols which should be
> >> >> >> >>>>     added to the symbol table in the executable.  This option is only
> >> >> >> >>>>     meaningful on ELF platforms which support shared libraries.
> >> >> >> >>>>
> >> >> >> >>>>The --dynamic-list* options are intended for shared libraries.
> >> >> >> >>>
> >> >> >> >>>--dynamic-list* work for both executables and shared libraries.
> >> >> >> >>>For an executable, export some symbols.
> >> >> >> >>>For a shared library, specify preemptible symbols.
> >> >> >> >>>
> >> >> >> >>>>The goal
> >> >> >> >>>>IS NOT put them in dynamic symbol table since all global symbols are in
> >> >> >> >>>>dynamic symbol table already in a shared library.  The goal is to make
> >> >> >> >>>>them PREEMPTIBLE.
> >> >> >> >>>
> >> >> >> >>>I agree with this sentence and the patch respects it.
> >> >> >> >>>
> >> >> >> >>>>So making the --dynamic-list* options override -Bsymbolic
> >> >> >> >>>>and -Bsymbolic-functions is incorrect since there is NOTHING to override.
> >> >> >> >>>
> >> >> >> >>>I don't agree with this statement. --dynamic-list* have function overlay
> >> >> >> >>>with -Bsymbolic. When two options overlap in functionality, many users
> >> >> >> >>>expect the more fine-grained option to win, thus my thought that
> >> >> >> >>>--dynamic-list* override -Bsymbolic and -Bsymbolic-functions.
> >> >> >> >>>
> >> >> >> >>>(FWIW LLD's preemptibility logic is
> >> >> >> >>>   ...
> >> >> >> >>>   if (config->hasDynamicList)
> >> >> >> >>>     return sym.inDynamicList;
> >> >> >> >>>   return !(config->bsymbolic || config->bsymbolicFunctions && sym.isFunc());
> >> >> >> >>>   This works fine and I won't change it.
> >> >> >> >>>)
> >> >> >> >>>
> >> >> >> >>>>To do it properly:
> >> >> >> >>>>
> >> >> >> >>>>1.  Extend  --dynamic-list* to executables.  Symbol binding is unchanged.
> >> >> >> >>>>Only add symbols to dynamic symbol table.
> >> >> >> >>>
> >> >> >> >>>This was already implemented when you implemented --dynamic-list in
> >> >> >> >>>2006. This patch does not change the fact.
> >> >> >> >>>
> >> >> >> >>>>2. Make --export-dynamic-symbol an alias of  --dynamic-list* for executables.
> >> >> >> >>>>and ignore --export-dynamic-symbol for -shared.  It has to be done at the
> >> >> >> >>>>end of command-line parsing.
> >> >> >> >>
> >> >> >> >>There is no need to change linker manual.  --dynamic-list* should work both
> >> >> >> >>before and after -Bsymbolic and -Bsymbolic-functions.  Your patch used
> >> >> >> >>libdl2d.so without creating it first.
> >> >> >> >>
> >> >> >> >>Here is the updated version of your patch.  I took the liberty to make
> >> >> >> >>the changes above.
> >> >> >> >>
> >> >> >> >>--
> >> >> >> >>H.J.
> >> >> >> >
> >> >> >> >Thanks for the fix (libdl4e.so->libdl2d.so)
> >> >> >> >
> >> >> >> >The updated patch looks good to me.
> >> >> >>
> >> >> >> Around lexsup.c:1162
> >> >> >>
> >> >> >> `case symbolic_functions:` should set link_info.dynamic_data and link_info.dynamic , otherwise
> >> >> >> -Bsymbolic-functions is broken.
> >> >> >>
> >> >> >>
> >> >> >> .globl _start, foo
> >> >> >> _start:
> >> >> >>    call foo
> >> >> >> foo
> >> >> >>
> >> >> >> ld-new -shared -Bsymbolic-functions a.o -o a.so
> >> >> >>
> >> >> >> foo@plt is not created
> >> >> >
> >> >> >'-Bsymbolic-functions'
> >> >> >     When creating a shared library, bind references to global function
> >> >> >     symbols to the definition within the shared library, if any.  This
> >> >> >     option is only meaningful on ELF platforms which support shared
> >> >> >     libraries.
> >> >> >
> >> >> >There should be no foo@plt.
> >> >>
> >> >> Sorry, my example missed .type:
> >> >>
> >> >> .global _start, foo
> >> >> .type foo, @function
> >> >> _start:
> >> >>    call foo
> >> >> foo:
> >> >>
> >> >>
> >> >> ld-new -shared -Bsymbolic-functions a.o -o a.so
> >> >> foo@plt was created without the patch.
> >> >
> >> >On master branch, I got
> >> >
> >> >[hjl@gnu-cfl-2 pr26018]$ cat func.s
> >> >.global _start, foo
> >> >.type foo, %function
> >> >.text
> >> >_start:
> >> >call foo
> >> >foo:
> >> >ret
> >> >[hjl@gnu-cfl-2 pr26018]$ make func.so
> >> >as   -o func.o func.s
> >> >./ld -shared  -Bsymbolic-function -o func.so func.o
> >> >[hjl@gnu-cfl-2 pr26018]$ objdump -dw func.so
> >> >
> >> >func.so:     file format elf64-x86-64
> >> >
> >> >
> >> >Disassembly of section .text:
> >> >
> >> >0000000000001000 <_start>:
> >> >    1000: e8 00 00 00 00        callq  1005 <foo>
> >> >
> >> >0000000000001005 <foo>:
> >> >    1005: c3                    retq
> >> >[hjl@gnu-cfl-2 pr26018]$
> >> >
> >> >It looks OK to me.
> >>
> >> At master, foo is bound locally (intended).
> >> With this patch, foo@plt is created
> >>
> >
> >I am checking in this.
>
> Thank for the test. I think the filename can be `Bsymbolic-functions.s`
> to make it clear its relation with the option.

Maybe next time.

-- 
H.J.