[RFC,gdb/tdep] Assume epilogue unwind info is valid unless gcc < 4.5.0

Message ID 20230121074807.22032-1-tdevries@suse.de
State Superseded
Headers
Series [RFC,gdb/tdep] Assume epilogue unwind info is valid unless gcc < 4.5.0 |

Commit Message

Tom de Vries Jan. 21, 2023, 7:48 a.m. UTC
  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.

PR tdep/30028
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30028
---
 gdb/amd64-tdep.c  |  5 ++++-
 gdb/dwarf2/read.c | 10 +++++++++-
 gdb/i386-tdep.c   |  5 ++++-
 3 files changed, 17 insertions(+), 3 deletions(-)


base-commit: 76f8ef8d53792ef89aee7a51b94bc7d1cf324379
  

Comments

Tom Tromey Jan. 21, 2023, 5:48 p.m. UTC | #1
>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

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

FWIW this approach makes sense to me.

It's pretty lame that there's no way to detect this failure from the
frame section -- it can't be producer-sniffed and the augmentation
strings can't really be changed.

gcc 4.5 was released in 2010, and so it's not like we're inconveniencing
a lot of users.  If needed I guess we could add a user setting to switch
this behavior back on.

Note there is a similar issue for the prologue, see:

https://sourceware.org/bugzilla/show_bug.cgi?id=25696
https://sourceware.org/bugzilla/show_bug.cgi?id=17265
https://sourceware.org/bugzilla/show_bug.cgi?id=21470

Also worth seeing the hilarious:

https://github.com/rust-lang/rust/issues/41252#issuecomment-293676579

I think that in this area we should assume the debug info is correct,
and keep a list of known-bad producers rather than assuming the debug
info is wrong and having a list of known-good ones.

Tom> +  if (/* In absence of producer information, optimistically assume that we're
Tom> +	 not dealing with gcc < 4.5.0.  */

This placement of the comment is pretty weird, it seems fine to just
stick it before the 'if'.

Tom> +      if (cu->producer == nullptr)
Tom> +	/* In absence of producer information, optimistically assume that we're
Tom> +	   not dealing with gcc < 4.5.0.  */
Tom> +	cust->set_epilogue_unwind_valid (true);
Tom> +      if (!producer_is_gcc (cu->producer, nullptr, nullptr))

Normally if there is a comment and a line of code as the consequence of
an 'if', we put them both in a block.

Anyway I was also thinking that the second one should say 'else if'.

Tom
  

Patch

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 70d0d0f3318..c19220d006d 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2906,7 +2906,10 @@  amd64_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
   struct compunit_symtab *cust;
 
   cust = find_pc_compunit_symtab (pc);
-  if (cust != NULL && cust->epilogue_unwind_valid ())
+  if (/* In absence of producer information, optimistically assume that we're
+	 not dealing with gcc < 4.5.0.  */
+      cust == NULL
+      || cust->epilogue_unwind_valid ())
     return 0;
 
   if (target_read_memory (pc, &insn, 1))
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index cd937f24ee7..40869f708e3 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -8484,7 +8484,15 @@  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)
+      if (cu->producer == nullptr)
+	/* In absence of producer information, optimistically assume that we're
+	   not dealing with gcc < 4.5.0.  */
+	cust->set_epilogue_unwind_valid (true);
+      if (!producer_is_gcc (cu->producer, nullptr, nullptr))
+	/* Not gcc.  */
+	cust->set_epilogue_unwind_valid (true);
+      else if (gcc_4_minor >= 5)
+	/* gcc >= 4.5.0.  */
 	cust->set_epilogue_unwind_valid (true);
 
       cust->set_call_site_htab (cu->call_site_htab);
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 580664d2ce5..4eab2e6f7a3 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -2222,7 +2222,10 @@  i386_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
   struct compunit_symtab *cust;
 
   cust = find_pc_compunit_symtab (pc);
-  if (cust != NULL && cust->epilogue_unwind_valid ())
+  if (/* In absence of producer information, optimistically assume that we're
+	 not dealing with gcc < 4.5.0.  */
+      cust == NULL
+      || cust->epilogue_unwind_valid ())
     return 0;
 
   if (target_read_memory (pc, &insn, 1))