[PATCH] Fix return code in _bfd_dwarf2_find_nearest_line().

[PATCH] Fix return code in _bfd_dwarf2_find_nearest_line().

[Steinar H. Gunderson]

If we find a function name through the debug ranges, but no line
information, we will have found = false (unless we happened to be
in the last compilation unit).

This means _bfd_dwarf2_find_nearest_line() will return 0 (not found)
instead of 2 (partial information found), causing the caller
(_bfd_elf_find_nearest_line) to go through an expensive linear
traversal of all symbols (_bfd_elf_find_function) in a fallback
effort to find a function name. Fix the return code for this case.
---
 bfd/dwarf2.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/bfd/dwarf2.c b/bfd/dwarf2.c
index edbcb5069ae..4d232ba1f79 100644
--- a/bfd/dwarf2.c
+++ b/bfd/dwarf2.c
@@ -5602,7 +5602,11 @@ _bfd_dwarf2_find_nearest_line (bfd *abfd,
 
  done:
   if (functionname_ptr && function && function->is_linkage)
-    *functionname_ptr = function->name;
+    {
+      *functionname_ptr = function->name;
+      if (!found)
+        found = 2;
+    }
   else if (functionname_ptr
 	   && (!*functionname_ptr
 	       || (function && !function->is_linkage)))
-- 
2.35.1

Re: [PATCH] Fix return code in _bfd_dwarf2_find_nearest_line().

[Nick Clifton]

Hi Steinar,

> +++ b/bfd/dwarf2.c
> @@ -5602,7 +5602,11 @@ _bfd_dwarf2_find_nearest_line (bfd *abfd,
>   
>    done:
>     if (functionname_ptr && function && function->is_linkage)
> -    *functionname_ptr = function->name;
> +    {
> +      *functionname_ptr = function->name;
> +      if (!found)
> +        found = 2;
> +    }
>     else if (functionname_ptr
>   	   && (!*functionname_ptr
>   	       || (function && !function->is_linkage)))

Patch approved and applied.  Thanks for fixing this!

Cheers
   Nick

Re: [PATCH] Fix return code in _bfd_dwarf2_find_nearest_line().

[Steinar H. Gunderson]

On Tue, Mar 22, 2022 at 03:33:40PM +0000, Nick Clifton wrote:
> Patch approved and applied.  Thanks for fixing this!

Thanks for applying!

I have a related question about this fallback. In Chromium, scanning
through all the symbols one by one is a linked list of 975k elements
(which means chasing 975k pointers, with associated cache misses!).
With this patch, we hit that path a _lot_ less, but it's still about
half the CPU time of running perf for my use case (perhaps a bit less,
like 40%).

I looked into trying to making a faster structure, probably reusing the
trie from the other patch I posted (ie., making a separate trie that
maps address-to-symbol instead addition-to-compilation-unit), but it
seems the API is that the client has to send in the list of symbols it
wants to use for fallback? Is that right; so we cannot really create a
persistent structure? (The find_nearest_line function is marked as not
documented yet, so it's not all that easy to know what the intended API
is :-) )

/* Steinar */

Re: [PATCH] Fix return code in _bfd_dwarf2_find_nearest_line().

[Nick Clifton]

Hi Steinar,

> I looked into trying to making a faster structure, probably reusing the
> trie from the other patch I posted (ie., making a separate trie that
> maps address-to-symbol instead addition-to-compilation-unit), but it
> seems the API is that the client has to send in the list of symbols it
> wants to use for fallback?

I do not think that it is for fallback, but rather that it is expected
that all callers of this function will have already decided upon the symbol
table that they want to use, and so it is provided as a parameter.  (A
single binary file can have multiple symbol tables, so there needs to be
some way to select which one to use.  Making the caller perform this choice
is the easiest solution).

  Is that right; so we cannot really create a
> persistent structure? (The find_nearest_line function is marked as not
> documented yet, so it's not all that easy to know what the intended API
> is :-) )


You probably could make a persistent structure.  It is unlikely that the
code will called with different symbol tables under most use cases.  But
you would need to detect if this has happened and ditch/refresh your
storage in that case.

Cheers
   Nick

Re: [PATCH] Fix return code in _bfd_dwarf2_find_nearest_line().

[Steinar H. Gunderson]

On Wed, Mar 23, 2022 at 11:47:25AM +0000, Nick Clifton wrote:
> I do not think that it is for fallback, but rather that it is expected
> that all callers of this function will have already decided upon the symbol
> table that they want to use, and so it is provided as a parameter.  (A
> single binary file can have multiple symbol tables, so there needs to be
> some way to select which one to use.  Making the caller perform this choice
> is the easiest solution).

Wording myself a bit more precisely: It is used for fallback when there
is no debug information that gives precise line/function information; if
there is, we never look at the provided symbol list. I agree it's not a
fallback in the (fairly common) case of e.g. no debug information.

> You probably could make a persistent structure.  It is unlikely that the
> code will called with different symbol tables under most use cases.  But
> you would need to detect if this has happened and ditch/refresh your
> storage in that case.

But how could it be detected? Even if the pointer being sent in is the
same, the contents of the array could be different, and we'd need to
scan through all of it, defeating the purpose?

/* Steinar */

Re: [PATCH] Fix return code in _bfd_dwarf2_find_nearest_line().

[Nick Clifton]

Hi Steinar,

>> You probably could make a persistent structure.  It is unlikely that the

> But how could it be detected? Even if the pointer being sent in is the
> same, the contents of the array could be different, and we'd need to
> scan through all of it, defeating the purpose?

The combination of the 'abfd' pointer and the 'symbols' is probably unique.
But you are right - this cannot be guaranteed.

So the proper way to solve this problem is to create a new function, or
rather group of functions.  One to register a symbol table and then another
the perform scans using that symbol table.  And another function to free
any stored data, of course.

Then callers can choose which API to use.  Fortunately (or maybe
unfortunately) the BFD library has a history of having an unstable API,
so you do not have to worry about versioning.

Cheers
   Nick

Re: [PATCH] Fix return code in _bfd_dwarf2_find_nearest_line().

[Steinar H. Gunderson]

On Wed, Mar 23, 2022 at 12:10:00PM +0000, Nick Clifton wrote:
> So the proper way to solve this problem is to create a new function, or
> rather group of functions.  One to register a symbol table and then another
> the perform scans using that symbol table.  And another function to free
> any stored data, of course.
> 
> Then callers can choose which API to use.  Fortunately (or maybe
> unfortunately) the BFD library has a history of having an unstable API,
> so you do not have to worry about versioning.

Well, I guess since they're also undocumented, and the symbol arrays
usually are provided by bfd, we could claim that this was an expectation
all along... :-) (Or we could track which ones come from bfd, and store
their associated persistent structures. The user isn't supposed to
modify them, I would suppose.)

The right thing to do is probably registration, but I don't think I'm
going down that path. With the patches posted so far (the two that have
been alreayd applied, and the trie for address-to-CU mapping), it's
already fast enough for my use case; an extra 40% would of course always
be nice to have, but not really a game-changer.

/* Steinar */