Don't drop static function bp locations w/o debug info
authorPedro Alves <pedro@palves.net>
Sun, 13 Sep 2020 12:34:10 +0000 (13:34 +0100)
committerPedro Alves <palves@redhat.com>
Sun, 13 Sep 2020 12:34:10 +0000 (13:34 +0100)
commit77f2120b200be6cabbf6f610942fc1173a8df6d3
treee32397733089d0f520c3696e8978d3432f9077d0
parent1f656a652ea8d489b5aa5b7f55c535fc9e6b0b29
Don't drop static function bp locations w/o debug info

Currently, with a program built from these sources:

 $ cat extern.c
 void foo () {}
 $ cat static.c
 static void foo () {}
 $ cat main.c
 int main () { return 0; }

... if you set a breakpoint on "foo", like:

 (gdb) break foo

.. when there's debug info, GDB creates a breakpoint with two
locations, one for each of the external and static functions.

But, when there's no debug info, GDB creates a breakpoint with a
single location, for the external foo.  Vis:

 $ gcc extern.c static.c main.c -o classify.nodebug
 $ gcc extern.c static.c main.c -o classify.debug -g

 $ gdb classify.nodebug
 Reading symbols from classify.nodebug...
 (No debugging symbols found in classify.nodebug)
 (gdb) b foo
 Breakpoint 1 at 0x40048b
 (gdb) info breakpoints
 Num     Type           Disp Enb Address            What
 1       breakpoint     keep y   0x000000000040048b <foo+4>
 (gdb)

 $ gdb classify.debug
 Reading symbols from classify.debug...
 (gdb) b foo
 Breakpoint 1 at 0x40048b: foo. (2 locations)
 (gdb) info breakpoints
 Num     Type           Disp Enb Address            What
 1       breakpoint     keep y   <MULTIPLE>
 1.1                         y   0x000000000040048b in foo at extern.c:1
 1.2                         y   0x0000000000400492 in foo at static.c:1

GDB drops the static function is search_minsyms_for_name, where at the
very end of that function we pick only the locations with highest
classification, according to classify_type.

The classify_type logic was introduced here:

  https://sourceware.org/pipermail/gdb-patches/2011-December/087864.html

which said:

 "Previously, linespec was trying to filter out minsyms as they were
  seen.  However, this isn't faithful to gdb's historical approach,
  which is to priority-order minsyms; see lookup_minimal_symbol."

lookup_minimal_symbol's intro says, in the .c file:

/* Look through all the current minimal symbol tables and find the
   first minimal symbol that matches NAME.  If OBJF is non-NULL, limit
   the search to that objfile.  If SFILE is non-NULL, the only file-scope
   symbols considered will be from that source file (global symbols are
   still preferred).  Returns a pointer to the minimal symbol that
   matches, or NULL if no match is found.

   Note:  One instance where there may be duplicate minimal symbols with
   the same name is when the symbol tables for a shared library and the
   symbol tables for an executable contain global symbols with the same
   names (the dynamic linker deals with the duplication).

   It's also possible to have minimal symbols with different mangled
   names, but identical demangled names.  For example, the GNU C++ v3
   ABI requires the generation of two (or perhaps three) copies of
   constructor functions --- "in-charge", "not-in-charge", and
   "allocate" copies; destructors may be duplicated as well.
   Obviously, there must be distinct mangled names for each of these,
   but the demangled names are all the same: S::S or S::~S.  */

struct bound_minimal_symbol
lookup_minimal_symbol (const char *name, const char *sfile,
       struct objfile *objf)
{

If you look inside this function, you'll see:

  /* External symbols are best.  */
...
  /* File-local symbols are next best.  */
...
  /* Symbols for shared library trampolines are next best.  */
...

While this logic is good when you're looking for the single "best"
symbol by name, I question it for linespecs, since we want to set
breakpoints in all the multiple locations that match.  I see no point
in hidding static functions.

Now, for breakpoints, it does make sense to filter out PLT/trampoline
symbols if we find the actual global matching function symbol.
Otherwise, if we did no filtering (i.e., just removed the
classify_type logic), you would end up with e.g.:

 (gdb) b printf
 Breakpoint 1 at 0x413a60 (2 locations)
 (top-gdb) info breakpoints
 Num     Type           Disp Enb Address            What
 1      breakpoint     keep y   <MULTIPLE>
 1.1                         y   0x0000000000413a60 <printf@plt>
 1.2                         y   0x00007ffff4653640 in __printf at printf.c:28

instead of this (which is what we get currently) before the shared
library is loaded (a location set in the PLT):

