Message-ID: <alpine.DEB.2.00.1611022043010.24498@tp.orcam.me.uk>
Date:   Fri, 4 Nov 2016 12:54:55 +0000
From:   "Maciej W. Rozycki" <macro@...tec.com>
To:     Guenter Roeck <linux@...ck-us.net>,
        Matthew Fortune <Matthew.Fortune@...tec.com>
CC:     Ralf Baechle <ralf@...ux-mips.org>,
        Alex Smith <alex.smith@...tec.com>,
        <linux-mips@...ux-mips.org>, <linux-kernel@...r.kernel.org>,
        James Hogan <james.hogan@...tec.com>
Subject: Re: [PATCH] MIPS: VDSO: Always select -msoft-float

On Tue, 1 Nov 2016, Guenter Roeck wrote:

> > > Some toolchains fail to build mips images with the following build error.
> > > 
> > > arch/mips/vdso/gettimeofday.c:1:0: error: '-march=r3000' requires '-mfp32'
> > > 
> > > This is seen, for example, with the 'mipsel-linux-gnu-gcc (Debian 6.1.1-9)
> > > 6.1.1 20160705' toolchain as used by the 0Day build robot when building
> > > decstation_defconfig.
> > > 
> > > Comparison of compile flags suggests that the major difference is a missing
> > > '-soft-float', which is otherwise defined unconditionally.
> > > 
> > > Reported-by: kbuild test robot <fengguang.wu@...el.com>
> > > Cc: James Hogan <james.hogan@...tec.com>
> > > Fixes: ebb5e78cc634 ("MIPS: Initial implementation of a VDSO")
> > > Signed-off-by: Guenter Roeck <linux@...ck-us.net>
> > > ---
[...]
> >  If you send me the generated assembly, i.e. `gettimeofday.s', that is 
> > causing you trouble, then I'll see if I can figure out what is going on 
> > here.  We may decide to paper a particularly nasty toolchain bug over from 
> 
> The problem is seen in 0Day builds, with the toolchain mentioned above.

 That does not tell me anything -- I have no idea how each of the 
toolchains out there has been configured.  I can only assess information
provided by the bug/patch submitter, and otherwise try guessing.

> I don't think that generated assembly is going to help, though, since the
> compiler fails to compile the code in the first place because, as it says,
> it doesn't like '-march=r3000' without '-mfp32'.

 Indeed I got that confused, even though there is supposed to be a mention 
in the compiler's diagnostics if a warning or error has originated from an 
external tool invoked.  Sorry about that.  The message itself could well 
have come from the assembler though as the same options are accepted by 
that tool.

 And indeed I have just guessed what the cause might be, that is the 
compiler must have been configured with an explicit `--with-fp-32=xx' or 
`--with-fp-32=64' option.

 Here's quite an extensive analysis of what these options do and why: 
<https://dmz-portal.mips.com/wiki/MIPS_O32_ABI_-_FR0_and_FR1_Interlinking>
and I note that there is a mention of a setup which would have avoided the 
situation we have now:

"Ideally any MIPS I arch/core will default to -mfp32 Any MIPS II -> 
MIPS32r2 arch/core will default to -mfpxx. However, this should be 
controlled via a configure time option to adjust the default ABI from O32 
FP32 to O32 FPXX (or O32 FP64 as needed). The new configure time option is 
--with-fp-32=[32|xx|64] and this affects the FP mode only when targetting 
the O32 ABI."

however this ideal arrangement, which I would expect a configuration 
option like `--with-fp-32=from-isa' (and a corresponding `-mfp=from-isa' 
compiler option) would handle, got lost/forgotten in the works somehow.

 Matthew has been the expert in this area and might be able to add some 
