Passing long double args on 32-bit SPARC violates ABI

[Quuxplusone]

Bugzilla Link	PR42493
Status	NEW
Importance	P normal
Reported by	Rainer Orth (ro@gcc.gnu.org)
Reported on	2019-07-03 01:28:12 -0700
Last modified on	2020-10-09 07:07:38 -0700
Version	trunk
Hardware	Sun Solaris
CC	efriedma@quicinc.com, llvm-bugs@lists.llvm.org, venkatra@cs.wisc.edu
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

I'm current working to fix the remaining compiler-rt testsuite bugs on SPARC
(https://reviews.llvm.org/D40900).  One of the failures is

    Builtins-sparc-sunos :: divtc3_test.c
    Builtins-sparcv9-sunos :: divtc3_test.c

While the sparcv9 failure is different, the sparc one boils down to incorrectly
passing long double arguments.  Consider the following testcase:

$ cat caller.c
extern void callee (long double);

int
main (void)
{
  long double ld = 1.0;

  callee (ld);
  return 0;
}
$ cat callee.c
extern void abort (void);

void
callee (long double ld)
{
  if (ld != 1.0)
    abort ();
}
$ clang -m32 -c caller.c -o caller.clang.o
$ clang -m32 -c callee.c -o callee.clang.o
$ gcc -m32 -c caller.c -o caller.gcc.o
$ gcc -m32 -c callee.c -o callee.gcc.o
$ gcc -m32 -o clang-gcc caller.clang.o callee.gcc.o
$ ./clang-gcc
Segmentation Fault (core dumped)
$ gcc -m32 -o gcc-clang caller.gcc.o callee.clang.o
$ ./gcc-clang
Abort (core dumped)

The SPARC psABI, p.3-15 (Structure, Union, and Quad-Precision Arguments)
requires long double args to be passed by reference, while clang passes
them by value (as is correct for SPARCv9).

[Quuxplusone]

I just discovered that things are even worse:

$ cat sald.c
#include <stdio.h>
#include <stdalign.h>

int
main (void)
{
  printf ("sizeof (long double) = %ld\n", (long) sizeof (long double));
  printf ("alignof(long double) = %ld\n", (long) alignof (long double));
  return 0;
}
$ cc -m32 -o sald-cc sald.c
$ ./sald-cc
sizeof (long double) = 16
alignof(long double) = 8
$ gcc -m32 -o sald-gcc sald.c
$ ./sald-gcc
sizeof (long double) = 16
alignof(long double) = 8
$ clang -m32 -o sald-clang sald.c
./sald-clang
sizeof (long double) = 8
alignof(long double) = 8

So clang even gets the size of long double wrong on 32-bit SPARC.

[Quuxplusone]

(In reply to Rainer Orth from comment #1)
> So clang even gets the size of long double wrong on 32-bit SPARC.

That's easy to fix; changing the underlying type for long double just requires
adding a few lines in clang/lib/Basic/Targets/ .  (It's controlled by
LongDoubleFormat/LongDoubleWidth/LongDoubleAlign.)

[Quuxplusone]

(In reply to Eli Friedman from comment #2)
> (In reply to Rainer Orth from comment #1)
> > So clang even gets the size of long double wrong on 32-bit SPARC.
>
> That's easy to fix; changing the underlying type for long double just
> requires adding a few lines in clang/lib/Basic/Targets/ .  (It's controlled
> by LongDoubleFormat/LongDoubleWidth/LongDoubleAlign.)

That's what I tried, matching the sparcv9 case except for the smaller alignment.

I ran into quite a number of issues, however:

* The long double tests in compiler-rt assume __int128_t for 128-bit long
double.
  However, that doesn't exist on 32-bit (sparc or otherwise).

* I also ran into

fatal error: error in backend: SPARCv8 does not handle f128 in calls; pass
indirectly

  I must admit I'm pretty confused about the interaction of the LongDouble*
  variables above, resetDataLayout calls and maybe more.  Besides, there's
  SparcCallingConv.td and support routines in llvm, so this tends to get more
  complex.

* To make things worse, even on sparcv9 (where long double was supposed to be
  correct), I find quite a number of long double test failing in a 2-stage
  build:

  Builtins-sparcv9-sunos :: addtf3_test.c
  Builtins-sparcv9-sunos :: divtf3_test.c
  Builtins-sparcv9-sunos :: extenddftf2_test.c
  Builtins-sparcv9-sunos :: extendsftf2_test.c
  Builtins-sparcv9-sunos :: floatditf_test.c
  Builtins-sparcv9-sunos :: floatsitf_test.c
  Builtins-sparcv9-sunos :: floattitf_test.c
  Builtins-sparcv9-sunos :: floatunditf_test.c
  Builtins-sparcv9-sunos :: floatunsitf_test.c
  Builtins-sparcv9-sunos :: floatuntitf_test.c
  Builtins-sparcv9-sunos :: multf3_test.c
  Builtins-sparcv9-sunos :: subtf3_test.c

  while the tests PASS in a 1-stage build with gcc.

  I haven't yet investigated more closely, but this shows that long double on
  sparc has a considerable number of issues, not just on 32-bit.

[Quuxplusone]

Okay, maybe a little more complicated. :)

For compiler-rt, see the bit that enables -fforce-enable-int128 for riscv in
compiler-rt/lib/builtins/CMakeLists.txt .

> fatal error: error in backend: SPARCv8 does not handle f128 in calls; pass
indirectly

Ideally, the fatal error here should be replaced with code to implement this;
it doesn't make sense to refuse to lower calls involving f128, and
optimizations are likely to explode if the backend doesn't implement it.  Let
me know if you want tips on implementing this.

On the clang side, if you want to mess with the way calls are lowered to LLVM
IR, the relevant code is clang/lib/CodeGen/TargetInfo.cpp .  There's some SPARC-
specific code, although it's pretty bare-bones at the moment.

> I must admit I'm pretty confused about the interaction of the LongDouble*
> variables above, resetDataLayout calls and maybe more.  Besides, there's
> SparcCallingConv.td and support routines in llvm, so this tends to get more
> complex.

In general, clang has its own view of types and memory layout, independent of
LLVM IR.  This is used, for example, to compute sizeof().  Theoretically, you
could use the clang libraries to analyze code on a target that doesn't have an
LLVM backend.  The datalayout is only used when it comes time to emit IR.

[Quuxplusone]

(In reply to Rainer Orth from comment #3)

> I ran into quite a number of issues, however:

> * To make things worse, even on sparcv9 (where long double was supposed to be
>   correct), I find quite a number of long double test failing in a 2-stage
>   build:
>
>   Builtins-sparcv9-sunos :: addtf3_test.c
>   Builtins-sparcv9-sunos :: divtf3_test.c
>   Builtins-sparcv9-sunos :: extenddftf2_test.c
>   Builtins-sparcv9-sunos :: extendsftf2_test.c
>   Builtins-sparcv9-sunos :: floatditf_test.c
>   Builtins-sparcv9-sunos :: floatsitf_test.c
>   Builtins-sparcv9-sunos :: floattitf_test.c
>   Builtins-sparcv9-sunos :: floatunditf_test.c
>   Builtins-sparcv9-sunos :: floatunsitf_test.c
>   Builtins-sparcv9-sunos :: floatuntitf_test.c
>   Builtins-sparcv9-sunos :: multf3_test.c
>   Builtins-sparcv9-sunos :: subtf3_test.c
>
>   while the tests PASS in a 1-stage build with gcc.
>
>   I haven't yet investigated more closely, but this shows that long double on
>   sparc has a considerable number of issues, not just on 32-bit.

I've now analyzed this further and filed Bug 47729 for the (unrelated) issue.

[Quuxplusone]

(In reply to Eli Friedman from comment #4)
> Okay, maybe a little more complicated. :)
>
> For compiler-rt, see the bit that enables -fforce-enable-int128 for riscv in
> compiler-rt/lib/builtins/CMakeLists.txt .

Nice.  Unfortunately, this is clang-only, so ultimately a different solution
will have to be chose to continue supporting builds with gcc.  For the moment,
it certainly helps me along.

> > fatal error: error in backend: SPARCv8 does not handle f128 in calls; pass
indirectly
>
> Ideally, the fatal error here should be replaced with code to implement
> this; it doesn't make sense to refuse to lower calls involving f128, and
> optimizations are likely to explode if the backend doesn't implement it.
> Let me know if you want tips on implementing this.

That would certainly be helpful: I've looked at both SparcCallingConv.td and
SparcISelLowering.cpp a bit, but am somewhat lost there.

> On the clang side, if you want to mess with the way calls are lowered to
> LLVM IR, the relevant code is clang/lib/CodeGen/TargetInfo.cpp .  There's
> some SPARC-specific code, although it's pretty bare-bones at the moment.

From a quick look, maybe the FP128TyID needs to be carried over from SparcV9;
can't tell yet.

> > I must admit I'm pretty confused about the interaction of the LongDouble*
> > variables above, resetDataLayout calls and maybe more.  Besides, there's
> > SparcCallingConv.td and support routines in llvm, so this tends to get more
> > complex.
>
> In general, clang has its own view of types and memory layout, independent
> of LLVM IR.  This is used, for example, to compute sizeof().  Theoretically,
> you could use the clang libraries to analyze code on a target that doesn't
> have an LLVM backend.  The datalayout is only used when it comes time to
> emit IR.

Ah, thanks.  Strangely SparcV8 already uses f128:64 in
clang/lib/Basic/Targets/Sparc.h
although clang otherwise uses 64-bit long double on SparcV8.

I've meanwhile found that despite the SVR4 SPARC psABI requiring 128-bit long
double, several targets do their own thing here which complicates things
tremendously:

* Solaris/SPARC with -m32 always uses 128-bit long double, following the psABI

* so do FreeBSD (but supports -mlong-double-64, too), OpenBSD (always 128-bit),
  sparc64-*-elf* and sparc64-*-rtems*

* NetBSD/sparc (i.e. 32-bit kernel) uses 64-bit long double, NetBSD/sparc64
  has 128-bit instead, optionally with support for -mlong-double-64

* similarly, Linux/sparc goes for 64-bit, while Linux/sparc64 does 128-bit,
  both with the -mlong-double-64 option

With the possible exception of Linux/sparc64 (in the GCC compile farm) I can
test none of those, so I'll leave them alone, allowing for the respective
maintainers to make adjustments.

Right now there are only two failing tests due to this issue in a 2-stage
build:

  UBSan-Standalone-sparc :: TestCases/Float/cast-overflow.cpp
  UBSan-Standalone-sparc :: TestCases/Misc/log-path_test.cpp

I wonder if it wouldn't be appropriate to XFAIL them to try and get the
Solaris/sparcv9 buildbot green finally, and either work in parallel on this
issue or leave it to the Sparc maintainers?

[Quuxplusone]

Thanks to your hints I've now been able to get the 128-bit long double support
working.  I've posted the current state of affairs at
https://reviews.llvm.org/D89130
where I believe it's easier to discuss than in this PR.