RE: [PATCHv3, MIPS] Add support for O32 FPXX and program header based ABI information

[Matthew Fortune <Matthew dot Fortune at imgtec dot com>]

Joseph S. Myers <joseph@codesourcery.com> writes:
> On Thu, 2 Oct 2014, Matthew Fortune wrote:
> 
> > == Mixing old and new executables with post-PT_MIPS_ABIFLAGS dynamic
> linkers ==
> >
> > One caveat appears when mixing old soft-float with new soft-float. Since
> the old
> > soft-float binaries had no dynamic linker visible markings then
> > post-PT_MIPS_ABIFLAGS logic detects these as hard-float. This means that
> mixing
> > old and new soft-float is not possible and will be rejected. Since soft-
> float is
> > not a publicly supported ABI variant for any major MIPS Linux
> distribution it
> > seems reasonable for this to take the hit of needing a re-build.
> 
> No, this should work - meaning that the new logic should accept old
> binaries without PT_MIPS_ABIFLAGS as possibly being either hard or soft
> float.

I'll have to consider the impact of this on the overall plan and also what
it will do to implementation. It is perhaps OK but I have some concerns over
how to implement it. I.e. I am very much against considering existing
libraries to be the 'any' ABI but will look into them being either soft-float
(fpabi 3) or double-precision (fpabi 1).

> > == GLIBC tests ==
> >
> > A cut down version of the full FPXX testsuite is included in the GLIBC
> test
> > system and covers as many ABI variants as the build configuration allows.
> The
> > glibc version of the test performs load and unload to exhaustively cover
> the
> > double-float ABI variants.
> 
> I take it the code will avoid building / running tests not supported in
> the configuration for which glibc is built (if e.g. that's soft-float, or
> with a compiler without FPXX support)?

That is the intention. The configure checks are there to determine what the
precise O32 FP ABI is and from that the test case may or may not be enabled
and the test libraries are built depending on what can successfully link.

> > +typedef struct
> > +{
> > +  /* Version of flags structure.  */
> > +  Elf32_Half version;
> > +  /* The level of the ISA: 1-5, 32, 64.  */
> > +  unsigned char isa_level;
> > +  /* The revision of ISA: 0 for MIPS V and below, 1-n otherwise.  */
> > +  unsigned char isa_rev;
> > +  /* The size of general purpose registers.  */
> > +  unsigned char gpr_size;
> > +  /* The size of co-processor 1 registers.  */
> > +  unsigned char cpr1_size;
> > +  /* The size of co-processor 2 registers.  */
> > +  unsigned char cpr2_size;
> > +  /* The floating-point ABI.  */
> > +  unsigned char fp_abi;
> > +  /* Processor-specific extension.  */
> > +  Elf32_Word isa_ext;
> > +  /* Mask of ASEs used.  */
> > +  Elf32_Word ases;
> > +  /* Mask of general flags.  */
> > +  Elf32_Word flags1;
> > +  Elf32_Word flags2;
> > +} Elf_ABIFlags_v0;
> 
> As this structure in elf.h is MIPS-specific, the name should reflect that,
> rather than having a generic name such as Elf_ABIFlags_v0.

Indeed. I simply copy/pasted this from binutils but binutils segregates
things more into architectures. I'll make it Elf_MIPS_ABIFlags_v0.

> > +/* Values for the register size bytes of an abi flags structure.  */
> > +
> > +#define AFL_REG_NONE		0x00	 /* No registers.  */
> > +#define AFL_REG_32		0x01	 /* 32-bit registers.  */
> > +#define AFL_REG_64		0x02	 /* 64-bit registers.  */
> > +#define AFL_REG_128		0x03	 /* 128-bit registers.  */
> 
> Similarly, MIPS should appear somewhere in all these AFL_* macro names.

Prefixing with MIPS_ throughout would seem OK here.

> > diff --git a/sysdeps/mips/bits/hwcap.h b/sysdeps/mips/bits/hwcap.h
> > new file mode 100644
> > index 0000000..0013717
> > --- /dev/null
> > +++ b/sysdeps/mips/bits/hwcap.h
> 
> Although small, shouldn't it be possible to split the bits/hwcap.h and
> dl-procinfo changes out to a separate patch?  I don't see anything in them
> that obviously depends on the other changes.

Will do.

> > +  /* Get the current hardware mode.  */
> > +  cur_mode = prctl (PR_GET_FP_MODE);
> 
> Is it namespace-safe to call prctl inside the dynamic linker or are there
> circumstances in which it could be overridden by the user's definition of
> prctl (given that prctl is not in the ISO C90 namespace)?  Unless there's
> a clear reason it's safe, __prctl should be used instead.

My honest answer is that I don't know so I will switch to __prctl regardless.
 
> > diff --git a/sysdeps/unix/sysv/linux/mips/ldsodefs.h
> b/sysdeps/unix/sysv/linux/mips/ldsodefs.h
> > index d7c62f4..70f8f16 100644
> > --- a/sysdeps/unix/sysv/linux/mips/ldsodefs.h
> > +++ b/sysdeps/unix/sysv/linux/mips/ldsodefs.h
> > @@ -34,7 +34,7 @@ extern void _dl_static_init (struct link_map *map);
> >  #undef VALID_ELF_ABIVERSION
> >  #define VALID_ELF_ABIVERSION(osabi,ver)			\
> >    (ver == 0						\
> > -   || (osabi == ELFOSABI_SYSV && ver < 2)		\
> > +   || (osabi == ELFOSABI_SYSV && ver < 4)		\
> >     || (osabi == ELFOSABI_GNU && ver < LIBC_ABI_MAX))
> 
> I think you should make the static linker set ELFOSABI_GNU for binaries
> with ABI version 3, so that this change isn't needed.

I'm not sure that I am OK with this. The ABI extension is not a GNU specific
extension, it is a generic ABI extension which is why I have applied it to
both osabis with the same value.

Thanks for the quick feedback, I'll wait a day or so before posting an update
to allow anyone else to comment.

Matthew