more -- Matthew?  Also shall we call it a compiler defect?  We seem to be 
getting more and more toolchain configurations which break the existing 
setups in a way requiring them to add more and more compiler options to 
Makefiles for preexisting targets even though the Makefiles were 
previously already prepared to choose the right compiler options for the 
target.  This seems the wrong way to go to me as it's causing people 
burden when upgrading their toolchains while it's supposed to be seamless 
(it's one aspect of maintaining backwards compatibility).

 Meanwhile your proposal may be indeed the way to go, or perhaps we could 
use a substitution rule like:

	$(patsubst -march=%,-mfp32,$(filter -march=r3%,$(KBUILD_CFLAGS)))

-- see the change below, it seems to work for me and the option is indeed 
passed with a `-march=r3000' build (as would with a `-march=r3900' build) 
and suppressed otherwise (such as with a `-march=mips32r2' build).  

 I have tried building `arch/mips/vdso/gettimeofday.s' to see how the 
module settings are chosen by the compiler in my successful build, and 
that produced an unexpected result in that the rule for .s targets is 
evidently broken for VDSO as different compiler flags were used from ones 
used for .o targets -- essentially the regular flags rather than the 
special VDSO ones (no `-fPIC', etc.).  So that looks like a bug to me to 
be fixed too.

 So instead I just checked the ABI annotation of a good .o object with 
`readelf':

$ readelf -A arch/mips/vdso/gettimeofday.o
Attribute Section: gnu
File Attributes
  Tag_GNU_MIPS_ABI_FP: Hard float (double precision)

MIPS ABI Flags Version: 0

ISA: MIPS1
GPR size: 32
CPR1 size: 32
CPR2 size: 0
FP ABI: Hard float (double precision)
ISA Extension: None
ASEs:
	None
FLAGS 1: 00000000
FLAGS 2: 00000000
$ 

and I think this would be good to keep.  Obviously this will prevent 
`-mfp64' executables or DSOs from being loaded together with VDSO, but 
they are incompatible with the 32-bit FPU MIPS I processors have anyway, 
so no change in the semantics there.

 On the other hand there's still the issue what the compiler default will 
be for o32 in a non-r3k build -- which could be any of `-mfp32', `-mfpxx' 
or `-mfp64'.  Requesting `-mfpxx' should allow the greatest flexibility, 
but may not be supported as older compilers did not have it, so this would 
have to be added conditionally only, letting those older compilers retain 
their long-established `-mfp32' default.  I have an idea how to implement 
this part if we agree this is indeed the right direction.

 For the record: `-mfp32' has been there since forever, my little research 
indicating this commit:

Wed Jan 15 06:22:34 1992  Michael Meissner  (meissner at osf.org)

	* mips.h (MIPS_VERSION): Bump Meissner version # to 8.
	(fcmp_op): Add function declaration.
	(TARGET_FLOAT64): Support for -mfp64 switch to support the R4000 FR
	psw bit, which doubles the number of floating point registers.
	(TARGET_SWITCHES): Likewise.
	(HARD_REGNO_NREGS): Likewise.
	(CLASS_FCMP_OP): New bit for classifing floating point compares.
	(PREDICATE_CODES): Add cmp2_op and fcmp_op support.
[...]

which corresponds to GCC 2.0 released roughly a month afterwards, although 
the exact diff seems to have been lost in the mist of history.  The oldest 
version of `mips.h' in the modern GCC repository dates back to r297 from 
11 Feb 1992 where it already contains `-mfp32' support; obviously a sign 
of an inaccurate import of those old changes.  Anyway the option is surely 
always safe to use nowadays.

  Maciej

linux-mips-r3k-vdso-fp32.diff
Index: linux-sfr-decstation/arch/mips/vdso/Makefile
===================================================================
--- linux-sfr-decstation.orig/arch/mips/vdso/Makefile	2016-10-23 06:15:11.000000000 +0100
+++ linux-sfr-decstation/arch/mips/vdso/Makefile	2016-11-03 22:44:31.752065000 +0000
@@ -6,7 +6,8 @@ ccflags-vdso := \
 	$(filter -I%,$(KBUILD_CFLAGS)) \
 	$(filter -E%,$(KBUILD_CFLAGS)) \
 	$(filter -mmicromips,$(KBUILD_CFLAGS)) \
-	$(filter -march=%,$(KBUILD_CFLAGS))
+	$(filter -march=%,$(KBUILD_CFLAGS)) \
+	$(patsubst -march=%,-mfp32,$(filter -march=r3%,$(KBUILD_CFLAGS)))
 cflags-vdso := $(ccflags-vdso) \
 	$(filter -W%,$(filter-out -Wa$(comma)%,$(KBUILD_CFLAGS))) \
 	-O2 -g -fPIC -fno-strict-aliasing -fno-common -fno-builtin -G 0 \

Powered by blists - more mailing lists