[gdb/tdep] Handle ldaex and stlex in {thumb,arm}_deal_with_atomic_sequence_raw

The Linaro CI reported a regression [1] in test-case
gdb.base/step-over-syscall.exp due to commit 674d4856730 ("[gdb/testsuite] Fix
gdb.base/step-over-syscall.exp with glibc 2.41").

Investigation shows that it's a progression in the sense that the test-case
fails at a later point than before.

The cause for the test-case failure is that an atomic sequence
ldaex/adds/strex is not skipped over when instruction stepping, leading to a
hang (in the sense of not being able to instruction-step out of the loop
containing the atomic sequence).

The arm target does have support for recognizing atomic sequences, but it
fails because it doesn't recognize the ldaex insn.

Fix this by:
- adding a new function ldaex_p which recognizes ldaex instructions, based
  on information found in opcodes/arm-dis.c, and
- using ldaex_p in thumb_deal_with_atomic_sequence_raw.

I was not able to reproduce the failure in its original setting, but I
was able to do so using a test.c:
...
static void exit (int status) {
  while (1)
    ;
}
void _start (void) {
  int a = 0;
  __atomic_fetch_add (&a, 1, __ATOMIC_ACQUIRE);
  exit (0);
}
...
compiled like this:
...
$ gcc test.c -march=armv8-a -mfloat-abi=soft -nostdlib -static
...
giving this atomic sequence of 32-bit Thumb-2 instructions:
...
   100ce:       e8d3 1fef       ldaex   r1, [r3]
   100d2:       f101 0101       add.w   r1, r1, #1
   100d6:       e843 1200       strex   r2, r1, [r3]
...

Without the fix, after 100 stepi's we're still in _start (and likewise with
10.000 stepi's):
...
$ gdb -q -batch a.out -ex 'display /i $pc' -ex starti -ex "stepi 100"
  ...
0x000100dc in _start ()
1: x/i $pc
=> 0x100dc <_start+26>:	bne.n	0x100ce <_start+12>
...
but with the fix we've managed to progress to exit:
...
$ gdb -q -batch a.out -ex 'display /i $pc' -ex starti -ex "stepi 100"
  ...
0x000100c0 in exit ()
1: x/i $pc
=> 0x100c0 <exit+8>:	b.n	0x100c0 <exit+8>
...

Having addressed the "-mthumb" case, do we need a similar fix for "-marm"?

Adding "-marm" in the compilation line mentioned above gives the following
atomic sequence:
...
   100e4:       e1931e9f        ldaex   r1, [r3]
   100e8:       e2811001        add     r1, r1, #1
   100ec:       e1832f91        strex   r2, r1, [r3]
...
and gdb already recognizes it as such because of this statement:
...
  if ((insn & 0xff9000f0) != 0xe1900090)
    return {};
...

The trouble with this statement is that it requires knowledge of arm
instruction encoding to understand which cases it does and doesn't cover.

Note that the corresponding comment only mentions ldrex:
...
  /* Assume all atomic sequences start with a ldrex{,b,h,d} instruction. ... */
...
but evidently at least some forms of ldaex are also detected.

So, also use ldaex_p in arm_deal_with_atomic_sequence_raw.  This may or may
not be redundant, but at least ldaex_p is explicit and precise about what it
supports.

Likewise for stlex (generated when using __ATOMIC_RELEASE instead of
__ATOMIC_ACQUIRE in the example above).

Tested in arm-linux chroot on aarch64-linux.

Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Co-Authored-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Approved-By: Luis Machado <luis.machado@arm.com>
PR tdep/32796
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32796

[1] https://linaro.atlassian.net/browse/GNU-1541

diff --git a/gdb/arch/arm-get-next-pcs.c b/gdb/arch/arm-get-next-pcs.c
index 8247278b74414a514d721a3bd76d1dd6db2e2aa1..82b077c6b0ad3b7b2e7705b101f367e0beb8cc37 100644 (file)
--- a/gdb/arch/arm-get-next-pcs.c
+++ b/gdb/arch/arm-get-next-pcs.c
@@ -39,10 +39,92 @@ arm_get_next_pcs_ctor (struct arm_get_next_pcs *self,
   self->regcache = regcache;
 }
 
-/* Checks for an atomic sequence of instructions beginning with a LDREX{,B,H,D}
-   instruction and ending with a STREX{,B,H,D} instruction.  If such a sequence
-   is found, attempt to step through it.  The end of the sequence address is
-   added to the next_pcs list.  */
+/* Return true if INSN matches one of the <value,mask> pairs in MATCHES.  */
+
+static bool
+insn_matches (uint32_t insn, gdb::array_view<const uint32_t> matches)
+{
+  gdb_assert (matches.size () % 2 == 0);
+
+  for (int i = 0; i < matches.size (); i += 2)
+    {
+      uint32_t value = matches[i];
+      uint32_t mask = matches[i + 1];
+      if ((insn & mask) == value)
+       return true;
+    }
+
+  return false;
+}
+
+/* Return true if INSN is an ldaex arm insn.  */
+
+static bool
+ldaex_arm_p (uint32_t insn)
+{
+  /* Copied from arm_opcodes in opcodes/arm-dis.c.  */
+  const uint32_t matches_ldaex_arm[] = {
+    0x01900e9f, 0x0ff00fff, /* ldaex.  */
+    0x01b00e9f, 0x0ff00fff, /* ldaexd.  */
+    0x01d00e9f, 0x0ff00fff, /* ldaexb.  */
+    0x01f00e9f, 0x0ff00fff  /* ldaexh.  */
+  };
+
+  return insn_matches (insn, matches_ldaex_arm);
+}
+
+/* Return true if INSN is an ldaex thumb32 insn.  */
+
+static bool
+ldaex_thumb32_p (uint32_t insn)
+{
+  /* Copied from thumb32_opcodes in opcodes/arm-dis.c.  */
+  const uint32_t matches_ldaex_thumb32[] = {
+    0xe8d00fcf, 0xfff00fff, /* ldaexb.  */
+    0xe8d00fdf, 0xfff00fff, /* ldaexh.  */
+    0xe8d00fef, 0xfff00fff, /* ldaex.  */
+    0xe8d000ff, 0xfff000ff  /* ldaexd.  */
+  };
+
+  return insn_matches (insn, matches_ldaex_thumb32);
+}
+
+/* Return true if INSN is an stlex arm insn.  */
+
+static bool
+stlex_arm_p (uint32_t insn)
+{
+  /* Copied from arm_opcodes in opcodes/arm-dis.c.  */
+  const uint32_t matches_stlex_arm[] = {
+    0x01800e90, 0x0ff00ff0, /* stlex.  */
+    0x01a00e90, 0x0ff00ff0, /* stlexd.  */
+    0x01c00e90, 0x0ff00ff0, /* stlexb.  */
+    0x01e00e90, 0x0ff00ff0, /* stlexh.  */
+  };
+
+  return insn_matches (insn, matches_stlex_arm);
+}
+
+/* Return true if INSN is an stlex thumb32 insn.  */
+
+static bool
+stlex_thumb32_p (uint32_t insn)
+{
+  /* Copied from thumb32_opcodes in opcodes/arm-dis.c.  */
+  const uint32_t matches_stlex_thumb32[] = {
+    0xe8c00fc0, 0xfff00ff0, /* stlexb.  */
+    0xe8c00fd0, 0xfff00ff0, /* stlexh.  */
+    0xe8c00fe0, 0xfff00ff0, /* stlex.  */
+    0xe8c000f0, 0xfff000f0, /* stlexd.  */
+  };
+
+  return insn_matches (insn, matches_stlex_thumb32);
+}
+
+/* Checks for an atomic sequence of instructions beginning with an
+   LD[AR]EX{,B,H,D} instruction and ending with a ST[LR]EX{,B,H,D} instruction.
+   If such a sequence is found, attempt to step through it.  The end of the
+   sequence address is added to the next_pcs list.  */
 
 static std::vector<CORE_ADDR>
 thumb_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self)
@@ -64,18 +146,22 @@ thumb_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self)
   if (itstate & 0x0f)
     return {};
 
