[PATCH] Abi selection based on set arch command in case of remote debugging for MIPS

[Denis Dmitriev]

Abi selection based on the execution of the "set arch" command in the case
of remote debugging. Mips. No need to manually configure abi.

diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 5e0a606..dc6b6181 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -8085,7 +8085,20 @@ mips_gdbarch_init (struct gdbarch_info info, struct
gdbarch_list *arches)
if (info.abfd && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour)
elf_flags = elf_elfheader (info.abfd)->e_flags;
else if (arches != NULL)
+ {
elf_flags = gdbarch_tdep (arches->gdbarch)->elf_flags;
+ if (elf_flags == 0)
+ {
+ if (info.bfd_arch_info && info.bfd_arch_info->bits_per_address == 64)
+ {
+ elf_flags = E_MIPS_ABI_EABI64;
+ }
+ else if (info.bfd_arch_info->bits_per_address == 32)
+ {
+ elf_flags = MIPS_ABI_EABI32;
+ }
+ }
+ }
else
elf_flags = 0;
if (gdbarch_debug)

[Simon Marchi]

Maciej,

Just wondering, would this patch still fall under your maintainership, and do
you plan on reviewing it? I think it's a followup this, which should give more
context:

https://sourceware.org/ml/gdb-patches/2018-10/msg00342.html

Denis: In the mean time, I noted some comments about formatting. Also, you would
need to provide a ChangeLog entry with the change. See this for more details:

https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_Formatted_GNU_ChangeLog

For this change, it could look like:

* mips-tdep.c (mips_gdbarch_init): Select ABI based on architecture if no
binary is provided.

Post by Denis Dmitriev
Abi selection based on the execution of the "set arch" command in the case
of remote debugging. Mips. No need to manually configure abi.
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 5e0a606..dc6b6181 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -8085,7 +8085,20 @@ mips_gdbarch_init (struct gdbarch_info info, struct
gdbarch_list *arches)
if (info.abfd && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour)
elf_flags = elf_elfheader (info.abfd)->e_flags;
else if (arches != NULL)
+ {

Applicable to all if/else, the braces should indented:

if (...)
{
...
}
else
{
...
}

Post by Denis Dmitriev
elf_flags = gdbarch_tdep (arches->gdbarch)->elf_flags;
+ if (elf_flags == 0)
+ {
+ if (info.bfd_arch_info && info.bfd_arch_info->bits_per_address == 64)

When checking if a pointer is null, use the explicit

if (info.bfd_arch_info != NULL && ...)

or

if (info.bfd_arch_info != nullptr && ...)

I know there are violations of this rule in some places, but for new code we try
to enforce the GNU style.

Post by Denis Dmitriev
+ {
+ elf_flags = E_MIPS_ABI_EABI64;
+ }
+ else if (info.bfd_arch_info->bits_per_address == 32)
+ {
+ elf_flags = MIPS_ABI_EABI32;
+ }

When there is a single statement, drop the braces:

if (...)
elf_flags = E_MIPS_ABI_EABI64;
else
elf_flags = MIPS_ABI_EABI32;

Post by Denis Dmitriev
+ }
+ }
else
elf_flags = 0;
if (gdbarch_debug)

Simon

[Maciej W. Rozycki]

Simon,

Apologies for the long RTT, I've come across your e-mail only now, while
routinely reviewing general mailing list traffic received. I think my
mips.com address does not work anymore (did you get a bounce?), and even
if it does, I have no access to whatever system collects incoming messages
there.

Post by Simon Marchi
Just wondering, would this patch still fall under your maintainership, and do
you plan on reviewing it? I think it's a followup this, which should give more
https://sourceware.org/ml/gdb-patches/2018-10/msg00342.html

Thanks for the pointer. I suppose I could look into it as a general
issue, even though Octeon specifically used here is not a legacy MIPS
implementation, but the change proposed makes no sense to me I am afraid.
You can't infer the ABI just from the architecture and whatever you choose
may work with one remote debug stub and then break with another (e.g. o32
Linux `gdbserver' will exchange register data in 32-bit quantities and use
32-bit addressing even with a 64-bit processor).

Also AFAIK the embedded MIPS ABIs have been obsolete for years now; I
haven't seen any use of them for well over a decade now, so I recommend
against making any new use of them.

The correct solution to get the address and register size right in GDB is
to communicate it with an XML target description, and this is what I'd
suggest implementing in the debug stub, as it is the stub that knows what
it communicates to GDB. Then GDB can present that information according
to the ABI selected, either manually or inferred from the ELF file
selected for debugging, however in the absence of an ELF file GDB still
won't be able to guess the ABI (we might consider having a different
default selected somehow at build time, although o32 has the benefit of
being compatible with all MIPS hardware).

It would be good to know in detail though what the original problem was
that the change proposed was intended to solve, as my proposal for the
solution has been based on a guess and may not actually address what has
been causing troubles.

Post by Simon Marchi

Post by Denis Dmitriev
+ {
+ elf_flags = E_MIPS_ABI_EABI64;
+ }
+ else if (info.bfd_arch_info->bits_per_address == 32)
+ {
+ elf_flags = MIPS_ABI_EABI32;
+ }

if (...)
elf_flags = E_MIPS_ABI_EABI64;
else
elf_flags = MIPS_ABI_EABI32;

This clearly hasn't been even barely tested :( as `E_MIPS_ABI_*' and
`MIPS_ABI_*' groups of constants have distinct uses each.

Maciej