Re: [PATCH PR gdb/18071] TLS variables can't be resolved on aarch64-linux-gnu

[Weimin Pan <weimin.pan@oracle.com>]

On 3/17/2018 9:52 AM, Simon Marchi wrote:
> On 2017-11-02 08:38 PM, Weimin Pan wrote:
>> Running the test case with upstream gdb shows two failures:
>>
>> (1) Receiving different error messages when printing TLS variable before
>>      program runs - because the ARM compiler does not emit dwarf attribute
>>      DW_AT_location for TLS, the result is expected and the baseline may
>>      need to be changed for aarch64.
>>
>> (2) Using "info address" command on C++ static TLS object resulted in
>>      "symbol unresolved" error - below is a snippet from the test case:
>>
>> class K {
>>   public:
>>    static __thread int another_thread_local;
>> };
>>
>> __thread int K::another_thread_local;
>>
>> (gdb) info address K::another_thread_local
>> Symbol "K::another_thread_local" is unresolved.
>>
>> This patch contains fix for (2).
>>
>> Function info_address_command() handles the "info address" command and
>> calls lookup_minimal_symbol_and_objfile() to find sym's symbol entry in
>> mininal symbol table if SYMBOL_COMPUTED_OPS (sym) is false. Problem is
>> that function lookup_minimal_symbol_and_objfile() only looked up an
>> objfile's minsym ordinary hash table, not its demangled hash table, which
>> was the reason why the C++ name was not found.
>>
>> The fix is to call lookup_minimal_symbol(), which already looks up entries
>> in both minsym's hash tables, to find names when traversing the object file
>> list in lookup_minimal_symbol_and_objfile().
>>
>> Tested in both aarch64-linux-gnu and amd64-linux-gnu. No regressions.
> Hi Weimin,
>
> I would be ok with making lookup_minimal_symbol_and_objfile look through demangled
> names as well, I don't see a need for it to only search through linkage names.
> I don't think it should break any user of that function, since it would mean that
> there is a clash between a mangled name and a demangled name, I don't think this is
> likely.

Hi Simon,

Yes, you're right that a name clash is unlikely to happen. I think you have
a better suggestion, at the end of your email, to leave this function alone.
But it'd be good to update the doc for lookup_minimal_symbol_and_objfile(),
where the term "linkage name" should be replaced with either "ordinary" or
"demangled" because it seems to me that "linkage name" == "mangled name".

> The doc of lookup_minimal_symbol_and_objfile in minsyms.h should be updated.
>
> So this now works:
>
> (gdb) info address K::another_thread_local2
> Symbol "K::another_thread_local2" is a thread-local variable at offset 0x4 in the thread-local storage for `/home/simark/build/binutils-gdb/gdb/test'.
>
> But printing the variable doesn't:
>
> (gdb) p K::another_thread_local2
> Cannot find thread-local storage for process 8959, executable file /home/simark/build/binutils-gdb/gdb/test:
> Cannot find thread-local variables on this target
>
> Is this also what you see?  If the scope of this patch is to only fix "info address",
> could you change the title of the patch to reflect it?  Otherwise it sounds like
> it fixes actually accessing/printing the variable as well.

Yes, printing of a TLS fails on all platforms, not just on aarch64.
How about changing the title to:

    [PATCH PR gdb/18071] aarch64: "info" command can't resolve TLS variables

>>   gdb/ChangeLog |    5 +++++
>>   gdb/minsyms.c |   17 +++--------------
>>   2 files changed, 8 insertions(+), 14 deletions(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 4b292e0..2f630bc 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,8 @@
>> +2017-11-01  Weimin Pan  <weimin.pan@oracle.com>
>> +
>> +	* minsyms.c (lookup_minimal_symbol_and_objfile): Use
>> +	lookup_minimal_symbol() to find symbol entry.
>> +
>>   2017-10-27  Keith Seitz  <keiths@redhat.com>
>>   
>>   	* breakpoint.c (print_breakpoint_location): Use the symbol saved
>> diff --git a/gdb/minsyms.c b/gdb/minsyms.c
>> index 37edbd8..4edd8b1 100644
>> --- a/gdb/minsyms.c
>> +++ b/gdb/minsyms.c
>> @@ -881,23 +881,12 @@ lookup_minimal_symbol_and_objfile (const char *name)
>>   {
>>     struct bound_minimal_symbol result;
>>     struct objfile *objfile;
>> -  unsigned int hash = msymbol_hash (name) % MINIMAL_SYMBOL_HASH_SIZE;
>>   
>>     ALL_OBJFILES (objfile)
>>       {
>> -      struct minimal_symbol *msym;
>> -
>> -      for (msym = objfile->per_bfd->msymbol_hash[hash];
>> -	   msym != NULL;
>> -	   msym = msym->hash_next)
>> -	{
>> -	  if (strcmp (MSYMBOL_LINKAGE_NAME (msym), name) == 0)
>> -	    {
>> -	      result.minsym = msym;
>> -	      result.objfile = objfile;
>> -	      return result;
>> -	    }
>> -	}
>> +      result = lookup_minimal_symbol (name, NULL, objfile);
>> +      if (result.minsym != NULL)
>> +        return result;
>>       }
> Is this code equivalent to calling lookup_minimal_symbol (name, NULL, NULL) ?  If
> so, there's already lookup_bound_minimal_symbol that does the same, so maybe we
> can just drop lookup_minimal_symbol_and_objfile and use lookup_bound_minimal_symbol.

Yes, it turns out lookup_minimal_symbol_and_objfile(name) is equivalent to
calling lookup_minimal_symbol (name, NULL, NULL). We can replace the call in
info_address_command() with either lookup_minimal_symbol (name, NULL, NULL)
or lookup_bound_minimal_symbol(name). Calling either one looks like a 
better fix.

>
> We could also just have lookup_minimal_symbol with parameters that default to nullptr.
> It is not clear at all to have lookup_bound_minimal_symbol and lookup_minimal_symbol
> that both return a bound_minimal_symbol, that's quite misleading.

I don't know the rational behind having these two functions which get 
called in
quite a few places. Yes, having default arguments for 
lookup_minimal_symbol()
will be another option.

Thanks very much for your comments.

Weimin

>
> Simon