Allow the discovery of Android NDK's triple-prefixed binaries.

[brianpl]

Summary

Currently, tool discovery checks for a tool with its target triple as a
prefix. Unfortunately, the triple it uses is canonicalized, and may
contain the Android platform numbers as a suffix. This prevents matching
of the binaries in the LLVM bin directory of the Android NDK.

This CL adds checks during discovery that will match the canonical
Android NDK triples. The matcher will work with vendor "unknown", and at
any level platform.

[brianpl]

Fix a typo.

[danalbert]

Just to clarify, this is needed for the triple-prefixed tools, but the triple-specific directory worked fine before this patch? If I'm understanding that correctly then LGTM, otherwise I'm confused as to why I haven't seen this problem before.

[brianpl]

Just to clarify, this is needed for the triple-prefixed tools, but the triple-specific directory worked fine before this patch? If I'm understanding that correctly then LGTM, otherwise I'm confused as to why I haven't seen this problem before.

Yes, that's right. Without the patch, Clang will overlook the triple-prefixed tools, but it will correctly find tools in the triple-specific directory either way.

[jyknight]

clang/lib/Driver/Driver.cpp

4693

Adding the hardcoding here seems unfortunate. We have some code like this already in other places. It may be better to reuse the same logic. In particular, I'd expect we want to find program-prefixed names the same way as we did paths.

That seems to be done here https://github.com/llvm/llvm-project/blob/b6598bcf4b81ed8fb66a7c576a81e422750b9329/clang/lib/Driver/ToolChains/Linux.cpp#L236

While this function doesn't currently provide for a list of prefixes to be input, adding such a mechanism, and then having the code referenced above above add the prefix to the list, would seem better.

[brianpl]

• Search for tools prefixed with GNU target triples.
• Merge branch 'master' of https://github.com/llvm/llvm-project

[jyknight]

Since this change is not android-only, please update change description, and update some non-android tests.

E.g. maybe the tests in clang/test/Driver/linux-ld.c for ubuntu_14.04_multiarch_tree should check the path at which ld is found. While the existing Inputs/ubuntu_14.04_multiarch_tree doesn't have any "ld" files populated, and the tests aren't checking for ld's path at all, it would be reasonable to add such files and then check the result.

This is what the layout of the "ld" files look like in current ubuntu versions on x86_64. It seems that it was almost the same back in 2014 -- except the triple-less names were the primary file, and the others linked to them. That's not really relevant to the test, anyways, just the filenames existing and being executable.

lrwxrwxrwx  /usr/bin/ld.bfd -> x86_64-linux-gnu-ld.bfd
lrwxrwxrwx  /usr/bin/ld.gold -> x86_64-linux-gnu-ld.gold
lrwxrwxrwx  /usr/bin/ld -> x86_64-linux-gnu-ld
-rwxr-xr-x  /usr/bin/x86_64-linux-gnu-ld.bfd
-rwxr-xr-x  /usr/bin/x86_64-linux-gnu-ld.gold
-rwxr-xr-x  /usr/bin/x86_64-linux-gnu-ld -> x86_64-linux-gnu-ld.bfd
-rwxr-xr-x  /usr/bin/powerpc64le-linux-gnu-ld.bfd
-rwxr-xr-x  /usr/bin/powerpc64le-linux-gnu-ld.gold
-rwxr-xr-x  /usr/bin/powerpc64le-linux-gnu-ld -> powerpc64le-linux-gnu-ld.bfd

Before this clang patch, we would've defaulted to /usr/bin/ld unless the target string was specified exactly as "x86_64-linux-gnu" or "powerpc64le-linux-gnu". Afterwards, we can also chose those tools for any other triple which we can detect as matching those gcc installation triples.

This would even have the useful effect of allowing an invocation like "clang -target powerpc64le-unknown-linux-gnu" to work, which it doesn't do currently -- because it defaults to /usr/bin/ld, which is an x86_64 linker.

clang/lib/Driver/ToolChains/Gnu.cpp

2578

I suspect this doesn't actually work here, since GCCInstallation.init() hasn't yet been called. But you have the same in Linux.cpp, after the init() call, which seems better.

clang/lib/Driver/ToolChains/Linux.cpp

242

Probably should check that GCCInstallation.getTriple() != Triple before adding, in order to avoid duplicating entries in the search-list.

