[AVR][clang] Improve search for avr-libc installation path

[benshi001]

Summary

Search avr-libc path according to avr-gcc installation at first,
then other possible installed pathes.

[benshi001]

There are already test cases

$llvm/clang/test/Driver/avr-ld.c
$llvm/clang/test/Driveravr-toolchain.c

and fake avr-gcc inside $llvm/clang/test/Driver/Inputs/basic_avr_tree to support the above two tests.

So there is no need to add new ones.

[mhjacobson]

clang/lib/Driver/ToolChains/AVR.cpp

457

Would it be worth encapsulating this logic in an accessor method on GCCInstallation?

[benshi001]

Thanks. I have added some comments about our agreed logic, as discussed in https://reviews.llvm.org/D107672.

I think this way is clear enough.

[benshi001]

@mhjacobson Could you please check if my modification also works as expected on your platform ?

[mhjacobson]

In D107682#2932599, @benshi001 wrote:

@mhjacobson Could you please check if my modification also works as expected on your platform ?

Hm... it doesn't, because there aren't enough ..s.

Here's what my directory tree looks like. My gcc install consists of:

/opt/local/bin/avr-gcc-10.3.0
/opt/local/lib/gcc/avr/10.3.0/libgcc.a
/opt/local/libexec/gcc/avr/10.3.0/cc1

And avr-libc is at:

/opt/local/avr/lib/libc.a
/opt/local/avr/include/stdio.h

It would work if you appended four ..s (which is effectively what GCC does, since it backs up to the "prefix", which in my case is /opt/local/).

Edit: my setup seems to match the one shown in this presentation (see slides titled "Toolchain contents").

Edit 2: it also matches this, namely /opt/local/avr/lib is $(tooldir)/lib

[mhjacobson]

For other examples of using four ..s, look in Gnu.cpp, where there are several places where getParentLibPath() is appended with /../. AddMultilibPaths(), PushPPaths().

getParentLibPath(), in turn, is GCCInstallPath/../../../

[benshi001]

In D107682#2932607, @mhjacobson wrote:

For other examples of using four ..s, look in Gnu.cpp, where there are several places where getParentLibPath() is appended with /../. AddMultilibPaths(), PushPPaths().

getParentLibPath(), in turn, is GCCInstallPath/../../../

Thanks for your help. I have updated my patch. Now it should work for both your platform and mine. It search GCCRoot.getParentLibPath()/avr first, otherwise GCCRoot.getParentLibPath()/../avr.

Please help me check.

[MaskRay]

clang/lib/Driver/ToolChains/AVR.cpp

460

return Path;

clang/test/Driver/avr-toolchain.c

2

You can UNSUPPORT windows if you don't want to wrestle with \ and / path separators.

2

UNSUPPORTED: system-windows

[mhjacobson]

In D107682#2932685, @benshi001 wrote:

In D107682#2932607, @mhjacobson wrote:

For other examples of using four ..s, look in Gnu.cpp, where there are several places where getParentLibPath() is appended with /../. AddMultilibPaths(), PushPPaths().

getParentLibPath(), in turn, is GCCInstallPath/../../../

Thanks for your help. I have updated my patch. Now it should work for both your platform and mine. It search GCCRoot.getParentLibPath()/avr first, otherwise GCCRoot.getParentLibPath()/../avr.

Please help me check.

Latest diff works for me, thanks!

[benshi001]

@MaskRay

I have update my patch based on your new test avr-toolchain.c.

Thank you!

[MaskRay]

Seems a new RUN line is needed. If having too many mock trees are inconvenient, consider clang/unittests/Driver/ToolChainTest.cpp

[benshi001]

In D107682#2936040, @MaskRay wrote:

Seems a new RUN line is needed. If having too many mock trees are inconvenient, consider clang/unittests/Driver/ToolChainTest.cpp

I am not familiar with the way in clang/unittests/Driver/ToolChainTest.cpp, I always use clang command line than libcall. Though I have created a new test TEST(ToolChainTest, VFSAVRInstallation) by imitating the existing TEST(ToolChainTest, VFSGCCInstallation), it did not work as I expected. So I created a new mock tree basic_avr_tree_2.

And the tests in avr-toolchain.cc should cover all three possible cases

1. find avr-libc at AVR-GCC/../../../avr (the case --check-prefix=CHECK1)
2. find avr-libc at AVR-GCC/../../../../avr (the case --check-prefix=CHECK2)
3. there is not avr-gcc and find avr-libc at SYSROOT/usr/avr (the case --check-prefix=CHECK3)

[benshi001]

The file/directory structure in basic_avr_tree_2/opt/local is the same as mhjacobson's machine.

