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

[Simon Marchi <simark at simark dot ca>]

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.

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.


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

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.

Simon