Re: [PATCH 1/2] Add support for O32 FPXX ABI

["Maciej W. Rozycki" <macro at codesourcery dot com>]

On Tue, 6 May 2014, Richard Sandiford wrote:

> > +/* Return true if the given ELF header flags describe a 32-bit binary.  */
> > +
> > +static bfd_boolean
> > +mips_32bit_flags_p (flagword flags)
> > +{
> > +  return ((flags & EF_MIPS_32BITMODE) != 0
> > +	  || (flags & EF_MIPS_ABI) == E_MIPS_ABI_O32
> > +	  || (flags & EF_MIPS_ABI) == E_MIPS_ABI_EABI32
> > +	  || (flags & EF_MIPS_ARCH) == E_MIPS_ARCH_1
> > +	  || (flags & EF_MIPS_ARCH) == E_MIPS_ARCH_2
> > +	  || (flags & EF_MIPS_ARCH) == E_MIPS_ARCH_32
> > +	  || (flags & EF_MIPS_ARCH) == E_MIPS_ARCH_32R2);
> > +}
> 
> OK, this is preexisting code, but I'm not sure it's right.
> 32bitness should be taken purely from the ABI, not the architecture,
> otherwise we could treat 32-bit MIPS64r2 code differently from MIPS32r2
> code.
> 
> Since o32 was the original (with no flags), I think we should have
> mips_64bit_flags_p instead.

 FWIW, it was my first thought too upon spotting this addition, before I 
realised it's really not one, and this change merely moves the function 
around intact.  Upon further checking this code seems to try mimicking 
what GAS does in mips_after_parse_args:

  /* This flag is set when we have a 64-bit capable CPU but use only
     32-bit wide registers.  Note that EABI does not use it.  */
  if (ISA_HAS_64BIT_REGS (mips_opts.isa)
      && ((mips_abi == NO_ABI && file_mips_gp32 == 1)
	  || mips_abi == O32_ABI))
    mips_32bitmode = 1;

and is really very old.  Note that in this case where ISA_HAS_64BIT_REGS 
and `mips_abi == NO_ABI' are both true (the latter condition being a rare 
corner case, possible for ELF and I think IRIX 5 toolchains only where 
MIPS_DEFAULT_ABI is NO_ABI) GAS will set EF_MIPS_32BITMODE according to 
-mgp32/-mgp64 and the ABI bits will remain unset.

 E_MIPS_ABI_O32 is also omitted entirely on IRIX, according to 
USE_E_MIPS_ABI_O32, so we may want to consider defining ABI_O32_P like 
below instead:

/* Nonzero if ABFD is using the O32 ABI.  */
#define ABI_O32_P(abfd) \
  ((elf_elfheader (abfd)->e_flags & EF_MIPS_ABI) == E_MIPS_ABI_O32
   || (!ABI_64_P (abfd)
       && (elf_elfheader (abfd)->e_flags & (EF_MIPS_ABI | EF_MIPS_ABI2)) == 0))

though I'm afraid the answer from this predicate won't be accurate either, 
for a different set of cases.

> > @@ -14688,6 +15083,86 @@ _bfd_mips_elf_merge_private_bfd_data (bfd *ibfd, bfd *obfd)
> >    if (null_input_bfd)
> >      return TRUE;
> >  
> > +  /* Populate abiflags using existing information.  */
> > +  if (!mips_elf_tdata (ibfd)->abiflags_valid)
> > +    {
> > +      infer_mips_abiflags (ibfd, &mips_elf_tdata (ibfd)->abiflags);
> > +      mips_elf_tdata (ibfd)->abiflags_valid = TRUE;
> > +    }
> > +  else
> > +    {
> > +      Elf_Internal_ABIFlags_v0 abiflags;
> > +      Elf_Internal_ABIFlags_v0 *iabiflags;
> > +      infer_mips_abiflags (ibfd, &abiflags);
> > +      iabiflags = &mips_elf_tdata (ibfd)->abiflags;
> > +
> > +      if (iabiflags->isa_level != abiflags.isa_level
> > +	  || iabiflags->isa_rev != abiflags.isa_rev)
> > +	(*_bfd_error_handler)
> > +	  (_("%B: warning: Inconsistent ISA between e_flags and "
> > +	     ".MIPS.abiflags"), ibfd);
> > +      if (abiflags.fp_abi != Val_GNU_MIPS_ABI_FP_ANY
> > +	  && iabiflags->fp_abi != abiflags.fp_abi)
> > +	(*_bfd_error_handler)
> > +	  (_("%B: warning: Inconsistent FP ABI between e_flags and "
> > +	     ".MIPS.abiflags"), ibfd);
> > +      if ((iabiflags->ases & abiflags.ases) != abiflags.ases)
> > +	(*_bfd_error_handler)
> > +	  (_("%B: warning: Inconsistent ASEs between e_flags and "
> > +	     ".MIPS.abiflags"), ibfd);
> 
> Minor nit, but "abiflags" vs. "iabiflags" is a bit hard to follow,
> since the "i" could be "implicit" (but I assume is "input").

 `in_abiflags' perhaps? -- following `in_attr' and `out_attr' from 
mips_elf_merge_obj_attributes.

  Maciej