[clang][driver] fix to correctly set devtoolset on RHEL

[quinnp]

Summary

This patch correctly sets the devtoolset on RHEL.

[nemanjai]

clang/lib/Driver/ToolChains/Gnu.cpp

2152

I believe this will cause a failure with the unit test added in the original patch.

And presumably since the unit test tests for paths that don't include /root/usr, there are real world toolsets that also don't use that suffix. So we should presumably add both Prefixes.

2157

Can we not just change this to something like:

if (ChosenToolsetVersion > 0) {
  Prefixes.push_back(ChosenToolsetDir);
  Prefixes.push_back(ChosenToolsetDir + "/root/usr");
}

[quinnp]

Addressing review comments.

[quinnp]

Thank you @nemanjai! I've updated the patch based on your suggestion and tested it for both the existing testcase and the RHEL buildbot failure.

[MaskRay]

Do you have more authoritative answer when /root/usr is used and when it isn't?
I'd wish that we have comments for them.

This change also needs a unit test.

[nemanjai]

In D127310#3567472, @MaskRay wrote:

Do you have more authoritative answer when /root/usr is used and when it isn't?

These suffixes were always part of the code and the mentioned changeset removed it without any justification. The burden of providing this answer should lie with the author of the change that removed this (i.e. @tbaeder). Here at IBM, we are not aware of any Redhat release that does not have this suffix. Namely, all of our RH PowerPC machines with all the toolset versions and distro versions we have available have the suffix. So we are ill equipped to answer this question. I assume that Timm is aware of situations where this isn't part of the path.

I'd wish that we have comments for them.

I agree 100%. Timm, do you know the answer to this and if so, can you propose a comment to that end?

This change also needs a unit test.

+1

[tbaeder]

In D127310#3568777, @nemanjai wrote:

In D127310#3567472, @MaskRay wrote:

Do you have more authoritative answer when /root/usr is used and when it isn't?

These suffixes were always part of the code and the mentioned changeset removed it without any justification. The burden of providing this answer should lie with the author of the change that removed this (i.e. @tbaeder). Here at IBM, we are not aware of any Redhat release that does not have this suffix. Namely, all of our RH PowerPC machines with all the toolset versions and distro versions we have available have the suffix. So we are ill equipped to answer this question. I assume that Timm is aware of situations where this isn't part of the path.

I did not remove that on purpose, so adding it back makes sense to me.

This change also needs a unit test.

+1

Does the existing unit test not break with this change?

[quinnp]

Addressing review comments. Removing path without /root/usr suffix and changing unit test to match.

[quinnp]

Thanks @tbaeder, @MaskRay, and @nemanjai.

I did not remove that on purpose, so adding it back makes sense to me.

I've updated the patch to remove the path without the /root/usr suffix and exclusively add the path with the root/usr suffix.

This change also needs a unit test.

I've updated the existing test case to match this change.

[MaskRay]

Looks great!

clang/lib/Driver/ToolChains/Gnu.cpp

for (llvm::vfs::directory_iterator LI = D.getVFS().dir_begin("/opt/rh", EC),
       if ((!ToolsetDir.startswith("gcc-toolset-") &&
            !ToolsetDir.startswith("devtoolset-")) ||
           ToolsetDir.substr(ToolsetDir.rfind('-') + 1)
               .getAsInteger(10, ToolsetVersion))
         continue;
 
       if (ToolsetVersion > ChosenToolsetVersion) {
         ChosenToolsetVersion = ToolsetVersion;
         ChosenToolsetDir = "/opt/rh/" + ToolsetDir.str();
       }
     }
 
     if (ChosenToolsetVersion > 0)
