NaCl ARM: fix assembler float abi flags

[dschuff]

Summary

tools::arm::getARMFloatABI() was falling back to guessing soft-float because
it wasn't seeing the GNUEABIHF environment from ComputeEffectivClangTriple
when it was called from gnutools::Assemble::ConstructJob.

Fix by using the effective clang triple in gnutools::Assemble, which now
matches the -triple flag used by cc1 and ClangAs jobs.

[dschuff]

Ping!

[jvoung]

Otherwise LGTM

lib/Driver/Tools.cpp

7401 ↗

(On Diff #23433)

Should this just set Triple = llvm::Triple(getToolChain().ComputeEffectiveClangTriple(Args)); here?

In case other stuff in this case statement depends on the triple later.

[dschuff]

lib/Driver/Tools.cpp

7401 ↗

(On Diff #23433)

That causes -mfpu=neon to be added to the linux build as well as NaCl (that causes the linux-as.c test to fail, which uses cortex-a8 as the cpu but does not expect -mfpu=neon). IIUC it should be ok to have -mfpu=neon (for both linux and nacl) because we always default to .fpu neon in the asm file when targeting cortex-a8. But that seems to mean that the assembler doesn't even care (at least for linux and nacl) whether it gets -mfpu=neon or not. I don't really understand why it should care anyway.

[rengolin]

The only thing I can think of that can happen, is that calculating the effective triple will then change it and possibly the default CPU (it can happen).

But since that code is a mess anyway, I don't think anyone can know beforehand. I'm happy to trust the tests, and if passing, feel free to commit.

We can catch the rest of the errors via buildbots or broken builds.

cheers,
--renato

lib/Driver/Tools.cpp

7401 ↗

(On Diff #23433)

Triple management in the Clang driver is composed of random non-abelian transformations. Don't try to understand it. :)

[dschuff]

ok, thanks, I'll commit it as uploaded here (without Jan's suggestion) since that passes all the existing tests and has the behavior we want for NaCl.

cfe/trunk/lib/Driver/Tools.cpp

case llvm::Triple::thumbeb: {
     case llvm::Triple::ARMSubArch_v8:
       CmdArgs.push_back("-mfpu=crypto-neon-fp-armv8");
       break;
     default:
       break;
     }
 
     StringRef ARMFloatABI = tools::arm::getARMFloatABI(
-        getToolChain().getDriver(), Args, Triple);
+        getToolChain().getDriver(), Args,
+        llvm::Triple(getToolChain().ComputeEffectiveClangTriple(Args)));
     CmdArgs.push_back(Args.MakeArgString("-mfloat-abi=" + ARMFloatABI));
 
     Args.AddLastArg(CmdArgs, options::OPT_march_EQ);
 
     // FIXME: remove krait check when GNU tools support krait cpu
     // for now replace it with -march=armv7-a  to avoid a lower
     // march from being picked in the absence of a cpu flag.
     Arg *A;

cfe/trunk/test/Driver/nacl-direct.c

 // CHECK-ARM: "-target-cpu" "cortex-a8"
 // CHECK-ARM: "-target-abi" "aapcs-linux"
 // CHECK-ARM: "-mfloat-abi" "hard"
 // CHECK-ARM: "-resource-dir" "foo"
 // CHECK-ARM: "-internal-isystem" "foo{{/|\\\\}}include"
 // CHECK-ARM: "-internal-isystem" "{{.*}}{{/|\\\\}}..{{/|\\\\}}arm-nacl{{/|\\\\}}usr{{/|\\\\}}include"
 // CHECK-ARM: "-internal-isystem" "{{.*}}{{/|\\\\}}..{{/|\\\\}}arm-nacl{{/|\\\\}}include"
 // CHECK-ARM: as{{(.exe)?}}"
+// CHECK-ARM: "-mfloat-abi=hard"
 // CHECK-ARM: ld{{(.exe)?}}"
 // CHECK-ARM: "--build-id"
 // CHECK-ARM: "-m" "armelf_nacl"
 // CHECK-ARM: "-static"
 // CHECK-ARM: "-L{{.*}}{{/|\\\\}}..{{/|\\\\}}arm-nacl{{/|\\\\}}lib"
 // CHECK-ARM: "-L{{.*}}{{/|\\\\}}..{{/|\\\\}}arm-nacl{{/|\\\\}}usr{{/|\\\\}}lib"
 // CHECK-ARM: "-Lfoo{{/|\\\\}}lib{{/|\\\\}}arm-nacl"
 // CHECK-ARM-NOT: -lpthread
 
 // Check that even when the target arch is just "arm" (as will be the case when
 // it is inferred from the binary name) that we get the right ABI flags
 // RUN: %clang -no-canonical-prefixes -### -o %t.o %s 2>&1 \
 // RUN:     -target arm-nacl \
 // RUN:   | FileCheck --check-prefix=CHECK-ARM-NOV7 %s
 // CHECK-ARM-NOV7: "-triple" "armv7--nacl-gnueabihf"
 // CHECK-ARM-NOV7: "-target-abi" "aapcs-linux"
 // CHECK-ARM-NOV7: "-mfloat-abi" "hard"
+// CHECK-ARM-NOV7: as{{(.exe)?}}"
+// CHECK-ARM-NOV7: "-mfloat-abi=hard"
 
 // Test clang c++ include dirs and link line when using clang++
 
 // RUN: %clangxx -no-canonical-prefixes -### -o %t.o %s \
 // RUN:     -target armv7a-unknown-nacl-gnueabihf -resource-dir foo 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-ARM-CXX %s
 // CHECK-ARM-CXX: {{.*}}clang{{.*}}" "-cc1"
 // CHECK-ARM-CXX: "-resource-dir" "foo"