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

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

   if (gdbarch_debug)
  

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.

On 2018-10-29 4:18 a.m., Denis Dmitriev wrote:
> 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
    {
      ...
    }

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

> +      {
> +        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;

> +    }
> +  }
>    else
>      elf_flags = 0;
>    if (gdbarch_debug)
> 
> 

Simon
  

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.

> 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

 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.

> > +      {
> > +        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;

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

  Maciej