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

[Cary Coutant <ccoutant at gmail dot com>]

+  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?

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