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

[Tom de Vries <tdevries@suse.de>]

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