 (gdb) b printf
 Breakpoint 1 at 0x413a60
 Num     Type           Disp Enb Address            What
 1       breakpoint     keep y   0x0000000000413a60 <printf@plt>

and this after the library is loaded (only one location, no breakpoint
in the PLT):

 (gdb) b printf
 Breakpoint 1 at 0x7ffff4653640: file printf.c, line 28.
 (gdb) info breakpoints
 Num     Type           Disp Enb Address            What
 1       breakpoint     keep y   0x00007ffff4653640 in __printf at printf.c:28

This patch fixes the missing breakpoint locations issue by replacing
the classify_type logic in linespec.c with a different logic.
Instead, discard a trampoline symbol if we also found a
global/external symbol with the same name.  The patch adds a couple of
testcases testing locations in external vs static functions vs
trampolines/PLTs.

We now get:

For the msym-bp.exp testcase (extern vs static), without debug info:

 (gdb) info breakpoints
 Num     Type           Disp Enb Address            What
 1       breakpoint     keep y   <MULTIPLE>
 1.1                         y   0x000000000040048b <foo+4>          ### missing before patch
 1.2                         y   0x000000000040049d <foo+4>

For the msym-bp.exp testcase (extern vs static), with debug info:

 (gdb) info breakpoints
 Num     Type           Disp Enb Address            What
 1       breakpoint     keep y   <MULTIPLE>
 1.1                         y   0x000000000040048b in foo at src/gdb/testsuite/gdb.base/msym-bp.c:21
 1.2                         y   0x000000000040049d in foo at src/gdb/testsuite/gdb.base/msym-bp-2.c:21

For the msym-bp-shl.exp testcase (static vs plt), without debug info, before running to main:

 (gdb) info breakpoints
 Num     Type           Disp Enb Address            What
 1       breakpoint     keep y   <MULTIPLE>
 1.1                         y   0x00000000004004e0 <foo@plt>        ### missing before patch
 1.2                         y   0x00000000004005db <foo+4>

For the msym-bp-shl.exp testcase (static vs plt), without debug info, after running to main:

 (gdb) info breakpoints
 Num     Type           Disp Enb Address            What
 1       breakpoint     keep y   <MULTIPLE>
 1.1                         y   0x00000000004005db <foo+4>          ### missing before patch
 1.2                         y   0x00007ffff7bd65de <foo+4>

For the msym-bp-shl.exp testcase (static vs plt), with debug info, before running to main:

 (gdb) info breakpoints
 Num     Type           Disp Enb Address            What
 1       breakpoint     keep y   <MULTIPLE>
 1.1                         y   0x00000000004004e0 <foo@plt>        ### missing before patch
 1.2                         y   0x00000000004005db in foo at src/gdb/testsuite/gdb.base/msym-bp-shl-main-2.c:21

For the msym-bp-shl.exp testcase (static vs plt), with debug info, after running to main:

 (gdb) info breakpoints
 Num     Type           Disp Enb Address            What
 1       breakpoint     keep y   <MULTIPLE>
 1.1                         y   0x00000000004005db in foo at src/gdb/testsuite/gdb.base/msym-bp-shl-main-2.c:21
 1.2                         y   0x00007ffff7bd65de in foo at src/gdb/testsuite/gdb.base/msym-bp-shl-lib.c:21

gdb/ChangeLog:

* linespec.c (classify_mtype, compare_msyms): Delete.
(search_minsyms_for_name): Remove classification logic.  Instead
filter out trampoline symbols if we also found an external
function of the same name.

gdb/testsuite/ChangeLog:

* gdb.base/msym-bp-2.c: New.
* gdb.base/msym-bp-shl-lib.c: New file.
* gdb.base/msym-bp-shl-main-2.c: New file.
* gdb.base/msym-bp-shl-main.c: New file.
* gdb.base/msym-bp-shl.exp: New file.
* gdb.base/msym-bp.c: New file.
* gdb.base/msym-bp.exp: New file.
gdb/ChangeLog
gdb/linespec.c
gdb/testsuite/ChangeLog
gdb/testsuite/gdb.base/msym-bp-2.c [new file with mode: 0644]
gdb/testsuite/gdb.base/msym-bp-shl-lib.c [new file with mode: 0644]
gdb/testsuite/gdb.base/msym-bp-shl-main-2.c [new file with mode: 0644]
gdb/testsuite/gdb.base/msym-bp-shl-main.c [new file with mode: 0644]
gdb/testsuite/gdb.base/msym-bp-shl.exp [new file with mode: 0644]
gdb/testsuite/gdb.base/msym-bp.c [new file with mode: 0644]
gdb/testsuite/gdb.base/msym-bp.exp [new file with mode: 0644]