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

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

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?
>
> 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_.begin();
> +       p != this->symbol_assignments_.end();
> +       ++p)
> +    {
> +      if ((*p)->is_defsym() && (*p)->value()->is_symbol_in_expression(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?

This is a nice idea and I am able to make this work for defsym uses.
Anything I could do for definitions too?  I could add a defined_ field
in Symbol which is set to Defined::DEFSYM.  That would avoid
is_defsym_def too.

expression.cc:
uint64_t
Symbol_expression::value(const Expression_eval_info* eei)
{
  Symbol* sym = eei->symtab->lookup(this->name_.c_str());

somewhere here would be too late.




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