public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH v2 0/5] [gdb/symtab] Assume epilogue unwind info is valid unless gcc < 4.5.0
@ 2023-02-13 14:23 Tom de Vries
  2023-02-13 14:23 ` [PATCH v2 1/5] [gdb/symtab] Factor out compunit_epilogue_unwind_valid Tom de Vries
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Tom de Vries @ 2023-02-13 14:23 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

[ This patch series is a resubmission of this patch (
https://sourceware.org/pipermail/gdb-patches/2023-January/196325.html ).

The problem with the earlier version was that I tested it with a broken
version of test-case gdb.base/unwind-on-each-insn.exp, something that was
fixed by commit d9195131530 ("[gdb/testsuite] Simplify
gdb.base/unwind-on-each-insn.exp.tcl").

Retesting with a fixed test-case revealed the need for the patch
"[gdb/tdep] Add amd64/i386 epilogue override unwinders".

Furthermore, I've added two introductory refactoring patches to make following
changes more compact.

Now that this turned into a patch series, I've also split off
the dwarf2/read.c change into a separate patch. ]

The gcc 4.4.x (and earlier) compilers had the problem that the unwind info in
the epilogue was inaccurate.

In order to work around this in gdb, epilogue unwinders were added with a
higher priority than the dwarf unwinders in the amd64 and i386 targets:
- amd64_epilogue_frame_unwind, and
- i386_epilogue_frame_unwind
see:
- submission emails:
  https://sourceware.org/pipermail/gdb-patches/2009-July/066779.html
  https://sourceware.org/pipermail/gdb-patches/2009-August/067684.html
- gdb commits 872761f485e and 06da04c6105

Subsequently, the epilogue unwind info problem got fixed in gcc 4.5.0, see:
- submission email
  https://gcc.gnu.org/pipermail/gcc-patches/2009-May/261377.html
- gcc commit cd9c1ca866b
- release notes gcc 4.5.0 ( https://gcc.gnu.org/gcc-4.5/changes.html ):
  GCC now generates unwind info also for epilogues.

However, the epilogue unwinders prevented gdb from taking advantage of the
fixed epilogue unwind info, so the scope of the epilogue unwinders was
limited, bailing out for gcc >= 4.5.0, see:
- submisssion email
  https://sourceware.org/pipermail/gdb-patches/2011-June/083429.html
- gdb commit e0d00bc749e "Disable epilogue unwinders on recent GCCs"

This scope limitation mechanism works well for gcc -g: the producer is
available in the debug info, and we can determine whether we're dealing
with reliable epilogue unwind info or not.

For gcc -g0 though, epilogue unwind information is available in .eh_frame, but
the producer is not availabe to determine whether that information is reliable
or not, and consequently the info is ignored:
- in the case of using gcc <= 4.4.x, that is the ok decision and we're working
  around the gcc problem, but
- in the case of gcc >= 4.5.0, that means we fail to take advantage of fixed
  epilogue unwind info.

Furthermore, let's review the history of what epilogue unwind information is
trusted by gdb:
- initial position: trust all information
- after the epilogue unwinders were added: trust none
- after the scope limitation: only trust gcc >= 4.5.0.

So, while we started out with trusting info from all compilers, we end up
trusting only gcc >= 4.5.0, which seems a bit of an overreach for a workaround
for a problem in the gcc compiler.

Fix these two issues by reversing the burden of proof:
- currently we assume epilogue unwind info is invalid unless we can proof that
  gcc >= 4.5.0.
- instead, assume epilogue unwind info is valid unless we can proof that
  gcc < 4.5.0.

An added benefit of this is that it makes the amd64 and i386 targets more
similar to other targets, which makes comparing behaviour easier.  Note that
some other targets also have an epilogue unwinder, but none of those have a
higher priority than the dwarf unwinders.

Tested on x86_64-linux with gcc 7.5.0, with target boards unix/-m64 and
unix/-m32.

A regression test-case has been added to test the -g0
-fasynchronous-unwind-tables scenario, on amd64 (but not on i386).

PR tdep/30028
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30028
---
 gdb/amd64-tdep.c                              |  4 +-
 gdb/dwarf2/read.c                             | 12 ++++-
 gdb/i386-tdep.c                               |  4 +-
 .../gdb.base/unwind-on-each-insn-amd64-2.exp  | 52 ++++++++++++++++++
 .../gdb.base/unwind-on-each-insn-amd64-2.s    | 54 +++++++++++++++++++
 5 files changed, 123 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.exp
 create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.s

Tom de Vries (5):
  [gdb/symtab] Factor out compunit_epilogue_unwind_valid
  [gdb/tdep] Fix amd64/i386_stack_frame_destroyed_p
  [gdb/tdep] Add amd64/i386 epilogue override unwinders
  [gdb/symtab] Trust epilogue unwind info for unknown producer (-g0
    case)
  [gdb/symtab] Trust epilogue unwind info for unknown or non-gcc
    producer

 gdb/amd64-tdep.c                              | 69 +++++++++++++++---
 gdb/dwarf2/read.c                             |  8 ++-
 gdb/i386-tdep.c                               | 71 +++++++++++++++----
 gdb/symtab.h                                  | 13 ++++
 .../gdb.base/unwind-on-each-insn-amd64-2.exp  | 52 ++++++++++++++
 .../gdb.base/unwind-on-each-insn-amd64-2.s    | 54 ++++++++++++++
 6 files changed, 243 insertions(+), 24 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.exp
 create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.s


base-commit: 97c195191578e9a68bfbb810eea373f5f3efcb7d
-- 
2.35.3


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/5] [gdb/symtab] Factor out compunit_epilogue_unwind_valid
  2023-02-13 14:23 [PATCH v2 0/5] [gdb/symtab] Assume epilogue unwind info is valid unless gcc < 4.5.0 Tom de Vries
@ 2023-02-13 14:23 ` Tom de Vries
  2023-02-14 15:56   ` Tom Tromey
  2023-02-13 14:23 ` [PATCH v2 2/5] [gdb/tdep] Fix amd64/i386_stack_frame_destroyed_p Tom de Vries
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Tom de Vries @ 2023-02-13 14:23 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

