[Bug gdb/31214] New: [gdb, aarch64] FAIL: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 0->5: continue

[Bug gdb/31214] New: [gdb, aarch64] FAIL: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 0->5: continue

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31214

            Bug ID: 31214
           Summary: [gdb, aarch64] FAIL: gdb.base/watch-bitfields.exp:
                    -location watch against bitfields: q.e: 0->5: continue
           Product: gdb
           Version: 15.1
            Status: NEW
          Severity: normal
          Priority: P2
         Component: gdb
          Assignee: unassigned at sourceware dot org
          Reporter: vries at gcc dot gnu.org
  Target Milestone: ---

On aarch64-linux, I see:
...
(gdb) PASS: gdb.base/watch-bitfields.exp: -location watch against bitfields:
q.e: 0->5: print expression before
continue^M
Continuing.^M
^M
Hardware watchpoint 2: -location q.a^M
^M
Old value = 1^M
New value = 0^M
main () at /home/vries/gdb/src/gdb/testsuite/gdb.base/watch-bitfields.c:42^M
42        q.h--;^M
(gdb) FAIL: gdb.base/watch-bitfields.exp: -location watch against bitfields:
q.e: 0->5: continue
...

This may be a duplicate of PR29423.  However, I've applied the v2 tentative
patch for that PR, and it doesn't fix this FAIL, so I thought it'd be worth
filing this separately.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31214] [gdb, aarch64] FAIL: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 0->5: continue

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31214

--- Comment #1 from Tom de Vries <vries at gcc dot gnu.org> ---
Minimal example:
...
$ gdb -q -batch \
  -iex "set trace-commands on" \
  outputs/gdb.base/watch-bitfields/watch-bitfields \
  -ex start \
  -ex "maint set show-debug-regs off" \
  -ex "watch -location q.a" \
  -ex "watch -location q.e" \
  -ex continue \
  -ex continue 
+start
Temporary breakpoint 1 at 0x4101a8: file
/home/vries/gdb/src/gdb/testsuite/gdb.base/watch-bitfields.c, line 33.
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".

Temporary breakpoint 1, main () at
/home/vries/gdb/src/gdb/testsuite/gdb.base/watch-bitfields.c:33
33        q.a = 1;
+maint set show-debug-regs off
+watch -location q.a
Hardware watchpoint 2: -location q.a
+watch -location q.e
Hardware watchpoint 3: -location q.e
+continue

Hardware watchpoint 2: -location q.a

Old value = 0
New value = 1
main () at /home/vries/gdb/src/gdb/testsuite/gdb.base/watch-bitfields.c:34
34        q.b = 2;
+continue

Hardware watchpoint 2: -location q.a

Old value = 1
New value = 0
main () at /home/vries/gdb/src/gdb/testsuite/gdb.base/watch-bitfields.c:42
42        q.h--;
...

The code contains:
...
  q.a = 1;
  ...
  q.e = 5;
...

There are two continues, and first one is supposed to trigger the watchpoint on
q.a, and the second one is supposed to trigger the watchpoint on q.e.

The problem is that the watchpoint on q.e doesn't trigger.

The layout of q is:
...
(gdb) ptype /o q
/* offset      |    size */  type = struct foo {
/*      0: 0   |       8 */    unsigned long a : 1;
/*      0: 1   |       1 */    unsigned char b : 2;
/*      0: 3   |       8 */    unsigned long c : 3;
/* XXX  2-bit hole       */
/*      1: 0   |       1 */    char d : 4;
/*      1: 4   |       4 */    int e : 5;
/*      2: 1   |       1 */    char f : 6;
/*      2: 7   |       4 */    int g : 7;
/*      3: 6   |       8 */    long h : 8;
/* XXX  2-bit padding    */
/* XXX  3-byte padding   */

                               /* total size (bytes):    8 */
                             }
...

Bitfield q.a is just bit 0 of byte 0, and bitfield q.e is bit 4..7 of byte 1
and bit 1 of byte 2.

So, watch q.a should watch byte 0, and watch q.e should watch bytes 1 and 2.

Using "maint set show-debug-regs on" we get:
...
WP2: addr=0x440028 (orig=0x440029), ctrl=0x000000d5, ref.count=1
  ctrl: enabled=1, offset=1, len=2
WP3: addr=0x440028 (orig=0x440028), ctrl=0x00000035, ref.count=1
  ctrl: enabled=1, offset=0, len=1
...
which matches that.