-      Prefixes.push_back(ChosenToolsetDir);
+      Prefixes.push_back(ChosenToolsetDir + "/root/usr");
   }
 
   // Fall back to /usr which is used by most non-Solaris systems.
   Prefixes.push_back(concat(SysRoot, "/usr"));
 }
 
 /*static*/ void Generic_GCC::GCCInstallationDetector::CollectLibDirsAndTriples(
     const llvm::Triple &TargetTriple, const llvm::Triple &BiarchTriple,

clang/unittests/Driver/ToolChainTest.cpp

// Check (newer) GCC toolset installation.
     InMemoryFileSystem->addFile("/opt/rh/gcc-toolset-", 0,
                                 llvm::MemoryBuffer::getMemBuffer("\n"));
     InMemoryFileSystem->addFile("/opt/rh/gcc-toolset--", 0,
                                 llvm::MemoryBuffer::getMemBuffer("\n"));
     InMemoryFileSystem->addFile("/opt/rh/gcc-toolset--1", 0,
                                 llvm::MemoryBuffer::getMemBuffer("\n"));
 
     // File needed for GCC installation detection.
-    InMemoryFileSystem->addFile(
-        "/opt/rh/gcc-toolset-12/lib/gcc/x86_64-redhat-linux/11/crtbegin.o", 0,
-        llvm::MemoryBuffer::getMemBuffer("\n"));
+    InMemoryFileSystem->addFile("/opt/rh/gcc-toolset-12/root/usr/lib/gcc/"
+                                "x86_64-redhat-linux/11/crtbegin.o",
+                                0, llvm::MemoryBuffer::getMemBuffer("\n"));
 
     DiagnosticsEngine Diags(DiagID, &*DiagOpts, new SimpleDiagnosticConsumer);
     Driver TheDriver("/bin/clang", "x86_64-redhat-linux", Diags,
                      "clang LLVM compiler", InMemoryFileSystem);
     std::unique_ptr<Compilation> C(
         TheDriver.BuildCompilation({"clang", "--gcc-toolchain="}));
     ASSERT_TRUE(C);
     std::string S;
     {
       llvm::raw_string_ostream OS(S);
       C->getDefaultToolChain().printVerboseInfo(OS);
     }
     EXPECT_EQ("Found candidate GCC installation: "
-              "/opt/rh/gcc-toolset-12/lib/gcc/x86_64-redhat-linux/11\n"
+              "/opt/rh/gcc-toolset-12/root/usr/lib/gcc/x86_64-redhat-linux/11\n"
               "Selected GCC installation: "
-              "/opt/rh/gcc-toolset-12/lib/gcc/x86_64-redhat-linux/11\n"
+              "/opt/rh/gcc-toolset-12/root/usr/lib/gcc/x86_64-redhat-linux/11\n"
               "Candidate multilib: .;@m64\n"
               "Selected multilib: .;@m64\n",
               S);
   }
 
   // And older devtoolset.
   {
     IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemoryFileSystem(
         new llvm::vfs::InMemoryFileSystem);
 
     // These should be ignored.
     InMemoryFileSystem->addFile("/opt/rh/devtoolset-2", 0,
                                 llvm::MemoryBuffer::getMemBuffer("\n"));
     InMemoryFileSystem->addFile("/opt/rh/devtoolset-", 0,
                                 llvm::MemoryBuffer::getMemBuffer("\n"));
     InMemoryFileSystem->addFile("/opt/rh/devtoolset--", 0,
                                 llvm::MemoryBuffer::getMemBuffer("\n"));
     InMemoryFileSystem->addFile("/opt/rh/devtoolset--1", 0,
                                 llvm::MemoryBuffer::getMemBuffer("\n"));
 
     // File needed for GCC installation detection.
-    InMemoryFileSystem->addFile(
-        "/opt/rh/devtoolset-12/lib/gcc/x86_64-redhat-linux/11/crtbegin.o", 0,
-        llvm::MemoryBuffer::getMemBuffer("\n"));
+    InMemoryFileSystem->addFile("/opt/rh/devtoolset-12/root/usr/lib/gcc/"
+                                "x86_64-redhat-linux/11/crtbegin.o",
+                                0, llvm::MemoryBuffer::getMemBuffer("\n"));
 
     DiagnosticsEngine Diags(DiagID, &*DiagOpts, new SimpleDiagnosticConsumer);
     Driver TheDriver("/bin/clang", "x86_64-redhat-linux", Diags,
                      "clang LLVM compiler", InMemoryFileSystem);
     std::unique_ptr<Compilation> C(
         TheDriver.BuildCompilation({"clang", "--gcc-toolchain="}));
     ASSERT_TRUE(C);
     std::string S;
     {
       llvm::raw_string_ostream OS(S);
       C->getDefaultToolChain().printVerboseInfo(OS);
     }
     EXPECT_EQ("Found candidate GCC installation: "
-              "/opt/rh/devtoolset-12/lib/gcc/x86_64-redhat-linux/11\n"
+              "/opt/rh/devtoolset-12/root/usr/lib/gcc/x86_64-redhat-linux/11\n"
               "Selected GCC installation: "
-              "/opt/rh/devtoolset-12/lib/gcc/x86_64-redhat-linux/11\n"
+              "/opt/rh/devtoolset-12/root/usr/lib/gcc/x86_64-redhat-linux/11\n"
               "Candidate multilib: .;@m64\n"
               "Selected multilib: .;@m64\n",
               S);
   }
 }
 
 } // end anonymous namespace.