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

  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.
---
 gdb/ChangeLog |    5 +++++
 gdb/minsyms.c |   17 +++--------------
 2 files changed, 8 insertions(+), 14 deletions(-)
  

Weimin Pan <weimin.pan@oracle.com> writes:

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

Why do we need to fix (2)?  It is a result of (1).  If DW_AT_location is
generated,

info address K::another_thread_local^M
Symbol "K::another_thread_local" is a thread-local variable at offset 0x4 in the thread-local storage for `gdb/testsuite/outputs/gdb.threads/tls/tls'.

without DW_AT_location, how does GDB tell where this variable is?  The
right fix to me is to fix GCC bug PR 83010, and xfail these tests here
for aarch64.
  

On Thu, Nov 16, 2017 at 9:13 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> without DW_AT_location, how does GDB tell where this variable is?  The
> right fix to me is to fix GCC bug PR 83010, and xfail these tests here
> for aarch64.

If I use clang (5.0), DW_AT_location is emitted, no failures at all.

# of expected passes 78
  

On 11/16/2017 1:13 AM, Yao Qi wrote:
> Weimin Pan <weimin.pan@oracle.com> writes:
>
>> (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).
> Why do we need to fix (2)?  It is a result of (1).  If DW_AT_location is
> generated,
>
> info address K::another_thread_local^M
> Symbol "K::another_thread_local" is a thread-local variable at offset 0x4 in the thread-local storage for `gdb/testsuite/outputs/gdb.threads/tls/tls'.
>
> without DW_AT_location, how does GDB tell where this variable is?  The
> right fix to me is to fix GCC bug PR 83010, and xfail these tests here
> for aarch64.
>

If a TLS does not have a DW_AT_location, it can still be found in the
.tbss section (with flag SEC_THREAD_LOCAL being set) which GLIBC uses
for TLS storage and that's what gdb function info_address_command()
tries to find by calling lookup_minimal_symbol_and_objfile().

Problem is, as described in the patch, lookup_minimal_symbol_and_objfile()
only looked up an objfile's minsym ordinary hash table, not its demangled
hash table, and resulted in not finding the C++ name.
  

On 17-11-16 14:40:18, Wei-min Pan wrote:
> 
> If a TLS does not have a DW_AT_location, it can still be found in the
> .tbss section (with flag SEC_THREAD_LOCAL being set) which GLIBC uses
> for TLS storage and that's what gdb function info_address_command()
> tries to find by calling lookup_minimal_symbol_and_objfile().
> 
> Problem is, as described in the patch, lookup_minimal_symbol_and_objfile()
> only looked up an objfile's minsym ordinary hash table, not its demangled
> hash table, and resulted in not finding the C++ name.
> 
> 

I finally understand what your patches does.  It is about finding TLS
variables from msymbol instead of full symbol.  It is nothing specific
to aarch64.  We've already had a test case gdb.threads/tls-nodebug.exp,
but it is about C, rather than C++.  Can you extend this test case for
a C++ TLS (which is mangled)?  I expect that GDB can't find the mangled
TLS without debug info on any architecture, and your patch fixes this
issue.

Even with your patch applied, there is still one fail in
gdb.threads/tls.exp,

