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

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

New patch attached.


* 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.
* plugin.h (Plugin_manager::is_defsym_def): New method.
(Plugin_manager::Plugin_manager): Initialize defsym_defines_set_.
(Plugin_manager::defsym_defines_set_): New member.
(Plugin_manager::Defsym_defines_set): New typedef.
* script.cc (Script_options::set_defsym_uses_in_real_elf): New method.
(Script_options::find_defsym_defs): 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::find_defsym_defs): 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.


On Wed, Feb 14, 2018 at 10:29 AM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Tue, Feb 13, 2018 at 10:14 PM, Cary Coutant <ccoutant@gmail.com> wrote:
>> +  void
>> +  set_expr_sym_in_real_elf(Symbol_table* symtab) const
>> +  {
>> +    Symbol* sym = symtab->lookup(this->name_.c_str());
>> +    sym->set_in_real_elf();
>> +  }
>>
>> Since you pointed out that this is running before
>> define_script_symbols(), I now realize that some of these symbols
>> might not be in the symbol table, and Symbol_table::lookup() can
>> return NULL. I think if that's the case, though, the symbol is not
>> referenced from IR, so it is unimportant, and you can just ignore
>> trying to set the in_real_elf flag if sym == NULL. Agreed?
>
> Yes, thanks for pointing this out.  Will make that change, thanks!
>
>>
>> -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
>>>>>
>>>>>
>>>>

Attachment: plugin_defsym_patch.txt
Description: Text document