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

Teresa Johnson via binutils binutils@sourceware.org
Thu Feb 15 03:14:00 GMT 2018


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



More information about the Binutils mailing list