diff mbox series

[gdb/tdep] Don't use i386 unwinder for amd64

Message ID	20230210125602.20437-1-tdevries@suse.de
State	Committed
Commit	af0d0f34d8bf7482e598308b54e7ba0ae9b0c349
Headers	DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BC09A3858D28 To: gdb-patches@sourceware.org Subject: [PATCH] [gdb/tdep] Don't use i386 unwinder for amd64 Date: Fri, 10 Feb 2023 13:56:02 +0100 Message-Id: <20230210125602.20437-1-tdevries@suse.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: list From: Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> Reply-To: Tom de Vries <tdevries@suse.de> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org>
Series	[gdb/tdep] Don't use i386 unwinder for amd64 \| [gdb/tdep] Don't use i386 unwinder for amd64

Commit Message

Tom de Vries Feb. 10, 2023, 12:56 p.m. UTC

  For i386 we have these unwinders:
...
$ 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
inline                  INLINE_FRAME
i386 epilogue           NORMAL_FRAME
dwarf2                  NORMAL_FRAME
dwarf2 signal           SIGTRAMP_FRAME
i386 stack tramp        NORMAL_FRAME
i386 sigtramp           SIGTRAMP_FRAME
i386 prologue           NORMAL_FRAME
...
and for amd64:
...
$ 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
i386 epilogue           NORMAL_FRAME
dwarf2                  NORMAL_FRAME
dwarf2 signal           SIGTRAMP_FRAME
amd64 sigtramp          SIGTRAMP_FRAME
amd64 prologue          NORMAL_FRAME
i386 stack tramp        NORMAL_FRAME
i386 sigtramp           SIGTRAMP_FRAME
i386 prologue           NORMAL_FRAME
...

ISTM me there's no reason for the i386 unwinders to be there for amd64.

Furthermore, there's a generic need to play around with enabling and disabling
unwinders, see PR8434.  Currently, that's only available for both the
dwarf2 unwinders at once using "maint set dwarf unwinders on/off".

If I manually disable the "amd64 epilogue" unwinder, the "i386 epilogue"
unwinder becomes active and gives the wrong answer, while I'm actually
interested in the result of the dwarf2 unwinder.  Of course I can also
manually disable the "i386 epilogue", but I take the fact that I have to do
that as evidence that on amd64, the "i386 epilogue" is not only unnecessary,
but in the way.

Fix this by only adding the i386 unwinders only if
"info.bfd_arch_info->bits_per_word == 32".

Note that for the x32 abi (x86_64/-mx32) the unwinder list is the same as for
amd64 (x86_64/-m64), without and with this commit.

Tested on x86_64-linux, -m64 and -m32.  Not tested with -mx32.

PR tdep/30102
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30102
---
 gdb/i386-tdep.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)


base-commit: be01687991aa6c8517b3e635b8f13b0bac6a851a

Comments

John Baldwin Feb. 10, 2023, 5:01 p.m. UTC | #1

On 2/10/23 4:56 AM, Tom de Vries via Gdb-patches wrote:
> For i386 we have these unwinders:
> ...
> $ 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
> inline                  INLINE_FRAME
> i386 epilogue           NORMAL_FRAME
> dwarf2                  NORMAL_FRAME
> dwarf2 signal           SIGTRAMP_FRAME
> i386 stack tramp        NORMAL_FRAME
> i386 sigtramp           SIGTRAMP_FRAME
> i386 prologue           NORMAL_FRAME
> ...
> and for amd64:
> ...
> $ 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
> i386 epilogue           NORMAL_FRAME
> dwarf2                  NORMAL_FRAME
> dwarf2 signal           SIGTRAMP_FRAME
> amd64 sigtramp          SIGTRAMP_FRAME
> amd64 prologue          NORMAL_FRAME
> i386 stack tramp        NORMAL_FRAME
> i386 sigtramp           SIGTRAMP_FRAME
> i386 prologue           NORMAL_FRAME
> ...
> 
> ISTM me there's no reason for the i386 unwinders to be there for amd64.
> 
> Furthermore, there's a generic need to play around with enabling and disabling
> unwinders, see PR8434.  Currently, that's only available for both the
> dwarf2 unwinders at once using "maint set dwarf unwinders on/off".
> 
> If I manually disable the "amd64 epilogue" unwinder, the "i386 epilogue"
> unwinder becomes active and gives the wrong answer, while I'm actually
> interested in the result of the dwarf2 unwinder.  Of course I can also
> manually disable the "i386 epilogue", but I take the fact that I have to do
> that as evidence that on amd64, the "i386 epilogue" is not only unnecessary,
> but in the way.
> 
> Fix this by only adding the i386 unwinders only if
> "info.bfd_arch_info->bits_per_word == 32".
> 
> Note that for the x32 abi (x86_64/-mx32) the unwinder list is the same as for
> amd64 (x86_64/-m64), without and with this commit.
> 
> Tested on x86_64-linux, -m64 and -m32.  Not tested with -mx32.

I strongly suspect you don't want these unwinders for x32 either.  I don't
know what bits_per_word is for x32.  If it is 32 then you at least aren't
changing anything for x32.  If it is 64 then I think your change is correct.
You might consider checking 'tdep->num_dword_regs == 0' instead if you want
to enable these for i386-only and exclude x32.  (IIRC, x32 is encoded in ELF
as EM_X86_64 but with ELFCLASS32 or the like which would mean x32 would have
bits_per_word of 32 I think).