Factor out compunit_epilogue_unwind_valid from both
amd64_stack_frame_destroyed_p and i386_stack_frame_destroyed_p.  NFC.

Also add a comment in the new function about the assumption that in absence of
producer information, epilogue unwind info is invalid.
---
 gdb/amd64-tdep.c |  4 +---
 gdb/i386-tdep.c  |  4 +---
 gdb/symtab.h     | 13 +++++++++++++
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 328e001f5bb..d2e683b6fa8 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2902,10 +2902,8 @@ static int
 amd64_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
 {
   gdb_byte insn;
-  struct compunit_symtab *cust;
 
-  cust = find_pc_compunit_symtab (pc);
-  if (cust != NULL && cust->epilogue_unwind_valid ())
+  if (compunit_epilogue_unwind_valid (find_pc_compunit_symtab (pc)))
     return 0;
 
   if (target_read_memory (pc, &insn, 1))
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 8dccae633f7..3fe548d8c68 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -2219,10 +2219,8 @@ static int
 i386_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
 {
   gdb_byte insn;
-  struct compunit_symtab *cust;
 
-  cust = find_pc_compunit_symtab (pc);
-  if (cust != NULL && cust->epilogue_unwind_valid ())
+  if (compunit_epilogue_unwind_valid (find_pc_compunit_symtab (pc)))
     return 0;
 
   if (target_read_memory (pc, &insn, 1))
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 11ff875c6b8..cd6b5f722fd 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1917,6 +1917,19 @@ is_main_symtab_of_compunit_symtab (struct symtab *symtab)
 {
   return symtab == symtab->compunit ()->primary_filetab ();
 }
+
+/* Return true if epilogue unwind info of CUST is valid.  */
+
+static inline bool
+compunit_epilogue_unwind_valid (struct compunit_symtab *cust)
+{
+  /* In absence of producer information, assume epilogue unwind info is
+     invalid.  */
+  if (cust == nullptr)
+    return false;
+
+  return cust->epilogue_unwind_valid ();
+}
 \f
 
 /* The virtual function table is now an array of structures which have the
-- 
2.35.3


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 2/5] [gdb/tdep] Fix amd64/i386_stack_frame_destroyed_p
  2023-02-13 14:23 [PATCH v2 0/5] [gdb/symtab] Assume epilogue unwind info is valid unless gcc < 4.5.0 Tom de Vries
  2023-02-13 14:23 ` [PATCH v2 1/5] [gdb/symtab] Factor out compunit_epilogue_unwind_valid Tom de Vries
@ 2023-02-13 14:23 ` Tom de Vries
  2023-02-13 14:23 ` [PATCH v2 3/5] [gdb/tdep] Add amd64/i386 epilogue override unwinders Tom de Vries
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Tom de Vries @ 2023-02-13 14:23 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

The use of compunit_epilogue_unwind_valid in both amd64_stack_frame_destroyed_p
and i386_stack_frame_destroyed_p is problematic, in the sense that the
functions no longer match their documented behaviour.

Fix this by moving the use of compunit_epilogue_unwind_valid to
amd64_epilogue_frame_sniffer and i386_epilogue_frame_sniffer.  NFC.
---
 gdb/amd64-tdep.c | 19 ++++++++++++-------
 gdb/i386-tdep.c  | 20 ++++++++++++--------
 2 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index d2e683b6fa8..0ec9b23922d 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2903,9 +2903,6 @@ amd64_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
 {
   gdb_byte insn;
 
-  if (compunit_epilogue_unwind_valid (find_pc_compunit_symtab (pc)))
-    return 0;
-
   if (target_read_memory (pc, &insn, 1))
     return 0;   /* Can't read memory at pc.  */
 
@@ -2920,11 +2917,19 @@ amd64_epilogue_frame_sniffer (const struct frame_unwind *self,
 			      frame_info_ptr this_frame,
 			      void **this_prologue_cache)
 {
-  if (frame_relative_level (this_frame) == 0)
-    return amd64_stack_frame_destroyed_p (get_frame_arch (this_frame),
-					  get_frame_pc (this_frame));
-  else
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
+  CORE_ADDR pc = get_frame_pc (this_frame);
+
+  if (frame_relative_level (this_frame) != 0)
+    /* We're not in the inner frame, so assume we're not in an epilogue.  */
     return 0;
+
+  if (compunit_epilogue_unwind_valid (find_pc_compunit_symtab (pc)))
+    /* Don't override the symtab unwinders.  */
+    return 0;
+
+  /* Check whether we're in an epilogue.  */
+  return amd64_stack_frame_destroyed_p (gdbarch, pc);
 }
 
 static struct amd64_frame_cache *
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 3fe548d8c68..5e797d098e8 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -2219,10 +2219,6 @@ static int
 i386_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
 {
   gdb_byte insn;
-
-  if (compunit_epilogue_unwind_valid (find_pc_compunit_symtab (pc)))
-    return 0;
-
   if (target_read_memory (pc, &insn, 1))
     return 0;	/* Can't read memory at pc.  */
 
@@ -2237,11 +2233,19 @@ i386_epilogue_frame_sniffer (const struct frame_unwind *self,
 			     frame_info_ptr this_frame,
 			     void **this_prologue_cache)
 {
-  if (frame_relative_level (this_frame) == 0)
-    return i386_stack_frame_destroyed_p (get_frame_arch (this_frame),
-					 get_frame_pc (this_frame));
-  else
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
+  CORE_ADDR pc = get_frame_pc (this_frame);
+
+  if (frame_relative_level (this_frame) != 0)
+    /* We're not in the inner frame, so assume we're not in an epilogue.  */
+    return 0;
+
+  if (compunit_epilogue_unwind_valid (find_pc_compunit_symtab (pc)))
+    /* Don't override the symtab unwinders.  */
     return 0;
+
+  /* Check whether we're in an epilogue.  */
+  return i386_stack_frame_destroyed_p (gdbarch, pc);
 }
 
 static struct i386_frame_cache *
-- 
2.35.3


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 3/5] [gdb/tdep] Add amd64/i386 epilogue override unwinders
  2023-02-13 14:23 [PATCH v2 0/5] [gdb/symtab] Assume epilogue unwind info is valid unless gcc < 4.5.0 Tom de Vries
  2023-02-13 14:23 ` [PATCH v2 1/5] [gdb/symtab] Factor out compunit_epilogue_unwind_valid Tom de Vries
  2023-02-13 14:23 ` [PATCH v2 2/5] [gdb/tdep] Fix amd64/i386_stack_frame_destroyed_p Tom de Vries
@ 2023-02-13 14:23 ` Tom de Vries
  2023-02-13 14:23 ` [PATCH v2 4/5] [gdb/symtab] Trust epilogue unwind info for unknown producer (-g0 case) Tom de Vries
  2023-02-13 14:23 ` [PATCH v2 5/5] [gdb/symtab] Trust epilogue unwind info for unknown or non-gcc producer Tom de Vries
  4 siblings, 0 replies; 9+ messages in thread
From: Tom de Vries @ 2023-02-13 14:23 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

For amd64 the current frame-unwinders are:
...
$ gdb -q -batch -ex "set arch i386:x86-64" -ex "maint info frame-unwinders"
The target architecture is set to "i386:x86-64".
dummy                   DUMMY_FRAME
dwarf2 tailcall         TAILCALL_FRAME
inline                  INLINE_FRAME
python                  NORMAL_FRAME
amd64 epilogue          NORMAL_FRAME
dwarf2                  NORMAL_FRAME
dwarf2 signal           SIGTRAMP_FRAME
amd64 sigtramp          SIGTRAMP_FRAME
amd64 prologue          NORMAL_FRAME
...

For a -g0 -fasynchronous-unwind-tables exec (without .debug_info but with
.eh_frame section), we'd like to start using the dwarf2 unwinder instead of
the "amd64 epilogue" unwinder, by returning true in
compunit_epilogue_unwind_valid for cust == nullptr.

But we'd run into the following problem for a -g0
-fno-asynchronous-unwind-tables (without .debug_info and .eh_frame section)
exec:
- the "amd64 epilogue" unwinder would not run
  (because compunit_epilogue_unwind_valid () == true)
- the dwarf2 unwinder would also not run
  (because there's no .eh_frame info).

Fix this by:
- renaming the "amd64 epilogue" unwinder to "amd64 epilogue override", and
- adding a fallback "amd64 epilogue" after the dwarf unwinders,
while making sure that only one of the two is active.  Likewise for i386.  NFC.

For amd64, this results in this change:
...
 $ gdb -q -batch -ex "set arch i386:x86-64" -ex "maint info frame-unwinders"
 The target architecture is set to "i386:x86-64".
 dummy                   DUMMY_FRAME
 dwarf2 tailcall         TAILCALL_FRAME
 inline                  INLINE_FRAME
 python                  NORMAL_FRAME
-amd64 epilogue          NORMAL_FRAME
+amd64 epilogue override NORMAL_FRAME
 dwarf2                  NORMAL_FRAME
 dwarf2 signal           SIGTRAMP_FRAME
+amd64 epilogue          NORMAL_FRAME
 amd64 sigtramp          SIGTRAMP_FRAME
 amd64 prologue          NORMAL_FRAME
...

And for i386:
...
 $ gdb -q -batch -ex "set arch i386" -ex "maint info frame-unwinders"
 The target architecture is set to "i386".
 dummy                   DUMMY_FRAME
 dwarf2 tailcall         TAILCALL_FRAME
 iline                  INLINE_FRAME
-i386 epilogue           NORMAL_FRAME
+i386 epilogue override  NORMAL_FRAME
 dwarf2                  NORMAL_FRAME
 dwarf2 signal           SIGTRAMP_FRAME
+i386 epilogue           NORMAL_FRAME
 i386 stack tramp        NORMAL_FRAME
 i386 sigtramp           SIGTRAMP_FRAME
 i386 prologue           NORMAL_FRAME
...
---
 gdb/amd64-tdep.c | 58 +++++++++++++++++++++++++++++++++++++++++------
 gdb/i386-tdep.c  | 59 ++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 103 insertions(+), 14 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 0ec9b23922d..c0c62bdd696 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2913,9 +2913,9 @@ amd64_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
 }
 
 static int
-amd64_epilogue_frame_sniffer (const struct frame_unwind *self,
-			      frame_info_ptr this_frame,
-			      void **this_prologue_cache)
+amd64_epilogue_frame_sniffer_1 (const struct frame_unwind *self,
+				frame_info_ptr this_frame,
+				void **this_prologue_cache, bool override_p)
 {
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
   CORE_ADDR pc = get_frame_pc (this_frame);
@@ -2924,14 +2924,45 @@ amd64_epilogue_frame_sniffer (const struct frame_unwind *self,
     /* We're not in the inner frame, so assume we're not in an epilogue.  */
     return 0;
 
-  if (compunit_epilogue_unwind_valid (find_pc_compunit_symtab (pc)))
-    /* Don't override the symtab unwinders.  */
-    return 0;
+  bool unwind_valid_p
+    = compunit_epilogue_unwind_valid (find_pc_compunit_symtab (pc));
+  if (override_p)
+    {
+      if (unwind_valid_p)
+	/* Don't override the symtab unwinders, skip
+	   "amd64 epilogue override".  */
+	return 0;
+    }
+  else
+    {
+      if (!unwind_valid_p)
+	/* "amd64 epilogue override" unwinder already ran, skip
+	   "amd64 epilogue".  */
+	return 0;
+    }
 
   /* Check whether we're in an epilogue.  */
   return amd64_stack_frame_destroyed_p (gdbarch, pc);
 }
 
+static int
+amd64_epilogue_override_frame_sniffer (const struct frame_unwind *self,
+				       frame_info_ptr this_frame,
+				       void **this_prologue_cache)
+{
+  return amd64_epilogue_frame_sniffer_1 (self, this_frame, this_prologue_cache,
+					 true);
+}
+
+static int
+amd64_epilogue_frame_sniffer (const struct frame_unwind *self,
+			      frame_info_ptr this_frame,
+			      void **this_prologue_cache)
+{
+  return amd64_epilogue_frame_sniffer_1 (self, this_frame, this_prologue_cache,
+					 false);
+}
+
 static struct amd64_frame_cache *
 amd64_epilogue_frame_cache (frame_info_ptr this_frame, void **this_cache)
 {
@@ -3000,6 +3031,17 @@ amd64_epilogue_frame_this_id (frame_info_ptr this_frame,
     (*this_id) = frame_id_build (cache->base + 16, cache->pc);
 }
 
+static const struct frame_unwind amd64_epilogue_override_frame_unwind =
+{
+  "amd64 epilogue override",
+  NORMAL_FRAME,
+  amd64_epilogue_frame_unwind_stop_reason,
+  amd64_epilogue_frame_this_id,
+  amd64_frame_prev_register,
+  NULL,
+  amd64_epilogue_override_frame_sniffer
+};
+
 static const struct frame_unwind amd64_epilogue_frame_unwind =
 {
   "amd64 epilogue",
@@ -3257,7 +3299,9 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch,
   /* Hook the function epilogue frame unwinder.  This unwinder is
      appended to the list first, so that it supercedes the other
      unwinders in function epilogues.  */
-  frame_unwind_prepend_unwinder (gdbarch, &amd64_epilogue_frame_unwind);
+  frame_unwind_prepend_unwinder (gdbarch, &amd64_epilogue_override_frame_unwind);
+
+  frame_unwind_append_unwinder (gdbarch, &amd64_epilogue_frame_unwind);
 
   /* Hook the prologue-based frame unwinders.  */
   frame_unwind_append_unwinder (gdbarch, &amd64_sigtramp_frame_unwind);
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 5e797d098e8..d765c1e95d7 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -2229,9 +2229,9 @@ i386_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
 }
 
 static int
-i386_epilogue_frame_sniffer (const struct frame_unwind *self,
-			     frame_info_ptr this_frame,
-			     void **this_prologue_cache)
+i386_epilogue_frame_sniffer_1 (const struct frame_unwind *self,
+			       frame_info_ptr this_frame,
+			       void **this_prologue_cache, bool override_p)
 {
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
   CORE_ADDR pc = get_frame_pc (this_frame);
@@ -2240,14 +2240,45 @@ i386_epilogue_frame_sniffer (const struct frame_unwind *self,
     /* We're not in the inner frame, so assume we're not in an epilogue.  */
     return 0;
 
-  if (compunit_epilogue_unwind_valid (find_pc_compunit_symtab (pc)))
-    /* Don't override the symtab unwinders.  */
-    return 0;
+  bool unwind_valid_p
+    = compunit_epilogue_unwind_valid (find_pc_compunit_symtab (pc));
+  if (override_p)
+    {
+      if (unwind_valid_p)
+	/* Don't override the symtab unwinders, skip
+	   "i386 epilogue override".  */
+	return 0;
+    }
+  else
+    {
+      if (!unwind_valid_p)
+	/* "i386 epilogue override" unwinder already ran, skip
+	   "i386 epilogue".  */
+	return 0;
+    }
 
   /* Check whether we're in an epilogue.  */
   return i386_stack_frame_destroyed_p (gdbarch, pc);
 }
 
+static int
+i386_epilogue_override_frame_sniffer (const struct frame_unwind *self,
+				      frame_info_ptr this_frame,
+				      void **this_prologue_cache)
+{
+  return i386_epilogue_frame_sniffer_1 (self, this_frame, this_prologue_cache,
+					true);
+}
+
+static int
+i386_epilogue_frame_sniffer (const struct frame_unwind *self,
+			     frame_info_ptr this_frame,
+			     void **this_prologue_cache)
+{
+  return i386_epilogue_frame_sniffer_1 (self, this_frame, this_prologue_cache,
+					false);
+}
+
 static struct i386_frame_cache *
 i386_epilogue_frame_cache (frame_info_ptr this_frame, void **this_cache)
 {
@@ -2320,6 +2351,17 @@ i386_epilogue_frame_prev_register (frame_info_ptr this_frame,
   return i386_frame_prev_register (this_frame, this_cache, regnum);
 }
 
+static const struct frame_unwind i386_epilogue_override_frame_unwind =
+{
+  "i386 epilogue override",
+  NORMAL_FRAME,
+  i386_epilogue_frame_unwind_stop_reason,
+  i386_epilogue_frame_this_id,
+  i386_epilogue_frame_prev_register,
+  NULL,
+  i386_epilogue_override_frame_sniffer
+};
+
 static const struct frame_unwind i386_epilogue_frame_unwind =
 {
   "i386 epilogue",
@@ -8616,13 +8658,16 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
      unwinder in function epilogues (where the DWARF unwinder
      currently fails).  */
   if (info.bfd_arch_info->bits_per_word == 32)
-    frame_unwind_append_unwinder (gdbarch, &i386_epilogue_frame_unwind);
+    frame_unwind_append_unwinder (gdbarch, &i386_epilogue_override_frame_unwind);
 
   /* Hook in the DWARF CFI frame unwinder.  This unwinder is appended
      to the list before the prologue-based unwinders, so that DWARF
      CFI info will be used if it is available.  */
   dwarf2_append_unwinders (gdbarch);
 
+  if (info.bfd_arch_info->bits_per_word == 32)
+    frame_unwind_append_unwinder (gdbarch, &i386_epilogue_frame_unwind);
+
   frame_base_set_default (gdbarch, &i386_frame_base);
 
   /* Pseudo registers may be changed by amd64_init_abi.  */
-- 
2.35.3


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 4/5] [gdb/symtab] Trust epilogue unwind info for unknown producer (-g0 case)
  2023-02-13 14:23 [PATCH v2 0/5] [gdb/symtab] Assume epilogue unwind info is valid unless gcc < 4.5.0 Tom de Vries
                   ` (2 preceding siblings ...)
  2023-02-13 14:23 ` [PATCH v2 3/5] [gdb/tdep] Add amd64/i386 epilogue override unwinders Tom de Vries
@ 2023-02-13 14:23 ` Tom de Vries
  2023-02-13 14:23 ` [PATCH v2 5/5] [gdb/symtab] Trust epilogue unwind info for unknown or non-gcc producer Tom de Vries
  4 siblings, 0 replies; 9+ messages in thread
From: Tom de Vries @ 2023-02-13 14:23 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

For a -g0 -fasynchronous-unwind-tables exec (without .debug_info but with
.eh_frame section), start using the dwarf2 unwinder instead of the
"amd64 epilogue override" unwinder, by returning true in
compunit_epilogue_unwind_valid for cust == nullptr.

This has effect both on the amd64 and i386 targets, but only add amd64
test-case gdb.base/unwind-on-each-insn-amd64-2.exp.
---
 gdb/symtab.h                                  |  4 +-
 .../gdb.base/unwind-on-each-insn-amd64-2.exp  | 52 ++++++++++++++++++
 .../gdb.base/unwind-on-each-insn-amd64-2.s    | 54 +++++++++++++++++++
 3 files changed, 108 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.exp
 create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.s

diff --git a/gdb/symtab.h b/gdb/symtab.h
index cd6b5f722fd..17d2746fd48 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1924,9 +1924,9 @@ static inline bool
 compunit_epilogue_unwind_valid (struct compunit_symtab *cust)
 {
   /* In absence of producer information, assume epilogue unwind info is
-     invalid.  */
+     valid.  */
   if (cust == nullptr)
-    return false;
+    return true;
 
   return cust->epilogue_unwind_valid ();
 }
diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.exp b/gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.exp
new file mode 100644
index 00000000000..c8a94ccef4f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.exp
@@ -0,0 +1,52 @@
+# Copyright 2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+# Check that epilogue unwind info is used, even if no debug info is available.
+
+require is_x86_64_m64_target
+
+set srcfile_flags {debug}
+set srcfile2_flags {nodebug}
+
+if [info exists COMPILE] {
+    # Make sure that we use .eh_frame info, by generating it
+    # using -fasynchronous-unwind-tables.
+    if { [gdb_can_simple_compile fasynchronous-unwind-tables \
+	      { void foo () { } } object -fasynchronous-unwind-tables] } {
+	lappend srcfile2_flags additional_flags=-fasynchronous-unwind-tables
+    } else {
+	unsupported "required: .eh_frame"
+	return
+    }
+    standard_testfile unwind-on-each-insn.c unwind-on-each-insn-foo.c
+    # When updating the .s file, use these flags to generate the file:
+    #lappend srcfile2_flags additional_flags=-save-temps
+    #lappend srcfile2_flags additional_flags=-dA
+    # and do the following:
+    # - copy it in place, run the test-case and verify that all tests pass.
+    # - confuse the amd64 epilogue unwinder by inserting the following
+    #   in foo:
+    #          nop
+    #     +    pushq $.L1
+    #     +    ret
+    #     +  .L1:
+    #     +    nop
+    #          popq %rbp
+    # - verify that the test-case passes.
+} else {
+    standard_testfile unwind-on-each-insn.c .s
+}
+
+source $srcdir/$subdir/unwind-on-each-insn.exp.tcl
diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.s b/gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.s
new file mode 100644
index 00000000000..c141f71817c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.s
@@ -0,0 +1,54 @@
+	.file	"unwind-on-each-insn-foo.c"
+	.text
+	.globl	foo
+	.type	foo, @function
+foo:
+.LFB0:
+	.cfi_startproc
+# BLOCK 2 seq:0
+# PRED: ENTRY (FALLTHRU)
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset 6, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register 6
+	movq	%rdi, -8(%rbp)
+	nop
+        pushq    $.L1
+        ret
+.L1:
+        nop
+	popq	%rbp
+	.cfi_def_cfa 7, 8
+# SUCC: EXIT [100.0%] 
+	ret
+	.cfi_endproc
+.LFE0:
+	.size	foo, .-foo
+	.globl	bar
+	.type	bar, @function
+bar:
+.LFB1:
+	.cfi_startproc
+# BLOCK 2 seq:0
+# PRED: ENTRY (FALLTHRU)
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset 6, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register 6
+	subq	$8, %rsp
+	movq	%rdi, -8(%rbp)
+	movq	-8(%rbp), %rax
+	movq	%rax, %rdi
+	call	foo
+	nop
+	leave
+	.cfi_def_cfa 7, 8
+# SUCC: EXIT [100.0%] 
+	ret
+	.cfi_endproc
+.LFE1:
+	.size	bar, .-bar
+	.ident	"GCC: (SUSE Linux) 7.5.0"
+	.section	.note.GNU-stack,"",@progbits
-- 
2.35.3


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 5/5] [gdb/symtab] Trust epilogue unwind info for unknown or non-gcc producer
  2023-02-13 14:23 [PATCH v2 0/5] [gdb/symtab] Assume epilogue unwind info is valid unless gcc < 4.5.0 Tom de Vries
                   ` (3 preceding siblings ...)
  2023-02-13 14:23 ` [PATCH v2 4/5] [gdb/symtab] Trust epilogue unwind info for unknown producer (-g0 case) Tom de Vries
@ 2023-02-13 14:23 ` Tom de Vries
  4 siblings, 0 replies; 9+ messages in thread
From: Tom de Vries @ 2023-02-13 14:23 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

Currently we only trust epilogue unwind info only for gcc >= 4.5.0.

This has the effect that we don't trust epilogue unwind info for:
- unknown producers (CU without DW_AT_producer attribute)
- non-gcc producers (say, clang).

Instead, only distrust epilogue unwind info only for gcc < 4.5.0.
---
 gdb/dwarf2/read.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index ce6c01ac771..46792bd9162 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -8451,7 +8451,13 @@ process_full_comp_unit (dwarf2_cu *cu, enum language pretend_language)
       if (cu->has_loclist && gcc_4_minor >= 5)
 	cust->set_locations_valid (true);
 
-      if (gcc_4_minor >= 5)
+      int major, minor;
+      if (cu->producer != nullptr
+	  && producer_is_gcc (cu->producer, &major, &minor)
+	  && (major < 4 || (major == 4 && minor < 5)))
+	/* Don't trust gcc < 4.5.x.  */
+	cust->set_epilogue_unwind_valid (false);
+      else
 	cust->set_epilogue_unwind_valid (true);
 
       cust->set_call_site_htab (cu->call_site_htab);
-- 
2.35.3


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/5] [gdb/symtab] Factor out compunit_epilogue_unwind_valid
  2023-02-13 14:23 ` [PATCH v2 1/5] [gdb/symtab] Factor out compunit_epilogue_unwind_valid Tom de Vries
@ 2023-02-14 15:56   ` Tom Tromey
  2023-02-14 22:36     ` Tom de Vries
  0 siblings, 1 reply; 9+ messages in thread
From: Tom Tromey @ 2023-02-14 15:56 UTC (permalink / raw)
  To: Tom de Vries via Gdb-patches; +Cc: Tom de Vries, Tom Tromey

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> Factor out compunit_epilogue_unwind_valid from both
Tom> amd64_stack_frame_destroyed_p and i386_stack_frame_destroyed_p.  NFC.

What does NFC means?

Tom> Also add a comment in the new function about the assumption that in absence of
Tom> producer information, epilogue unwind info is invalid.

Thanks.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/5] [gdb/symtab] Factor out compunit_epilogue_unwind_valid
  2023-02-14 15:56   ` Tom Tromey
@ 2023-02-14 22:36     ` Tom de Vries
  2023-02-20 10:27       ` Tom de Vries
  0 siblings, 1 reply; 9+ messages in thread
From: Tom de Vries @ 2023-02-14 22:36 UTC (permalink / raw)
  To: Tom Tromey, Tom de Vries via Gdb-patches

On 2/14/23 16:56, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> Factor out compunit_epilogue_unwind_valid from both
> Tom> amd64_stack_frame_destroyed_p and i386_stack_frame_destroyed_p.  NFC.
> 
> What does NFC means?
> 

No Functional Changes.

I guess I picked this up when working on clang/llvm.

Mentioned here: https://llvm.org/docs/Lexicon.html#n .

> Tom> Also add a comment in the new function about the assumption that in absence of
> Tom> producer information, epilogue unwind info is invalid.
> 
> Thanks.
> Approved-By: Tom Tromey <tom@tromey.com>
> 

Thanks.
- Tom


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/5] [gdb/symtab] Factor out compunit_epilogue_unwind_valid
  2023-02-14 22:36     ` Tom de Vries
@ 2023-02-20 10:27       ` Tom de Vries
  0 siblings, 0 replies; 9+ messages in thread