-Cannot read `a_thread_local' without registers^M
-(gdb) PASS: gdb.threads/tls.exp: print a_thread_local
+Cannot find thread-local storage for process 0, executable file /home/yao.qi/gnu/build/gdb/testsuite/outputs/gdb.threads/tls/tls:^M
+Cannot find thread-local variables on this target^M
+(gdb) FAIL: gdb.threads/tls.exp: print a_thread_local

Looks GDB error messages in different code patch should be adjusted
as well.
  

On 11/21/2017 7:36 AM, Yao Qi wrote:
> On 17-11-16 14:40:18, Wei-min Pan wrote:
>> If a TLS does not have a DW_AT_location, it can still be found in the
>> .tbss section (with flag SEC_THREAD_LOCAL being set) which GLIBC uses
>> for TLS storage and that's what gdb function info_address_command()
>> tries to find by calling lookup_minimal_symbol_and_objfile().
>>
>> Problem is, as described in the patch, lookup_minimal_symbol_and_objfile()
>> only looked up an objfile's minsym ordinary hash table, not its demangled
>> hash table, and resulted in not finding the C++ name.
>>
>>
> I finally understand what your patches does.  It is about finding TLS
> variables from msymbol instead of full symbol.  It is nothing specific
> to aarch64.  We've already had a test case gdb.threads/tls-nodebug.exp,
> but it is about C, rather than C++.  Can you extend this test case for
> a C++ TLS (which is mangled)?  I expect that GDB can't find the mangled
> TLS without debug info on any architecture, and your patch fixes this
> issue.

If the test is built with no -g, my patch won't be able to find the C++ 
TLS either.  For example,

class foo {
  public:
   static __thread int total;
};

__thread int foo::total = 31;

Its mangled name is _ZN3foo5totalE and it's the only way to access the 
C++ TLS.

>
> Even with your patch applied, there is still one fail in
> gdb.threads/tls.exp,
>
> -Cannot read `a_thread_local' without registers^M
> -(gdb) PASS: gdb.threads/tls.exp: print a_thread_local
> +Cannot find thread-local storage for process 0, executable file /home/yao.qi/gnu/build/gdb/testsuite/outputs/gdb.threads/tls/tls:^M
> +Cannot find thread-local variables on this target^M
> +(gdb) FAIL: gdb.threads/tls.exp: print a_thread_local

Please note that the failed "print" command, which operated on a C TLS, was
issued before the test program got executed. For arch other than 
aarch64, the
checking of SYMBOL_NEEDS_REGISTERS && target_has_registers was done 
early and
resulted in "without registers" error for program that hasn't started in
default_read_var_value().

But for aarch64, it didn't fail until the to_get_thread_local_address() call
which caused the "Cannot find thread-local storage" exception.

BTW, this was (1) in my original patch report.


Thanks.

>
> Looks GDB error messages in different code patch should be adjusted
> as well.
>
  

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
  

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
  

On 2018-03-19 15:36, Weimin Pan wrote:
> 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

Maybe "info address" instead of "info"?

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

Thanks for your patience :).  This refactoring can be done in a separate 
patch, to keep this one focused on fixing the bug.

Simon
  

Hi Simon,

On 3/21/2018 9:22 PM, Simon Marchi wrote:
> On 2018-03-19 15:36, Weimin Pan wrote:
>> 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
>
> Maybe "info address" instead of "info"?

OK.

>
>>> 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.
>
> Thanks for your patience :).  This refactoring can be done in a 
> separate patch, to keep this one focused on fixing the bug.

Thank you very much for getting this process moving :) Would you like me 
to submit a revised patch which calls
lookup_bound_minimal_symbol instead of lookup_minimal_symbol_and_objfile?

Weimin

>
> Simon
  

On 2018-03-22 14:18, Wei-min Pan wrote:
> Thank you very much for getting this process moving :) Would you like
> me to submit a revised patch which calls
> lookup_bound_minimal_symbol instead of 
> lookup_minimal_symbol_and_objfile?

Hi Wei-min,

If you'd like to clean up these functions a bit, you can go ahead and 
push this patch and make another one more focused of 
cleanup/refactoring.

Simon
  

On 3/22/2018 1:16 PM, Simon Marchi wrote:
> On 2018-03-22 14:18, Wei-min Pan wrote:
>> Thank you very much for getting this process moving :) Would you like
>> me to submit a revised patch which calls
>> lookup_bound_minimal_symbol instead of 
>> lookup_minimal_symbol_and_objfile?
>
> Hi Wei-min,
>
> If you'd like to clean up these functions a bit, you can go ahead and 
> push this patch and make another one more focused of cleanup/refactoring.
>
> Simon