basic_avr_tree_2/opt/local/bin/avr-gcc-10.3.0
basic_avr_tree_2/opt/local/lib/gcc/avr/10.3.0/libgcc.a

basic_avr_tree_2/opt/local/avr/
basic_avr_tree_2/opt/local/avr/lib/libc.a
basic_avr_tree_2/opt/local/avr/include/.keep

The above is for CHECK2, and there is an avr-libc only structure for CHECK3.

basic_avr_tree_2/usr/avr/
basic_avr_tree_2/usr/avr/lib/libc.a
basic_avr_tree_2/usr/avr/include/.keep

clang/lib/Driver/ToolChains/AVR.cpp

void AVR::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   }
 
   C.addCommand(std::make_unique<Command>(
       JA, *this, ResponseFileSupport::AtFileCurCP(), Args.MakeArgString(Linker),
       CmdArgs, Inputs, Output));
 }
 
 llvm::Optional<std::string> AVRToolChain::findAVRLibcInstallation() const {
+  // Search avr-libc installation according to avr-gcc installation.
+  std::string GCCParent(GCCInstallation.getParentLibPath());
+  std::string Path(GCCParent + "/avr");
+  if (llvm::sys::fs::is_directory(Path))
+    return Path;
+  Path = GCCParent + "/../avr";
+  if (llvm::sys::fs::is_directory(Path))
+    return Path;
+
+  // Search avr-libc installation from possible locations, and return the first
+  // one that exists, if there is no avr-gcc installed.
   for (StringRef PossiblePath : PossibleAVRLibcLocations) {
     std::string Path = getDriver().SysRoot + PossiblePath.str();
-    // Return the first avr-libc installation that exists.
     if (llvm::sys::fs::is_directory(Path))
-      return Optional<std::string>(Path);
+      return Path;
   }
 
   return llvm::None;
 }

clang/test/Driver/avr-toolchain.c

 // UNSUPPORTED: system-windows
 // A basic clang -cc1 command-line.
 
 // RUN: %clang %s -### -target avr --sysroot %S/Inputs/basic_avr_tree -resource-dir=%S/Inputs/resource_dir 2>&1 | FileCheck --check-prefix=CHECK1 %s
 // CHECK1: clang{{.*}} "-cc1" "-triple" "avr"
 // CHECK1-SAME: "-resource-dir" "[[RESOURCE:[^"]+]]"
 // CHECK1-SAME: "-isysroot" "[[SYSROOT:[^"]+/basic_avr_tree]]"
 // CHECK1-SAME: "-internal-isystem"
-// CHECK1-SAME: {{^}} "[[SYSROOT]]/usr/lib/avr/include"
+// CHECK1-SAME: {{^}} "[[SYSROOT]]/usr/lib/gcc/avr/5.4.0/../../../avr/include"
 // CHECK1-NOT:  "-L
 // CHECK1:      avr-ld"
 // CHECK1-SAME: "-o" "a.out"
 // CHECK1-SAME: {{^}} "--gc-sections"
 
+// RUN: %clang %s -### -target avr --sysroot %S/Inputs/basic_avr_tree_2/opt/local -S 2>&1 | FileCheck --check-prefix=CHECK2 %s
+// CHECK2: clang{{.*}} "-cc1" "-triple" "avr"
+// CHECK2-SAME: "-isysroot" "[[SYSROOT:[^"]+/basic_avr_tree_2/opt/local]]"
+// CHECK2-SAME: "-internal-isystem"
+// CHECK2-SAME: {{^}} "[[SYSROOT]]/lib/gcc/avr/10.3.0/../../../../avr/include"
+
+// RUN: %clang %s -### -target avr --sysroot %S/Inputs/basic_avr_tree_2 -S 2>&1 | FileCheck --check-prefix=CHECK3 %s
+// CHECK3: clang{{.*}} "-cc1" "-triple" "avr"
+// CHECK3-SAME: "-isysroot" "[[SYSROOT:[^"]+/basic_avr_tree_2]]"
+// CHECK3-SAME: "-internal-isystem"
+// CHECK3-SAME: {{^}} "[[SYSROOT]]/usr/avr/include"
+
 // RUN: %clang %s -### -target avr 2>&1 | FileCheck -check-prefix=CC1 %s
 // CC1: clang{{.*}} "-cc1" "-triple" "avr" {{.*}} "-fno-use-init-array"
 
 // RUN: %clang %s -### -target avr --sysroot %S/Inputs/basic_avr_tree 2>&1 -nostdinc | FileCheck --check-prefix=NOSTDINC %s
 // RUN: %clang %s -### -target avr --sysroot %S/Inputs/basic_avr_tree 2>&1 -nostdlibinc | FileCheck --check-prefix=NOSTDINC %s
 // NOSTDINC-NOT: "-internal-isystem" {{".*avr/include"}}