From: Tom de Vries @ 2023-02-20 10:27 UTC (permalink / raw)
  To: Tom Tromey, Tom de Vries via Gdb-patches

On 2/14/23 23:36, Tom de Vries via Gdb-patches wrote:
> On 2/14/23 16:56, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries via Gdb-patches 
>>>>>>> <gdb-patches@sourceware.org> writes:
>>
>> Tom> Factor out compunit_epilogue_unwind_valid from both
>> Tom> amd64_stack_frame_destroyed_p and i386_stack_frame_destroyed_p.  
>> NFC.
>>
>> What does NFC means?
>>
> 
> No Functional Changes.
> 
> I guess I picked this up when working on clang/llvm.
> 
> Mentioned here: https://llvm.org/docs/Lexicon.html#n .
> 

And grepping through the commit log, I see I used it before.

Anyway, replaced by "No functional changes".

>> Tom> Also add a comment in the new function about the assumption that 
>> in absence of
>> Tom> producer information, epilogue unwind info is invalid.
>>
>> Thanks.
>> Approved-By: Tom Tromey <tom@tromey.com>
>>

Thanks, I've added the tag to this commit and will commit the series 
after retesting.

Thanks,
- Tom


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-02-20 10:27 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-13 14:23 [PATCH v2 0/5] [gdb/symtab] Assume epilogue unwind info is valid unless gcc < 4.5.0 Tom de Vries
2023-02-13 14:23 ` [PATCH v2 1/5] [gdb/symtab] Factor out compunit_epilogue_unwind_valid Tom de Vries
2023-02-14 15:56   ` Tom Tromey
2023-02-14 22:36     ` Tom de Vries
2023-02-20 10:27       ` Tom de Vries
2023-02-13 14:23 ` [PATCH v2 2/5] [gdb/tdep] Fix amd64/i386_stack_frame_destroyed_p Tom de Vries
2023-02-13 14:23 ` [PATCH v2 3/5] [gdb/tdep] Add amd64/i386 epilogue override unwinders Tom de Vries
2023-02-13 14:23 ` [PATCH v2 4/5] [gdb/symtab] Trust epilogue unwind info for unknown producer (-g0 case) Tom de Vries
2023-02-13 14:23 ` [PATCH v2 5/5] [gdb/symtab] Trust epilogue unwind info for unknown or non-gcc producer Tom de Vries

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).