OK, thanks. But I don't think that I have the permission to push the patch.
  

On 2018-03-22 04:49 PM, Wei-min Pan wrote:
> OK, thanks. But I don't think that I have the permission to push the patch.

Hi Weimin,

I pushed the patch for you.  Do you intend on submitting a patch that removes
lookup_minimal_symbol_and_objfile and replaces its usages with
lookup_bound_minimal_symbol?

Simon
  

Will submit a revised patch which

  (1) calls lookup_bound_minimal_symbol in info_address_command() and
  (2) corrects the doc for lookup_minimal_symbol_and_objfile() in minsyms.h.

Thanks,
Weimin

On 3/23/2018 8:10 PM, Simon Marchi wrote:
> On 2018-03-22 04:49 PM, Wei-min Pan wrote:
>> OK, thanks. But I don't think that I have the permission to push the patch.
> Hi Weimin,
>
> I pushed the patch for you.  Do you intend on submitting a patch that removes
> lookup_minimal_symbol_and_objfile and replaces its usages with
> lookup_bound_minimal_symbol?
>
> Simon
  

On 2018-03-24 15:45, Wei-min Pan wrote:
> Will submit a revised patch which
> 
>  (1) calls lookup_bound_minimal_symbol in info_address_command() and
>  (2) corrects the doc for lookup_minimal_symbol_and_objfile() in 
> minsyms.h.

I did (2) when pushing your patch (I just removed the mention about it 
only looking through linkage names).  But I think the idea now would be 
to get rid of lookup_minimal_symbol_and_objfile, since it's basically 
the same as lookup_bound_minimal_symbol, isn't it?

Simon
  

On 3/24/2018 12:52 PM, Simon Marchi wrote:
> On 2018-03-24 15:45, Wei-min Pan wrote:
>> Will submit a revised patch which
>>
>>  (1) calls lookup_bound_minimal_symbol in info_address_command() and
>>  (2) corrects the doc for lookup_minimal_symbol_and_objfile() in 
>> minsyms.h.
>
> I did (2) when pushing your patch (I just removed the mention about it 
> only looking through linkage names).  But I think the idea now would 
> be to get rid of lookup_minimal_symbol_and_objfile, since it's 
> basically the same as lookup_bound_minimal_symbol, isn't it?
>

Big difference is  lookup_minimal_symbol_and_objfile(char *name) only 
searches the ordinary hash table for "name"
while lookup_bound_minimal_symbol(char *name) does both the ordinary 
hash table  and the demangled hash table.


> Simon
  

On 2018-03-24 16:11, Wei-min Pan wrote:
> Big difference is  lookup_minimal_symbol_and_objfile(char *name) only
> searches the ordinary hash table for "name"
> while lookup_bound_minimal_symbol(char *name) does both the ordinary
> hash table  and the demangled hash table.

Am I missing something?  Your patch (the origin of this thread) changed 
lookup_minimal_symbol_and_objfile to also search for demangled minsyms, 
didn't it?

Simon
  

On 3/24/2018 1:44 PM, Simon Marchi wrote:
> On 2018-03-24 16:11, Wei-min Pan wrote:
>> Big difference is lookup_minimal_symbol_and_objfile(char *name) only
>> searches the ordinary hash table for "name"
>> while lookup_bound_minimal_symbol(char *name) does both the ordinary
>> hash table  and the demangled hash table.
>
> Am I missing something?  Your patch (the origin of this thread) 
> changed lookup_minimal_symbol_and_objfile to also search for demangled 
> minsyms, didn't it?

Yes, originally I changed lookup_minimal_symbol_and_objfile to call 
lookup_bound_minimal_symbol for each objfile
and now lookup_bound_minimal_symbol will call 
lookup_bound_minimal_symbol once.

>
> Simon