-  /* Assume all atomic sequences start with a ldrex{,b,h,d} instruction.  */
   insn1 = self->ops->read_mem_uint (loc, 2, byte_order_for_code);
-
   loc += 2;
+
   if (thumb_insn_size (insn1) != 4)
     return {};
 
   insn2 = self->ops->read_mem_uint (loc, 2, byte_order_for_code);
-
   loc += 2;
+
+  uint32_t insn = (uint32_t)insn2 | ((uint32_t)insn1 << 16);
+
+  /* Assume all atomic sequences start with an ld[ar]ex{,b,h,d}
+     instruction.  */
   if (!((insn1 & 0xfff0) == 0xe850
-       || ((insn1 & 0xfff0) == 0xe8d0 && (insn2 & 0x00c0) == 0x0040)))
+       || ((insn1 & 0xfff0) == 0xe8d0 && (insn2 & 0x00c0) == 0x0040)
+       || ldaex_thumb32_p (insn)))
     return {};
 
   /* Assume that no atomic sequence is longer than "atomic_sequence_length"
@@ -110,9 +196,10 @@ thumb_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self)
       else
        {
          insn2 = self->ops->read_mem_uint (loc, 2, byte_order_for_code);
-
          loc += 2;
 
+         insn = (uint32_t)insn2 | ((uint32_t)insn1 << 16);
+
          /* Assume that there is at most one conditional branch in the
             atomic sequence.  If a conditional branch is found, put a
             breakpoint in its destination address.  */
