public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Shahab Vahedi <shahab.vahedi@gmail.com>
To: gdb-patches@sourceware.org
Cc: Shahab Vahedi <shahab.vahedi@gmail.com>,
	Shahab Vahedi <shahab@synopsys.com>,
	Simon Marchi <simon.marchi@polymtl.ca>,
	Tom Tromey <tom@tromey.com>,
	Francois Bedard <fbedard@synopsys.com>
Subject: [PUSHED master] gdb: Do not interrupt atomic sequences for ARC
Date: Mon,  8 Feb 2021 13:07:54 +0100	[thread overview]
Message-ID: <20210208120754.15502-1-shahab.vahedi@gmail.com> (raw)
In-Reply-To: <20210125230230.12250-1-shahab.vahedi@gmail.com>

From: Shahab Vahedi <shahab@synopsys.com>

When stepping over thread-lock related codes (in uClibc), the inferior process
gets stuck and never manages to enter the critical section:

------8<-------
 1 size_t fwrite(const void * __restrict ptr, size_t size,
 2               size_t nmemb, register FILE * __restrict stream)
 3 {
 4     size_t retval;
 5     __STDIO_AUTO_THREADLOCK_VAR;
 6
 7 >   __STDIO_AUTO_THREADLOCK(stream);
 8
 9     retval = fwrite_unlocked(ptr, size, nmemb, stream);
10
11     __STDIO_AUTO_THREADUNLOCK(stream);
12
13     return retval;
14 }
------>8-------

Here, we are at line 7.  Using the "next" command leads no where.
However, setting a breakpoint on line 9 and issuing "continue" works.

Looking at the assembly instructions reveals that we're dealing with the
critical section entry code [1] that should never be interrupted, in this
case by the debugger's implicit breakpoints:

------8<-------
  ...
1 add_s   r0,r13,0x38
2 mov_s   r3,1
3 llock   r2,[r0]        <-.
4 brne.nt r2,0,14     --.  |
5 scond   r3,[r0]       |  |
6 bne     -10         --|--'
7 brne_s  r2,0,84     <-'
  ...
------>8-------

Lines 3 until 5 (inclusive) are supposed to be executed atomically.
Therefore, GDB should never (implicitly) insert a breakpoint on lines
4 and 5, else the program will try to acquire the lock again by jumping
back to line 3 and gets stuck in an infinite loop.

The solution is to make GDB aware of these patterns so it inserts
breakpoints after the sequence -- line 6 in this example.

[1]
https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/libc/sysdeps/linux/arc/bits/atomic.h#n46
------8<-------
  ({									\
	__typeof(oldval) prev;						\
									\
	__asm__ __volatile__(						\
	"1:	llock   %0, [%1]	\n"				\
	"	brne    %0, %2, 2f	\n"				\
	"	scond   %3, [%1]	\n"				\
	"	bnz     1b		\n"				\
	"2:				\n"				\
	: "=&r"(prev)							\
	: "r"(mem), "ir"(oldval),					\
	  "r"(newval) /* can't be "ir". scond can't take limm for "b" */\
	: "cc", "memory");						\
									\
	prev;								\
  })
------>8-------
"llock" (Load Locked) loads the 32-bit word pointed by the source
operand.  If the load is completed without any interruption or
exception, the physical address is remembered, in Lock Physical Address
(LPA), and the Lock Flag (LF) is set to 1.  LF is a non-architecturally
visible flag and is cleared whenever an interrupt or exception takes
place.  LF is also cleared (atomically) whenever another process writes
to the LPA.

"scond" (Store Conditional) will write to the destination address if
and only if the LF is set to 1.  When finished, with or without a write,
it atomically copies the LF value to ZF (Zero Flag).

These two instructions together provide the mechanism for entering a
critical section.  The code snippet above comes from uClibc:
-----------------------

