[1/2] gdb, linespec: avoid multiple locations with same PC

Message ID 20241018102156.350310-2-klaus.gerlicher@intel.com
State New
Headers
Series some amendments to rejecting inserting breakpoints between functions |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed

Commit Message

Klaus Gerlicher Oct. 18, 2024, 10:21 a.m. UTC
  From: "Gerlicher, Klaus" <klaus.gerlicher@intel.com>

Setting a BP on a line like this would incorrectly yield two BP locations:

01 void two () { {int var = 0;} }

(gdb) break 1
Breakpoint 1 at 0x1164: main.cpp:1. (2 locations)

(gdb) info breakpoints
Num     Type           Disp Enb Address            What
1       breakpoint     keep y   <MULTIPLE>
1.1                         y   0x0000000000001164 in two() at main.cpp:1
1.2                         y   0x0000000000001164 in two() at main.cpp:1

In this case decode_digits_ordinary () returns two SALs, exactly matching the
requested line.  One for the entry PC and one for the prologue end PC.  This was
tested with GCC, CLANG and ICPX.  Subsequent code tries to skip the prologue
on these PCs, which in turn makes them the same.

To fix this, ignore SALs with the same PC and program space when adding to the
list of SALs.

This will then properly set only one location:

(gdb) break 1
Breakpoint 1 at 0x1164: file main.cpp, line 1

(gdb) info breakpoints
Num     Type           Disp Enb Address            What
1       breakpoint     keep y   0x0000000000001164 in two() at main.cpp:1
---
 gdb/linespec.c                          | 6 ++++++
 gdb/testsuite/gdb.linespec/linespec.exp | 6 ++++++
 gdb/testsuite/gdb.linespec/lspec.cc     | 2 ++
 3 files changed, 14 insertions(+)
  

Comments

Alexandra Petlanova Hajkova Oct. 20, 2024, 7:34 p.m. UTC | #1
On Fri, Oct 18, 2024 at 12:22 PM Klaus Gerlicher <klaus.gerlicher@intel.com>
wrote:

> From: "Gerlicher, Klaus" <klaus.gerlicher@intel.com>
>
> Setting a BP on a line like this would incorrectly yield two BP locations:
>
> 01 void two () { {int var = 0;} }
>
> (gdb) break 1
> Breakpoint 1 at 0x1164: main.cpp:1. (2 locations)
>
> (gdb) info breakpoints
> Num     Type           Disp Enb Address            What
> 1       breakpoint     keep y   <MULTIPLE>
> 1.1                         y   0x0000000000001164 in two() at main.cpp:1
> 1.2                         y   0x0000000000001164 in two() at main.cpp:1
>
> In this case decode_digits_ordinary () returns two SALs, exactly matching
> the
> requested line.  One for the entry PC and one for the prologue end PC.
> This was
> tested with GCC, CLANG and ICPX.  Subsequent code tries to skip the
> prologue
> on these PCs, which in turn makes them the same.
>
> To fix this, ignore SALs with the same PC and program space when adding to
> the
> list of SALs.
>
> This will then properly set only one location:
>
> (gdb) break 1
> Breakpoint 1 at 0x1164: file main.cpp, line 1
>
> (gdb) info breakpoints
> Num     Type           Disp Enb Address            What
> 1       breakpoint     keep y   0x0000000000001164 in two() at main.cpp:1
> ---
>  gdb/linespec.c                          | 6 ++++++
>  gdb/testsuite/gdb.linespec/linespec.exp | 6 ++++++
>  gdb/testsuite/gdb.linespec/lspec.cc     | 2 ++
>  3 files changed, 14 insertions(+)
>
>
> Hi Klaus,

it's great you fixed this issue and extended the  linespec test. I can also
confirm this change adds no regressions for
Fedora Rawhide x86_64.

Reviewed-By: Alexandra Petlanova Hajkova <ahajkova@redhat.com>
  

Patch

diff --git a/gdb/linespec.c b/gdb/linespec.c
index d5256261eff..a2723e1c06a 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -1047,6 +1047,12 @@  add_sal_to_sals (struct linespec_state *self,
 		 struct symtab_and_line *sal,
 		 const char *symname, int literal_canonical)
 {
+  /* We don't want two SALs with the same PC from the
+     same program space.  */
+  for (const auto &s : *sals)
+    if (sal->pc == s.pc && sal->pspace == s.pspace)
+     return;
+
   sals->push_back (*sal);
 
   if (self->canonical)
diff --git a/gdb/testsuite/gdb.linespec/linespec.exp b/gdb/testsuite/gdb.linespec/linespec.exp
index 576d788cae0..789f1287b07 100644
--- a/gdb/testsuite/gdb.linespec/linespec.exp
+++ b/gdb/testsuite/gdb.linespec/linespec.exp
@@ -194,6 +194,12 @@  gdb_test "break lspec.h:$line" \
     "Breakpoint \[0-9\]+ at $hex: file .*lspec.h, line $line." \
     "set breakpoint in f1"
 
+# This should only have a single location -- in no_multi_locs.
+set line [gdb_get_line_number no_multi_locs]
+gdb_test "break $line" \
+    "Breakpoint \[0-9\]+ at $hex: file .*$srcfile, line $line." \
+    "set breakpoint at no_multi_locs"
+
 #
 # Multi-inferior tests.
 #
diff --git a/gdb/testsuite/gdb.linespec/lspec.cc b/gdb/testsuite/gdb.linespec/lspec.cc
index bb660fbc79e..ab0a193da89 100644
--- a/gdb/testsuite/gdb.linespec/lspec.cc
+++ b/gdb/testsuite/gdb.linespec/lspec.cc
@@ -13,6 +13,8 @@  int body_elsewhere()
 #include "body.h"
 }
 
+void no_multi_locs () { {int var = 0;} }
+
 int main()
 {
   return dupname(0) + m(0) + n(0) + f1() + f2() + body_elsewhere();