This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: Linker plugins should be aware of --defsym during symbol resolution
On Mon, Feb 12, 2018 at 12:19 PM, Cary Coutant <ccoutant@gmail.com> wrote:
> > include/
> > * plugin-api.h (LDPR_LINKER_REDEFINED): New enum value.
>
> As always, this change needs to be synced with the GCC tree, and the
> whopr/driver wiki page needs updating.
>
> Can you explain why this new resolution is needed? Why would
> LDPR_PREEMPTED_REG not work?
>
They mean different things. For example, LDPR_PREEMPTED_REG will mean that
there is a prevailing definition in a regular object file. But if the IR
def has weak ODR linkage, the LTO implementation knows that all copies must
be the same, and we can and currently do keep that def around long enough
for inlining. That would be incorrect in the defsym case, where this symbol
is actually redefined by the linker. That is what we are trying to
distinguish.
Teresa
> As we did with LDPR_PREVAILING_DEF_IRONLY_EXP and LDPS_NO_SYMS, you'll
> need to create a new version of get_symbols (LDPT_GET_SYMBOLS_V4), so
> that old plugins that aren't prepared for the new resolution enum
> don't break.
>
> Hmmm, in get_symbol_resolution_info, we have this:
>
> if (nsyms > this->nsyms_)
> return LDPS_NO_SYMS;
>
> That needs to be qualified with a test for version > 2 (as was done a
> few lines below).
>
> > gold/
> > * expression.cc (Symbol_expression::is_symbol_in_expression):
> > New method.
> > (Unary_expression::is_symbol_in_expression): New method.
> > (Binary_expression::is_symbol_in_expression): New method.
> > (Trinary_expression::is_symbol_in_expression): New method.
> > * plugin.cc (is_referenced_from_outside): Check if the symbol is
> > used in a defsym expression.
> > (get_symbol_resolution_info): Fix symbol resolution if defined or
> > used in defsyms.
> > * script.cc (Script_options::is_defsym_def): New method.
> > (Script_options::is_defsym_use): New method.
> > * script.h (Expression::is_symbol_in_expression): New method.
> > (Symbol_assignment::is_defsym): New method.
> > (Symbol_assignment::value): New method.
> > (Script_options::is_defsym_def): New method.
> > (Script_options::is_defsym_use): 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.
>
>
> +bool
> +Script_options::is_defsym_use(const char* name)
> +{
> + for (Symbol_assignments::iterator p = this->symbol_assignments_.begi
> n();
> + p != this->symbol_assignments_.end();
> + ++p)
> + {
> + if ((*p)->is_defsym() && (*p)->value()->is_symbol_in_ex
> pression(name))
> + return true;
> + }
> + return false;
> +}
>
> This looks like a pretty expensive test. Wouldn't it be better to just
> call Symbol::set_in_real_elf() for each symbol we see used in an
> expression?
>
> + bool
> + is_symbol_in_expression(const char* name)
> + {
> + return (this->left_->is_symbol_in_expression(name) ||
> + this->right_->is_symbol_in_expression(name));
> + }
>
> Operators should be moved to the beginning of the next line.
>
> + bool
> + is_symbol_in_expression(const char* name)
> + { return (this->arg1_->is_symbol_in_expression(name) ||
> + this->arg2_->is_symbol_in_expression(name) ||
> + this->arg3_->is_symbol_in_expression(name)); }
>
> Same, and the braces should be on their own lines here.
>
> -cary
>
--
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
<(408)%20460-2413>