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

["Teresa Johnson via binutils" <binutils at sourceware dot org>]

Confirmed this works for my LTO test cases (and no change required to the
LLVM gold plugin with this approach).

Thanks,
Teresa

On Wed, Feb 14, 2018 at 12:57 PM, Sriraman Tallam <tmsriram@google.com>
wrote:

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



-- 
Teresa Johnson |  Software Engineer |  tejohnson@google.com |  408-460-2413