v3 (after Tom's remarks[2]):
 handle_atomic_sequence()
  - no need to initialize the std::vector with "{}"
  - fix typo in comments: "conditial" -> "conditional"
  - add braces to the body of "if" condition because of the comment line
 arc_linux_software_single_step()
  - make the performance slightly more efficient by moving a few
    variables after the likely "return" point.

v2 (after Simon's remarks[3]):
- handle_atomic_sequence() gets a copy of an instruction instead of
  a reference.
- handle_atomic_sequence() asserts if the given instruction is an llock.

[2]
https://sourceware.org/pipermail/gdb-patches/2021-February/175805.html

[3]
https://sourceware.org/pipermail/gdb-patches/2021-January/175487.html

gdb/ChangeLog:

	PR tdep/27369
	* arc-linux-tdep.c (handle_atomic_sequence): New.
	(arc_linux_software_single_step): Call handle_atomic_sequence().
---
 gdb/ChangeLog        |  6 ++++
 gdb/arc-linux-tdep.c | 77 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 912ca6b9444..e67668d315c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2021-02-08  Shahab Vahedi  <shahab@synopsys.com>
+
+	PR tdep/27369
+	* arc-linux-tdep.c (handle_atomic_sequence): New.
+	(arc_linux_software_single_step): Call handle_atomic_sequence().
+
 2021-02-08  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* python/py-tui.c (gdbpy_tui_window) <is_valid>: New member
diff --git a/gdb/arc-linux-tdep.c b/gdb/arc-linux-tdep.c
index c9fbd7ddc28..cf18b8d6b03 100644
--- a/gdb/arc-linux-tdep.c
+++ b/gdb/arc-linux-tdep.c
@@ -332,6 +332,78 @@ arc_linux_sw_breakpoint_from_kind (struct gdbarch *gdbarch,
 	  : arc_linux_trap_s_le);
 }
 
+/* Check for an atomic sequence of instructions beginning with an
+   LLOCK instruction and ending with a SCOND instruction.
+
+   These patterns are hand coded in libc's (glibc and uclibc). Take
+   a look at [1] for instance:
+
+   main+14: llock   r2,[r0]
+   main+18: brne.nt r2,0,main+30
+   main+22: scond   r3,[r0]
+   main+26: bne     main+14
+   main+30: mov_s   r0,0
+
+   If such a sequence is found, attempt to step over it.
+   A breakpoint is placed at the end of the sequence.
+
+   This function expects the INSN to be a "llock(d)" instruction.
+
+   [1]
+   https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/libc/ \
+     sysdeps/linux/arc/bits/atomic.h#n46
+   */
+
+static std::vector<CORE_ADDR>
+handle_atomic_sequence (arc_instruction insn, disassemble_info &di)
+{
+  const int atomic_seq_len = 24;    /* Instruction sequence length.  */
+  std::vector<CORE_ADDR> next_pcs;
+
+  /* Sanity check.  */
+  gdb_assert (insn.insn_class == LLOCK);
+
+  /* Data size we are dealing with: LLOCK vs. LLOCKD  */
+  arc_ldst_data_size llock_data_size_mode = insn.data_size_mode;
+  /* Indicator if any conditional branch is found in the sequence.  */
+  bool found_bc = false;
+  /* Becomes true if "LLOCK(D) .. SCOND(D)" sequence is found.  */
+  bool is_pattern_valid = false;
+
+  for (int insn_count = 0; insn_count < atomic_seq_len; ++insn_count)
+    {
+      arc_insn_decode (arc_insn_get_linear_next_pc (insn),
+		       &di, arc_delayed_print_insn, &insn);
+
+      if (insn.insn_class == BRCC)
+        {
+          /* If more than one conditional branch is found, this is not
+             the pattern we are interested in.  */
+          if (found_bc)
+	    break;
+	  found_bc = true;
+	  continue;
+        }
+
+      /* This is almost a happy ending.  */
+      if (insn.insn_class == SCOND)
+        {
+	  /* SCOND should match the LLOCK's data size.  */
+	  if (insn.data_size_mode == llock_data_size_mode)
+	    is_pattern_valid = true;
+	  break;
+        }
+    }
+
+  if (is_pattern_valid)
+    {
+      /* Get next instruction after scond(d).  There is no limm.  */
+      next_pcs.push_back (insn.address + insn.length);
+    }
+
+  return next_pcs;
+}
+
 /* Implement the "software_single_step" gdbarch method.  */
 
 static std::vector<CORE_ADDR>
@@ -345,8 +417,11 @@ arc_linux_software_single_step (struct regcache *regcache)
   struct arc_instruction curr_insn;
   arc_insn_decode (regcache_read_pc (regcache), &di, arc_delayed_print_insn,
 		   &curr_insn);
-  CORE_ADDR next_pc = arc_insn_get_linear_next_pc (curr_insn);
 
+  if (curr_insn.insn_class == LLOCK)
+    return handle_atomic_sequence (curr_insn, di);
+
+  CORE_ADDR next_pc = arc_insn_get_linear_next_pc (curr_insn);
   std::vector<CORE_ADDR> next_pcs;
 
   /* For instructions with delay slots, the fall thru is not the
-- 
2.30.0


  parent reply	other threads:[~2021-02-08 12:07 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-25 23:02 [PATCH] " Shahab Vahedi
2021-01-26  3:34 ` Simon Marchi
2021-01-26 10:11   ` Shahab Vahedi
2021-01-26 16:02     ` Simon Marchi
2021-02-04 19:27       ` Shahab Vahedi
2021-02-04 19:20 ` [PATCH v2] " Shahab Vahedi
2021-02-05 16:13   ` Tom Tromey
2021-02-08 12:07 ` Shahab Vahedi [this message]
2021-02-08 12:20 ` [PUSHED gdb-10-branch] " Shahab Vahedi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210208120754.15502-1-shahab.vahedi@gmail.com \
    --to=shahab.vahedi@gmail.com \
    --cc=fbedard@synopsys.com \
    --cc=gdb-patches@sourceware.org \
    --cc=shahab@synopsys.com \
    --cc=simon.marchi@polymtl.ca \
    --cc=tom@tromey.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).