When writing to q.a, a hw watchpoint trap happens, and
aarch64_stopped_data_address is called with trap_addr == 0x440028, which is
mapped to WP3, and so the stopped data address (*addr_p) is reported as
0x440028, which is then mapped to gdb's q.a watchpoint.  Since q.a has changed
value, it's reported.

When writing to q.e, the same happens: a hw watchpoint trap happens, and
aarch64_stopped_data_address is called with trap_addr == 0x440028, which is
mapped to WP3, and so the stopped data address (*addr_p) is reported as
0x440028, which again is mapped to gdb's q.a watchpoint.  Since q.a hasn't
changed, it's ignored.

The problem is that aarch64_stopped_data_address cannot distinguish between WP2
and WP3.  The fact that WP3 is chosen in both cases, is because it's checked
first.

Put differently, aarch64_stopped_data_address should report back 0x440028 and
0x440029 in both cases, and the caller should figure out which of the two is
actually triggered.  Alternatively, it could report back a range
[0x440028-0x44002a].

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31214] [gdb, aarch64] FAIL: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 0->5: continue

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31214

--- Comment #2 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to Tom de Vries from comment #1)
> Put differently, aarch64_stopped_data_address should report back 0x440028
> and 0x440029 in both cases, and the caller should figure out which of the
> two is actually triggered.  Alternatively, it could report back a range
> [0x440028-0x44002a].

This tentative patch addresses it slightly differently.  It reports back no
address:
...
diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 5b4e3c2bde1..624884cd375 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -972,9 +972,7 @@ aarch64_linux_nat_target::stopped_data_address (CORE_ADDR
*addr_p)
 bool
 aarch64_linux_nat_target::stopped_by_watchpoint ()
 {
-  CORE_ADDR addr;
-
-  return stopped_data_address (&addr);
+  return stopped_data_address (nullptr);
 }

 /* Implement the "can_do_single_step" target_ops method.  */
diff --git a/gdb/aarch64-nat.c b/gdb/aarch64-nat.c
index ee8c5a1e21d..2cc0a0b707c 100644
--- a/gdb/aarch64-nat.c
+++ b/gdb/aarch64-nat.c
@@ -232,6 +232,7 @@ aarch64_stopped_data_address (const struct
aarch64_debug_reg_state *state,
                              CORE_ADDR addr_trap, CORE_ADDR *addr_p)
 {
   int i;
+  bool found = false;

   for (i = aarch64_num_wp_regs - 1; i >= 0; --i)
     {
@@ -265,12 +266,19 @@ aarch64_stopped_data_address (const struct
aarch64_debug_reg_state *state,
             range.  ADDR_WATCH <= ADDR_TRAP < ADDR_ORIG is a false
             positive on kernels older than 4.10.  See PR
             external/20207.  */
-         *addr_p = addr_orig;
-         return true;
+
+         if (addr_p != nullptr)
+           if (found && *addr_p != addr_orig)
+             return false;
+
+         found = true;
+
+         if (addr_p != nullptr)
+           *addr_p = addr_orig;
        }
     }

-  return false;
+  return found;
 }

 /* Define AArch64 maintenance commands.  */

...

So, if called from stopped_by_watchpoint (addr_p == nullptr), if there is any
hit, it returns true.

Otherwise (addr_p != nullptr) if there are multiple hits with conflicting
addresses, it return false.

Fixes the test-case.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31214] [gdb, aarch64] FAIL: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 0->5: continue

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31214

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |luis.machado at arm dot com

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31214] [gdb, aarch64] FAIL: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 0->5: continue

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31214

--- Comment #3 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to Tom de Vries from comment #2)
> Fixes the test-case.

But regresses here:
...
FAIL: gdb.base/watchpoint-unaligned.exp: wpcount(4)
...

Indeed, I suppose this approach regresses read watch points.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31214] [gdb, aarch64] FAIL: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 0->5: continue

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31214

--- Comment #4 from Tom de Vries <vries at gcc dot gnu.org> ---
https://sourceware.org/pipermail/gdb-patches/2024-February/206707.html

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/31214] [gdb, aarch64] FAIL: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 0->5: continue

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31214

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|gdb                         |tdep

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/31214] [gdb, aarch64] FAIL: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 0->5: continue

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31214

--- Comment #5 from Sourceware Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tom de Vries <vries@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=cf16ab724a41e4cbaf723b5633d4e7b29f61372b