LGTM as-is or if you decide to test num_dword_regs == 0 instead.

Tom de Vries Feb. 11, 2023, 8:14 a.m. UTC | #2

On 2/10/23 18:01, John Baldwin wrote:
> On 2/10/23 4:56 AM, Tom de Vries via Gdb-patches wrote:
>> For i386 we have these unwinders:
>> ...
>> $ 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
>> inline                  INLINE_FRAME
>> i386 epilogue           NORMAL_FRAME
>> dwarf2                  NORMAL_FRAME
>> dwarf2 signal           SIGTRAMP_FRAME
>> i386 stack tramp        NORMAL_FRAME
>> i386 sigtramp           SIGTRAMP_FRAME
>> i386 prologue           NORMAL_FRAME
>> ...
>> and for amd64:
>> ...
>> $ 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
>> i386 epilogue           NORMAL_FRAME
>> dwarf2                  NORMAL_FRAME
>> dwarf2 signal           SIGTRAMP_FRAME
>> amd64 sigtramp          SIGTRAMP_FRAME
>> amd64 prologue          NORMAL_FRAME
>> i386 stack tramp        NORMAL_FRAME
>> i386 sigtramp           SIGTRAMP_FRAME
>> i386 prologue           NORMAL_FRAME
>> ...
>>
>> ISTM me there's no reason for the i386 unwinders to be there for amd64.
>>
>> Furthermore, there's a generic need to play around with enabling and 
>> disabling
>> unwinders, see PR8434.  Currently, that's only available for both the
>> dwarf2 unwinders at once using "maint set dwarf unwinders on/off".
>>
>> If I manually disable the "amd64 epilogue" unwinder, the "i386 epilogue"
>> unwinder becomes active and gives the wrong answer, while I'm actually
>> interested in the result of the dwarf2 unwinder.  Of course I can also
>> manually disable the "i386 epilogue", but I take the fact that I have 
>> to do
>> that as evidence that on amd64, the "i386 epilogue" is not only 
>> unnecessary,
>> but in the way.
>>
>> Fix this by only adding the i386 unwinders only if
>> "info.bfd_arch_info->bits_per_word == 32".
>>
>> Note that for the x32 abi (x86_64/-mx32) the unwinder list is the same 
>> as for
>> amd64 (x86_64/-m64), without and with this commit.
>>
>> Tested on x86_64-linux, -m64 and -m32.  Not tested with -mx32.
> 
> I strongly suspect you don't want these unwinders for x32 either.  I don't
> know what bits_per_word is for x32.  If it is 32 then you at least aren't
> changing anything for x32.  If it is 64 then I think your change is 
> correct.

Hi,

thanks for the review.

It's the latter, 64.

> You might consider checking 'tdep->num_dword_regs == 0' instead if you want
> to enable these for i386-only and exclude x32.  (IIRC, x32 is encoded in 
> ELF
> as EM_X86_64 but with ELFCLASS32 or the like which would mean x32 would 
> have
> bits_per_word of 32 I think).
> > LGTM as-is or if you decide to test num_dword_regs == 0 instead.
> 
I'm leaving it as is.

I've reformulated a bit to make the x32 abi discussion more clear:
...
Note that the x32 abi (x86_64/-mx32):
- has the same unwinder list as amd64 (x86_64/-m64) before this commit,
- has info.bfd_arch_info->bits_per_word == 64, the same as amd64, and
   consequently,
- has the same unwinder list as amd64 after this commit.
...
and committed.

Thanks,
- Tom

diff mbox series

Patch

diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 580664d2ce5..8dccae633f7 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -8613,7 +8613,8 @@  i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
      appended to the list first, so that it supercedes the DWARF
      unwinder in function epilogues (where the DWARF unwinder
      currently fails).  */
-  frame_unwind_append_unwinder (gdbarch, &i386_epilogue_frame_unwind);
+  if (info.bfd_arch_info->bits_per_word == 32)
+    frame_unwind_append_unwinder (gdbarch, &i386_epilogue_frame_unwind);
 
   /* Hook in the DWARF CFI frame unwinder.  This unwinder is appended
      to the list before the prologue-based unwinders, so that DWARF
@@ -8792,9 +8793,12 @@  i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
     tdep-> bnd0_regnum = -1;
 
   /* Hook in the legacy prologue-based unwinders last (fallback).  */
-  frame_unwind_append_unwinder (gdbarch, &i386_stack_tramp_frame_unwind);
-  frame_unwind_append_unwinder (gdbarch, &i386_sigtramp_frame_unwind);
-  frame_unwind_append_unwinder (gdbarch, &i386_frame_unwind);
+  if (info.bfd_arch_info->bits_per_word == 32)
+    {
+      frame_unwind_append_unwinder (gdbarch, &i386_stack_tramp_frame_unwind);
+      frame_unwind_append_unwinder (gdbarch, &i386_sigtramp_frame_unwind);
+      frame_unwind_append_unwinder (gdbarch, &i386_frame_unwind);
+    }
 
   /* If we have a register mapping, enable the generic core file
      support, unless it has already been enabled.  */