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

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

On Tue, Feb 13, 2018 at 10:32 AM, Sriraman Tallam <tmsriram@google.com> wrote:
> 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.

Please ignore this part below:

 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