commit cf16ab724a41e4cbaf723b5633d4e7b29f61372b
Author: Tom de Vries <tdevries@suse.de>
Date:   Tue Mar 12 17:08:18 2024 +0100

    [gdb/tdep] Fix gdb.base/watch-bitfields.exp on aarch64

    On aarch64-linux, with test-case gdb.base/watch-bitfields.exp I run into:
    ...
    (gdb) continue^M
    Continuing.^M
    ^M
    Hardware watchpoint 2: -location q.a^M
    ^M
    Old value = 1^M
    New value = 0^M
    main () at watch-bitfields.c:42^M
    42        q.h--;^M
    (gdb) FAIL: $exp: -location watch against bitfields: q.e: 0->5: continue
    ...

    In a minimal form, if we step past line 37 which sets q.e, and we have a
    watchpoint set on q.e, it triggers:
    ...
    $ gdb -q -batch watch-bitfields -ex "b 37" -ex run -ex "watch q.e" -ex step
    Breakpoint 1 at 0x410204: file watch-bitfields.c, line 37.

    Breakpoint 1, main () at watch-bitfields.c:37
    37        q.e = 5;
    Hardware watchpoint 2: q.e

    Hardware watchpoint 2: q.e

    Old value = 0
    New value = 5
    main () at /home/vries/gdb/src/gdb/testsuite/gdb.base/watch-bitfields.c:38
    38        q.f = 6;
    ...

    However, if we set in addition a watchpoint on q.a, the watchpoint on q.e
    doesn't trigger.

    How does this happen?

    Bitfield q.a is just bit 0 of byte 0, and bitfield q.e is bit 4..7 of byte
1
    and bit 1 of byte 2.  So, watch q.a should watch byte 0, and watch q.e
should
    watch bytes 1 and 2.

    Using "maint set show-debug-regs on" (and some more detailed debug prints)
we
    get:
    ...
    WP2: addr=0x440028 (orig=0x440029), ctrl=0x000000d5, ref.count=1
      ctrl: enabled=1, offset=1, len=2
    WP3: addr=0x440028 (orig=0x440028), ctrl=0x00000035, ref.count=1
      ctrl: enabled=1, offset=0, len=1
    ...
    which matches that.

    When executing line 37, a hardware watchpoint trap triggers and we hit
    aarch64_stopped_data_address with addr_trap == 0x440028:
    ...
    (gdb) p /x addr_trap
    $1 = 0x440028
    ....
    and since the loop in aarch64_stopped_data_address walks backward, we check
    WP3 first, which matches, and consequently target_stopped_by_watchpoint
    returns true in watchpoints_triggered.

    Likewise for target_stopped_data_address, which also returns addr ==
0x440028.
    Watchpoints_triggered matches watchpoint q.a to that address, and sets
    watch_triggered_yes.

    However, subsequently the value of q.a is checked, and it's the same value
as
    before (becase the insn in line 37 didn't change q.a), so the watchpoint
    hardware trap is not reported to the user.

    The problem originates from that fact that aarch64_stopped_data_address
picked
    WP3 instead of WP2.

    There's something we can do about this.  In the example above, both
    target_stopped_by_watchpoint and target_stopped_data_address returned true.
    Instead we can return true in target_stopped_by_watchpoint but false in
    target_stopped_data_address.  This lets watchpoints_triggered known that a
    watchpoint was triggered, but we don't know where, and both watchpoints
    get set to watch_triggered_unknown.

    Subsequently, the values of both q.a and q.e are checked, and since q.e is
not
    the same value as before, the watchpoint hardware trap is reported to the
user.

    Note that this works well for regular (write) watchpoints (watch command),
but
    not for read watchpoints (rwatch command), because for those no value is
    checked.  Likewise for access watchpoints (awatch command).

    So, fix this by:
    - passing a nullptr in aarch64_fbsd_nat_target::stopped_by_watchpoint and
      aarch64_linux_nat_target::stopped_by_watchpoint to make clear we're not
      interested in the stop address,
    - introducing a two-phase approach in aarch64_stopped_data_address, where:
      - phase one handles access and read watchpoints, as before, and
      - phase two handles write watchpoints, where multiple matches cause:
        - return true if addr_p == null, and
        - return false if addr_p != null.

    Tested on aarch64-linux.

    Approved-By: Luis Machado <luis.machado@arm.com>

    PR tdep/31214
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31214

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/31214] [gdb, aarch64] FAIL: gdb.base/watch-bitfields.exp: -location watch against bitfields: q.e: 0->5: continue

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31214

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED
   Target Milestone|---                         |15.1

--- Comment #6 from Tom de Vries <vries at gcc dot gnu.org> ---
Fixed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.