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

["Joseph S. Myers" <joseph at codesourcery dot com>]

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.

> == 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)?

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

> +/* 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.

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

> +#if defined PR_GET_FP_MODE && defined PR_SET_FP_MODE
> +#define HAVE_PRCTL_FP_MODE 1
> +#else
> +#define HAVE_PRCTL_FP_MODE 0
> +#endif

"# " indentation for preprocessor directives inside #if.

> +  /* 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.

> +  if (cannot_mode_switch)
> +    return cur_mode != fr1_mode
> +	   && (odd_double_required || cur_mode != 0);

Use parentheses around the return value in such cases as this to help 
ensure proper indentation of the continuation line.

> +int fp32(void)

Please keep to GNU style in testcases, so return type on a separate line 
and space before open parenthesis.

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

-- 
Joseph S. Myers
joseph@codesourcery.com