@@ -147,9 +234,10 @@ thumb_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self)
          else if (thumb2_instruction_changes_pc (insn1, insn2))
            return {};
 
-         /* If we find a strex{,b,h,d}, we're done.  */
+         /* If we find a st[lr]ex{,b,h,d}, we're done.  */
          if ((insn1 & 0xfff0) == 0xe840
-             || ((insn1 & 0xfff0) == 0xe8c0 && (insn2 & 0x00c0) == 0x0040))
+             || ((insn1 & 0xfff0) == 0xe8c0 && (insn2 & 0x00c0) == 0x0040)
+             || stlex_thumb32_p (insn))
            break;
        }
     }
@@ -177,10 +265,10 @@ thumb_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self)
   return next_pcs;
 }
 
-/* Checks for an atomic sequence of instructions beginning with a LDREX{,B,H,D}
-   instruction and ending with a STREX{,B,H,D} instruction.  If such a sequence
-   is found, attempt to step through it.  The end of the sequence address is
-   added to the next_pcs list.  */
+/* Checks for an atomic sequence of instructions beginning with an
+   LD[AR]EX{,B,H,D} instruction and ending with a ST[LR]EX{,B,H,D} instruction.
+   If such a sequence is found, attempt to step through it.  The end of the
+   sequence address is added to the next_pcs list.  */
 
 static std::vector<CORE_ADDR>
 arm_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self)
@@ -195,13 +283,16 @@ arm_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self)
   int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed).  */
   const int atomic_sequence_length = 16; /* Instruction sequence length.  */
 
-  /* Assume all atomic sequences start with a ldrex{,b,h,d} instruction.
-     Note that we do not currently support conditionally executed atomic
-     instructions.  */
   insn = self->ops->read_mem_uint (loc, 4, byte_order_for_code);
-
   loc += 4;
-  if ((insn & 0xff9000f0) != 0xe1900090)
+
+  /* Currently we do not support conditionally executed atomic instructions.  */
+  if (!insn_condition_always_true (insn))
+    return {};
+
+  /* Assume all atomic sequences start with an ld[ar]ex{,b,h,d} instruction.  */
+  if (!((insn & 0x0f9000f0) == 0x01900090
+       || ldaex_arm_p (insn)))
     return {};
 
   /* Assume that no atomic sequence is longer than "atomic_sequence_length"
@@ -231,8 +322,14 @@ arm_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self)
       else if (arm_instruction_changes_pc (insn))
        return {};
 
-      /* If we find a strex{,b,h,d}, we're done.  */
-      if ((insn & 0xff9000f0) == 0xe1800090)
+      /* Currently we do not support conditionally executed atomic
+        instructions.  */
+      if (!insn_condition_always_true (insn))
+       return {};
+
+      /* If we find a st[lr]ex{,b,h,d}, we're done.  */
+      if ((insn & 0x0f9000f0) == 0x01800090
+         || stlex_arm_p (insn))
        break;
     }
 

diff --git a/gdb/arch/arm.c b/gdb/arch/arm.c
index e85a1e3f327f1e435b16265ecce05d7ef7d3dac5..1843dbee61285ea52f8633f1406423fa57fda8fe 100644 (file)
--- a/gdb/arch/arm.c
+++ b/gdb/arch/arm.c
@@ -46,7 +46,7 @@ thumb_insn_size (unsigned short inst1)
 int
 condition_true (unsigned long cond, unsigned long status_reg)
 {
-  if (cond == INST_AL || cond == INST_NV)
+  if (condition_always_true (cond))
     return 1;
 
   switch (cond)

diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h
index 5f7aa85a3c1c7d2ea6b3d456e56241a019d5acfa..d9c750329a48799f9ae231279eda162280ca8d4c 100644 (file)
--- a/gdb/arch/arm.h
+++ b/gdb/arch/arm.h
@@ -194,6 +194,23 @@ struct reg_buffer_common;
    first halfword is INST1.  */
 int thumb_insn_size (unsigned short inst1);
 
+/* Returns true if COND always evaluates to true.  */
+
+static inline bool
+condition_always_true (unsigned long cond)
+{
+  return cond == INST_AL || cond == INST_NV;
+}
+
+/* Returns true if cond of INSN always evaluates to true.  */
+
+static inline bool
+insn_condition_always_true (uint32_t insn)
+{
+  unsigned long cond = bits (insn, 28, 31);
+  return condition_always_true (cond);
+}
+
 /* Returns true if the condition evaluates to true.  */
 int condition_true (unsigned long cond, unsigned long status_reg);