Page MenuHomeFreeBSD

Replace the incorrect C arm fabs(3) code with an asm version.
Needs ReviewPublic

Authored by andrew on Nov 17 2017, 9:43 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 6, 2:02 AM
Unknown Object (File)
Oct 7 2024, 9:37 AM
Unknown Object (File)
Oct 6 2024, 5:52 AM
Unknown Object (File)
Oct 3 2024, 6:29 AM
Unknown Object (File)
Oct 3 2024, 4:03 AM
Unknown Object (File)
Oct 2 2024, 7:00 AM
Unknown Object (File)
Oct 1 2024, 1:46 AM
Unknown Object (File)
Sep 30 2024, 11:20 PM

Details

Reviewers
None
Group Reviewers
ARM
Summary

The arm fabs(3) code is incorrect for fabs(-0). This is because -0 == +0
so the comparison is incorrect. Fix this by implementing the code in
assembly. On ARMv6 and later we can use the VFP unit to return the correct
value. On ARMv4 and ARMv5 just clear the sign bit.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 12807
Build 13074: arc lint + arc unit

Event Timeline

arichardson added inline comments.
lib/libc/arm/gen/fabs.S
43

Clang and GCC both support __builtin_fabs which should expand to the right thing. The documentation for GCC 3.4 says it supports it so using it without a __has_builtin check should be fine: https://gcc.gnu.org/onlinedocs/gcc-3.4.5/gcc/Other-Builtins.html

I don't think we can rely on VFP hardware being available. It is at least theoretically possible to build a kernel using "nooptions VFP" and build all of userland with --float-abi=soft and any use of a vfp instruction will be a SIGILL, right?

Given that fabs() will invoke the clang builtin codegen unless something has specifically disabled that (and clang knows whether to generate vfp insns or not), would a viable fix for this should-never-be-called function be something like the following?

double
fabs(double x)
{

if (x == 0)
  return 0;
else if (x < 0)
  return -x;
else
  return x;

}

In D13131#288739, @ian wrote:

I don't think we can rely on VFP hardware being available. It is at least theoretically possible to build a kernel using "nooptions VFP" and build all of userland with --float-abi=soft and any use of a vfp instruction will be a SIGILL, right?

In softfloat mode __builtin_fabs() will just perform a bitwise and to remove the sign bit which will avoid depending on FP being available.
I made the change to use __builtin_fabs() for MIPS in D13135 and now we have the libc++ tests passing on MIPS softfloat as well.

As usual, the clang/llvm documentation ranges from murky to non-existant on this stuff. I did track down in the gcc docs that you can always explicitly call __builtin_foo() even when -fno-builtin and -ffreestanding are in effect (such as when we build libstand and loader(8)). Since clang tries to be gcc compatible, I guess we have to assume it will behave the same.

That being the case, I agree that the solution used for mips, that is, the implementation of fabs(x) is {return __builtin_fabs(x);}, seems exactly right.