Re: Linker plugins should be aware of --defsym during symbol resolution

["Sriraman Tallam via binutils" <binutils at sourceware dot org>]

On Tue, Feb 13, 2018 at 9:59 PM, Cary Coutant <ccoutant@gmail.com> wrote:
> @@ -989,6 +989,9 @@ Pluginobj::get_symbol_resolution_info(Symbol_table* symtab,
>        return version > 2 ? LDPS_NO_SYMS : LDPS_OK;
>      }
>
> +  Layout *layout = parameters->options().plugins()->layout();
> +  layout->script_options()->set_defsym_uses_in_real_elf(symtab);
>
> This is going to run set_defsym_uses_in_real_elf() every time the
> get_symbols API is called -- i.e., once for each input object. It only
> needs to be done once, so I think it would be appropriate to call this
> from Plugin_manager::all_symbols_read().

I did think of this :) added a check and then removed it as I thought
there is no real reason to call get_symbol_resolution_info more than
once.  Now looking at the plugin, I see this can be called for every
claimed file, sorry about that.

>
> If you're still worried about the performance of is_defsym_def(),
> all_symbols_read() might also be a good time to build a quick little
> hash table of all the symbols on the LHS of an assignment.

I fear the performance could be bad with heavy use of linker scripts
but I have no idea on how long Script_options::symbol_assignments_
could potentially get.

Thanks
Sri


>
> -cary
>
>
> On Tue, Feb 13, 2018 at 5:05 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>> New patch attached with all changes made.
>>
>> gold/
>> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):
>>       New method.
>> (Unary_expression::set_expr_sym_in_real_elf): New method.
>> (Binary_expression::set_expr_sym_in_real_elf): New method.
>> (Trinary_expression::set_expr_sym_in_real_elf): New method.
>> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if
>> defined or used in defsyms.
>> * script.cc (set_defsym_uses_in_real_elf): New method.
>> (Script_options::is_defsym_def): New method.
>> * script.h (Expression::set_expr_sym_in_real_elf): New method.
>> (Symbol_assignment::is_defsym): New method.
>> (Symbol_assignment::value): New method.
>> (Script_options::is_defsym_def): New method.
>> (Script_options::set_defsym_uses_in_real_elf): New method.
>>
>> On Tue, Feb 13, 2018 at 5:03 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>>>
>>>
>>> On Tue, Feb 13, 2018 at 4:52 PM, Sriraman Tallam <tmsriram@google.com>
>>> wrote:
>>>>
>>>>
>>>>
>>>> On Tue, Feb 13, 2018 at 4:49 PM, Teresa Johnson <tejohnson@google.com>
>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On Tue, Feb 13, 2018 at 4:04 PM, Cary Coutant <ccoutant@gmail.com> wrote:
>>>>>>
>>>>>> >> Do you have a real-world example? I'm having trouble imagining a case
>>>>>> >> where --defsym would be used to override a symbol that's subject to
>>>>>> >> the ODR and yet remain a valid program.
>>>>>> >
>>>>>> > I just concocted one:
>>>>>> > ...
>>>>>> > With defsym:
>>>>>> >
>>>>>> > $ ~/llvm/llvm_8_build/bin/clang++ hello[12].cc  -fuse-ld=gold
>>>>>> > -Wl,--defsym,_Z3barv=_Z3bazv
>>>>>>
>>>>>> To me, this is the same as providing an overriding definition of bar()
>>>>>> that prints "in baz", which clearly violates the one-definition rule.
>>>>>>
>>>>>> On what basis do you consider this a valid thing to do, to the extent
>>>>>> that you want to preserve the unoptimized behavior across LTO?
>>>>>>
>>>>>> Is there a real-world example where someone would want to do this in
>>>>>> production code? I'm afraid I'd have zero sympathy for them. If they
>>>>>> want something like that to work, they should just turn off
>>>>>> cross-module inlining.
>>>>>
>>>>>
>>>>> Fair enough. It was a contrived example, not based on anything I have
>>>>> seen so far. If that violates ODR then I agree all bets are off.
>>>>>
>>>>> Let me try with a modified change that marks these with
>>>>> LDPR_PREEMPTED_REG. Sri, would you mind changing the patch and I'll give the
>>>>> new version a try?
>>>
>>>
>>>
>>> New patch attached.
>>>
>>>
>>> gold/
>>> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):
>>>       New method.
>>> (Unary_expression::set_expr_sym_in_real_elf): New method.
>>> (Binary_expression::set_expr_sym_in_real_elf): New method.
>>> (Trinary_expression::set_expr_sym_in_real_elf): New method.
>>> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if
>>> defined or used in defsyms.
>>> * script.cc (set_defsym_uses_in_real_elf): New method.
>>> (Script_options::is_defsym_def): New method.
>>> * script.h (Expression::set_expr_sym_in_real_elf): New method.
>>> (Symbol_assignment::is_defsym): New method.
>>> (Symbol_assignment::value): New method.
>>> (Script_options::is_defsym_def): New method.
>>> (Script_options::set_defsym_uses_in_real_elf): New method.
>>> * testsuite/Makefile.am (plugin_test_defsym): New test.
>>> * testsuite/Makefile.in: Regenerate.
>>> * testsuite/plugin_test.c: Check for new symbol resolution.
>>> * testsuite/plugin_test_defsym.sh: New script.
>>> * testsuite/plugin_test_defsym.c: New test source.
>>>
>>>
>>>
>>>>
>>>>
>>>> No problem.
>>>>
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Teresa
>>>>>
>>>>>>
>>>>>> I need a lot more justification to extend the plugin API.
>>>>>>
>>>>>> -cary
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
>>>>
>>>>
>>>