From: Tom de Vries Date: Wed, 9 Apr 2025 06:59:42 +0000 (+0200) Subject: [gdb/tdep] Handle ldaex and stlex in {thumb,arm}_deal_with_atomic_sequence_raw X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=5ea1eec52f47fbc9155bd8f057d26429e6881628;p=thirdparty%2Fbinutils-gdb.git [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 : b.n 0x100c0 ... 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 Co-Authored-By: Thiago Jung Bauermann Approved-By: Luis Machado 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 8247278b744..82b077c6b0a 100644 --- 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 pairs in MATCHES. */ + +static bool +insn_matches (uint32_t insn, gdb::array_view 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 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 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 e85a1e3f327..1843dbee612 100644 --- 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 5f7aa85a3c1..d9c750329a4 100644 --- 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);