clang/test/Driver/android-triple-version.c

1

This seems like too many test-cases. It doesn't seem to me that they're actually adding any meaningful coverage, after the first 3?

6

You shouldn't edit files in the Inputs tree (or add/remove files there either). If this script stops, it may be left in an edited state, which would be bad. Also, sometimes it might be read-only.

Another way to achieve this test goal may be with -fuse-ld=prefixtest, and then create bin/$triple-ld.prefixtest, but not $triple/bin/ld.prefixtest

clang/include/clang/Driver/ToolChain.h

bool isEmpty() const {
     return TargetPrefix.empty() && ModeSuffix.empty() && DriverMode == nullptr;
   }
 };
 
 /// ToolChain - Access to tools for a single platform.
 class ToolChain {
 public:
   using path_list = SmallVector<std::string, 16>;
+  using prefix_list = SmallVector<std::string, 16>;
 
   enum CXXStdlibType {
     CST_Libcxx,
     CST_Libstdcxx
   };
 
   enum RuntimeLibType {
     RLT_CompilerRT,
private:
   path_list LibraryPaths;
 
   /// The list of toolchain specific path prefixes to search for files.
   path_list FilePaths;
 
   /// The list of toolchain specific path prefixes to search for programs.
   path_list ProgramPaths;
 
+  /// The list of toolchain specific name prefixes to search for programs.
+  prefix_list ProgramPrefixes;
+
   mutable std::unique_ptr<Tool> Clang;
   mutable std::unique_ptr<Tool> Flang;
   mutable std::unique_ptr<Tool> Assemble;
   mutable std::unique_ptr<Tool> Link;
   mutable std::unique_ptr<Tool> IfsMerge;
   mutable std::unique_ptr<Tool> OffloadBundler;
   mutable std::unique_ptr<Tool> OffloadWrapper;
 
public:
   const path_list &getLibraryPaths() const { return LibraryPaths; }
 
   path_list &getFilePaths() { return FilePaths; }
   const path_list &getFilePaths() const { return FilePaths; }
 
   path_list &getProgramPaths() { return ProgramPaths; }
   const path_list &getProgramPaths() const { return ProgramPaths; }
 
+  prefix_list &getProgramPrefixes() { return ProgramPrefixes; }
+  const prefix_list &getProgramPrefixes() const { return ProgramPrefixes; }
+
   const MultilibSet &getMultilibs() const { return Multilibs; }
 
   const Multilib &getMultilib() const { return SelectedMultilib; }
 
   const SanitizerArgs& getSanitizerArgs() const;
 
   const XRayArgs& getXRayArgs() const;
 

clang/lib/Driver/Driver.cpp

 //===--- Driver.cpp - Clang GCC Compatible Driver -------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
-
 #include "clang/Driver/Driver.h"
 #include "InputInfo.h"
 #include "ToolChains/AIX.h"
 #include "ToolChains/AMDGPU.h"
 #include "ToolChains/AVR.h"
 #include "ToolChains/Ananas.h"
 #include "ToolChains/BareMetal.h"
 #include "ToolChains/Clang.h"
std::string Driver::GetFilePath(StringRef Name, const ToolChain &TC) const {
   return Name;
 }
 
 void Driver::generatePrefixedToolNames(
     StringRef Tool, const ToolChain &TC,
     SmallVectorImpl<std::string> &Names) const {
   // FIXME: Needs a better variable than TargetTriple
   Names.emplace_back((TargetTriple + "-" + Tool).str());
+  for(std::string PPrefix : TC.getProgramPrefixes()) {
+    Names.emplace_back((PPrefix + Tool).str());
+  }
   Names.emplace_back(Tool);
-
-  // Allow the discovery of tools prefixed with LLVM's default target triple.
-  std::string DefaultTargetTriple = llvm::sys::getDefaultTargetTriple();
-  if (DefaultTargetTriple != TargetTriple)
-    Names.emplace_back((DefaultTargetTriple + "-" + Tool).str());
 }
 
 static bool ScanDirForExecutable(SmallString<128> &Dir,
                                  ArrayRef<std::string> Names) {
   for (const auto &Name : Names) {
     llvm::sys::path::append(Dir, Name);
     if (llvm::sys::fs::can_execute(Twine(Dir)))
       return true;
     llvm::sys::path::remove_filename(Dir);
   }
   return false;
 }

clang/lib/Driver/ToolChains/Gnu.cpp

 //===--- Gnu.cpp - Gnu Tool and ToolChain Implementations -------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
-
 #include "Gnu.h"
 #include "Arch/ARM.h"
 #include "Arch/Mips.h"
 #include "Arch/PPC.h"
 #include "Arch/RISCV.h"
 #include "Arch/Sparc.h"
 #include "Arch/SystemZ.h"
 #include "CommonArgs.h"

 
 Generic_GCC::Generic_GCC(const Driver &D, const llvm::Triple &Triple,
                          const ArgList &Args)
     : ToolChain(D, Triple, Args), GCCInstallation(D),
       CudaInstallation(D, Triple, Args) {
   getProgramPaths().push_back(getDriver().getInstalledDir());
   if (getDriver().getInstalledDir() != getDriver().Dir)
     getProgramPaths().push_back(getDriver().Dir);
+  getProgramPrefixes().push_back(Twine(GCCInstallation.getTriple().str() + "-").str());
+
 }
 
 Generic_GCC::~Generic_GCC() {}
 
 Tool *Generic_GCC::getTool(Action::ActionClass AC) const {
   switch (AC) {
   case Action::PreprocessJobClass:
     if (!Preprocess)

clang/lib/Driver/ToolChains/Linux.cpp

Linux::Linux(const Driver &D, const llvm::Triple &Triple, const ArgList &Args)
   // least) put various tools in a triple-prefixed directory off of the parent
   // of the GCC installation. We use the GCC triple here to ensure that we end
   // up with tools that support the same amount of cross compiling as the
   // detected GCC installation. For example, if we find a GCC installation
   // targeting x86_64, but it is a bi-arch GCC installation, it can also be
   // used to target i386.
   // FIXME: This seems unlikely to be Linux-specific.
   ToolChain::path_list &PPaths = getProgramPaths();
+  ToolChain::prefix_list &PPrefixes = getProgramPrefixes();
   if (GCCInstallation.isValid()) {
     PPaths.push_back(Twine(GCCInstallation.getParentLibPath() + "/../" +
                            GCCInstallation.getTriple().str() + "/bin")
                          .str());
+    PPrefixes.push_back(Twine(GCCInstallation.getTriple().str() + "-").str());
   }
 
   Distro Distro(D.getVFS(), Triple);
 
   if (Distro.IsAlpineLinux() || Triple.isAndroid()) {
     ExtraOpts.push_back("-z");
     ExtraOpts.push_back("now");
   }

clang/test/Driver/android-triple-version.c

+// Android's target triples can contain a version number in the environment
+// field (e.g. arm-linux-androideabi9).
+// Make sure that any version is stripped when finding toolchain binaries.
+
+// Ensure no execute permissions on .../bin/{target-triple}/ld.
+// RUN: chmod -x %S/Inputs/basic_android_ndk_tree/arm-linux-androideabi/bin/ld
+// RUN: chmod -x %S/Inputs/basic_android_ndk_tree/aarch64-linux-android/bin/ld
+// RUN: chmod -x %S/Inputs/basic_android_ndk_tree/i686-linux-android/bin/ld
+// RUN: chmod -x %S/Inputs/basic_android_ndk_tree/mipsel-linux-android/bin/ld
+
+// Ensure execute permissions on .../bin/{target-triple}-ld
+// RUN: chmod +x  %S/Inputs/basic_android_ndk_tree/bin/arm-linux-androideabi-ld
+// RUN: chmod +x  %S/Inputs/basic_android_ndk_tree/bin/aarch64-linux-android-ld
+// RUN: chmod +x %S/Inputs/basic_android_ndk_tree/bin/i686-linux-android-ld
+// RUN: chmod +x %S/Inputs/basic_android_ndk_tree/bin/mipsel-linux-android-ld
+
+// Link clang into the mock NDK.
+// RUN: rm -f %S/Inputs/basic_android_ndk_tree/bin/clang
+// RUN: ln -s %clang %S/Inputs/basic_android_ndk_tree/bin/clang
+
+// Test target arm-linux-androideabi
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target arm-linux-androideabi %s |& \
+// RUN: grep arm-linux-androideabi-ld
+
+// Test target arm-unknown-linux-androideabi
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target arm-unknown-linux-androideabi %s |& \
+// RUN: grep arm-linux-androideabi-ld
+
+// Test target arm-linux-androideabi14
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target arm-linux-androideabi14 %s |& \
+// RUN: grep arm-linux-androideabi-ld
+
+// Test target arm-linux-androideabi29
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target arm-linux-androideabi29 %s |& \
+// RUN: grep arm-linux-androideabi-ld
+
+// Test target arm-unknown-linux-androideabi14
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target arm-unknown-linux-androideabi14 %s |& \
+// RUN: grep arm-linux-androideabi-ld
+
+// Test target arm-unknown-linux-androideabi29
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target arm-unknown-linux-androideabi29 %s |& \
+// RUN: grep arm-linux-androideabi-ld
+
+// Test target aarch64-linux-android
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target aarch64-linux-android %s |& \
+// RUN: grep aarch64-linux-android-ld
+
+// Test target aarch64-unknown-linux-android
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target aarch64-unknown-linux-android %s |& \
+// RUN: grep aarch64-linux-android-ld
+
+// Test target aarch64-linux-android14
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target aarch64-linux-android14 %s |& \
+// RUN: grep aarch64-linux-android-ld
+
+// Test target aarch64-linux-android29
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target aarch64-linux-android29 %s |& \
+// RUN: grep aarch64-linux-android-ld
+
+// Test target aarch64-unknown-linux-android14
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target aarch64-unknown-linux-android14 %s |& \
+// RUN: grep aarch64-linux-android-ld
+
+// Test target aarch64-unknown-linux-android29
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target aarch64-unknown-linux-android29 %s |& \
+// RUN: grep aarch64-linux-android-ld
+
+// Test target i686-linux-android
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target i686-linux-android %s |& \
+// RUN: grep i686-linux-android-ld
+
+// Test target i686-unknown-linux-android
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target i686-unknown-linux-android %s |& \
+// RUN: grep i686-linux-android-ld
+
+// Test target i686-linux-android14
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target i686-linux-android14 %s |& \
+// RUN: grep i686-linux-android-ld
+
+// Test target i686-linux-android29
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target i686-linux-android29 %s |& \
+// RUN: grep i686-linux-android-ld
+
+// Test target i686-unknown-linux-android14
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target i686-unknown-linux-android14 %s |& \
+// RUN: grep i686-linux-android-ld
+
+// Test target i686-unknown-linux-android29
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target i686-unknown-linux-android29 %s |& \
+// RUN: grep i686-linux-android-ld
+
+// Test target mipsel-linux-android
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target mipsel-linux-android %s |& \
+// RUN: grep mipsel-linux-android-ld
+
+// Test target mipsel-unknown-linux-android
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target mipsel-unknown-linux-android %s |& \
+// RUN: grep mipsel-linux-android-ld
+
+// Test target mipsel-linux-android14
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target mipsel-linux-android14 %s |& \
+// RUN: grep mipsel-linux-android-ld
+
+// Test target mipsel-linux-android29
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target mipsel-linux-android29 %s |& \
+// RUN: grep mipsel-linux-android-ld
+
+// Test target mipsel-unknown-linux-android14
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target mipsel-unknown-linux-android14 %s |& \
+// RUN: grep mipsel-linux-android-ld
+
+// Test target mipsel-unknown-linux-android29
+// RUN: env "PATH=%S/Inputs/basic_android_ndk_tree/bin" \
+// RUN:     %S/Inputs/basic_android_ndk_tree/bin/clang \
+// RUN:     -### -target mipsel-unknown-linux-android29 %s |& \
+// RUN: grep mipsel-linux-android-ld
+
+// Ensure no execute permissions on .../bin/{target-triple}-ld
+// RUN: chmod -x %S/Inputs/basic_android_ndk_tree/bin/arm-linux-androideabi-ld
+// RUN: chmod -x %S/Inputs/basic_android_ndk_tree/bin/aarch64-linux-android-ld
+// RUN: chmod -x %S/Inputs/basic_android_ndk_tree/bin/i686-linux-android-ld
+// RUN: chmod -x %S/Inputs/basic_android_ndk_tree/bin/mipsel-linux-android-ld
+
+// Unlink clang from the mock NDK.
+// RUN: rm -f %S/Inputs/basic_android_ndk_tree/bin/clang
 No newline at end of file