[Driver] Perform Linux distribution detection just once

[dmantipov]

Summary

When running regular ('clang++ foo.cc') native compilation on Linux target in a typical configuration, an attempt to detect distribution type is performed 4 times at least:

• during an attempt to detect CUDA installation in clang::driver::CudaInstallationDetector::CudaInstallationDetector();

• during an instantiation of Linux-specific toolchain driver (clang::driver::toolchains::Linux::Linux());

• when special handling of '-faddrsig' is performed in clang::driver::tools::Clang::ConstructJob();

• finally when constructing a linker job in clang::driver::toolchains::Linux::getDynamicLinker().

Since distribution detection involves VFS, filesystem I/O and even parsing of file(s) contents, it looks desirable to cache the result and perform the whole procedure just once.

[dmantipov]

Add trivial {/etc,/usr/lib}/os-release parser and fix tests.

[sylvestre.ledru]

I guess it isn't possible to write tests for this?

Does it have an impact on cross compile ?

Also, please fix the clang-format warning!

clang/lib/Driver/Distro.cpp

229

you could probably do "return Distro::Un" here
(same line 230)

[aganea]

Please install & run git clang-format before uploading the patch.

clang/include/clang/Driver/Distro.h

27

You can leave out the -1 since this isn't serialized.

clang/lib/Driver/Distro.cpp

31–48

Early exit & move L23 to L32.

52–53

Early exit to avoid indent and ease reading:

if (!File)
  return Distro::UnknownDistro;

Also move L50 to L57.

207

I'm not sure if this is safe. @rnk Do we expect to call GetDistro with a different VFS or a different Triple when compiling with Clang? For GPU targets? Shouldn't we do this higher up, on the call site?

[rnk]

clang/lib/Driver/Distro.cpp

207

I think there is a plausible use case for a compile server that does two compiles with different VFSs where you would get different distro detection results. I think this should be a field on the Driver object instead of a global variable.

I know LLD uses lots of globals, but that's not part of the clang design philosophy.

[rnk]

clang/lib/Driver/Distro.cpp

207

Sorry, I spoke too soon.

For @aganea's concern, my understanding is that the distro is a property of the host machine doing the compilation. Even if we do two compiles in the same process on the same machine with two triples, they should have the same distro, if they are using the same real FS. At least, the FS shouldn't change out from under us in a way that changes the result of distro detection. :) I think it's reasonable to use distro detection to inform header search paths, but we should avoid using it to control properties of the output, like stack protection security features, for example. Otherwise, it becomes impossible to target one Linux distro from another, because the distro does not appear to be controllable by a command line flag.

And, now that I see what is happening with the real FS checks here, I withdraw my concern. Even if you did make this a property of the driver, I would want some global variable somewhere to check that we don't end up doing the distro check twice. It's the only way to really be sure that some passerby doesn't introduce a new distro check. =P

[dmantipov]

Updated (style adjustments).

[dmantipov]

What's next with this? Should I add more reviewers?

[rnk]

I think we can go forward with the reviewers we have. I have one more concern. Are the other reviewers happy?

clang/lib/Driver/Distro.cpp

207

I guess I have one more concern: this code isn't thread safe. It's unlikely to every be called from multiple threads, but let's use the safe code pattern anyway. You can use lazy static local initialization like so:

static ... GetDistro(...) {
  if (FS is Real FS && on Linux ...) {
    static const Distro::DistroType hostDistro = computeRealDistro();
    return hostDistro;
  }
  return computeDistroWithVFS();
}

[dmantipov]

IIUC compiler driver is not intended to be multithreaded. But OK, here is the version with llvm::call_once(...).

[aganea]

In D87187#2291806, @dmantipov wrote:

IIUC compiler driver is not intended to be multithreaded. But OK, here is the version with llvm::call_once(...).

Thanks! There could be at least two cases I see, where the driver could be multi-threaded:

1. once clang-cl /MP -fintegrated-cc1 is supported (D52193), I'm still planning to work on that, although in that case, it wouldn't reach this distro detection. However that patch could be extended to support clang ... -j.
2. Further, if the idea of llvm-buildozer ever lands (D86351) which in essence is a generalization of the above.

There's a slight issue with your last update, it now fails the tests (because several distros are checked one after another in the same process). You can perhaps leverage EXPECT_EXIT(..., ::testing::ExitedWithCode(0)) in the distro tests to run each of them in a separate process (see llvm/unittests/Support/ErrorTest.cpp for an example).
Also please run git clang-format.

[rnk]

llvm::call_once is fine, but IMO the static local version is preferable: it's built in to the language, no headers to include.

clang/lib/Driver/Distro.cpp

224

I think this would work and be simpler than call_once:

static Distro::DistroType LinuxDistro = DetectDistro(llvm::vfs::getRealFileSystem());

I could be wrong.

[dmantipov]

Well, the problem with tests seems to be a bit wider - tests uses llvm::vfs::InMemoryFileSystem, which is not "real", so detection will return Distro::UnknownDistro anyway. So I think we can just add fallback branch for such pseudo filesystems and live with current tests as is.

[aganea]

Makes sense, you cache the commonly taken path, but not the (very uncommon) VFS codepath.

LGTM, with some minor comments. Thanks again!

clang/include/clang/Driver/Distro.h

117

Please leave out code that doesn't participate in the patch. You can always commit NFC patches afterward with just clang-format changes if you wish. It's fine though if you move code around.

clang/lib/Driver/Distro.cpp

225

You can leave out the once_flag/call_once and simply apply Reid's suggestion, since they are equivalent (static initialization is thread safe [1]):

if (onRealFS) {
  static Distro::DistroType LinuxDistro = DetectDistro(VFS);
  return LinuxDistro;
}

[1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3797.pdf, section 6.7.4 "If control enters the declaration concurrently while the variable is being initialized, the concurrent execution shall wait for completion of the initialization"

clang/include/clang/Driver/Distro.h

 /// Distro - Helper class for detecting and classifying Linux distributions.
 ///
 /// This class encapsulates the clang Linux distribution detection mechanism
 /// as well as helper functions that match the specific (versioned) results
 /// into wider distribution classes.
 class Distro {
 public:
   enum DistroType {
+    // Special value means that no detection was performed yet.
+    UninitializedDistro,
     // NB: Releases of a particular Linux distro should be kept together
     // in this enum, because some tests are done by integer comparison against
     // the first and last known member in the family, e.g. IsRedHat().
     AlpineLinux,
     ArchLinux,
     DebianLenny,
     DebianSqueeze,
     DebianWheezy,
public:
   /// @}
   /// @name Convenience Predicates
   /// @{
 
   bool IsRedhat() const {
     return DistroVal == Fedora || (DistroVal >= RHEL5 && DistroVal <= RHEL7);
   }
 
-  bool IsOpenSUSE() const {
-    return DistroVal == OpenSUSE;
-  }
+  bool IsOpenSUSE() const { return DistroVal == OpenSUSE; }
 
   bool IsDebian() const {
     return DistroVal >= DebianLenny && DistroVal <= DebianBullseye;
   }
 
   bool IsUbuntu() const {
     return DistroVal >= UbuntuHardy && DistroVal <= UbuntuGroovy;
   }
 
-  bool IsAlpineLinux() const {
-    return DistroVal == AlpineLinux;
-  }
+  bool IsAlpineLinux() const { return DistroVal == AlpineLinux; }
 
-  bool IsGentoo() const {
-    return DistroVal == Gentoo;
-  }
+  bool IsGentoo() const { return DistroVal == Gentoo; }
 
   /// @}
 };
 
 } // end namespace driver
 } // end namespace clang
 
 #endif

clang/lib/Driver/Distro.cpp

 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Threading.h"
 
 using namespace clang::driver;
 using namespace clang;
 
-static Distro::DistroType DetectDistro(llvm::vfs::FileSystem &VFS,
-                                       const llvm::Triple &TargetOrHost) {
-  // If we don't target Linux, no need to check the distro. This saves a few
-  // OS calls.
-  if (!TargetOrHost.isOSLinux())
+static Distro::DistroType DetectOsRelease(llvm::vfs::FileSystem &VFS) {
+  llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> File =
+      VFS.getBufferForFile("/etc/os-release");
+  if (!File)
+    File = VFS.getBufferForFile("/usr/lib/os-release");
+  if (!File)
     return Distro::UnknownDistro;
 
-  // If the host is not running Linux, and we're backed by a real file system,
-  // no need to check the distro. This is the case where someone is
-  // cross-compiling from BSD or Windows to Linux, and it would be meaningless
-  // to try to figure out the "distro" of the non-Linux host.
-  IntrusiveRefCntPtr<llvm::vfs::FileSystem> RealFS =
-      llvm::vfs::getRealFileSystem();
-  llvm::Triple HostTriple(llvm::sys::getProcessTriple());
-  if (!HostTriple.isOSLinux() && &VFS == RealFS.get())
-    return Distro::UnknownDistro;
+  SmallVector<StringRef, 16> Lines;
+  File.get()->getBuffer().split(Lines, "\n");
+  Distro::DistroType Version = Distro::UnknownDistro;
+
+  // Obviously this can be improved a lot.
+  for (StringRef Line : Lines)
+    if (Version == Distro::UnknownDistro && Line.startswith("ID="))
+      Version = llvm::StringSwitch<Distro::DistroType>(Line.substr(3))
+                    .Case("fedora", Distro::Fedora)
+                    .Case("gentoo", Distro::Gentoo)
+                    .Case("arch", Distro::ArchLinux)
+                    // On SLES, /etc/os-release was introduced in SLES 11.
+                    .Case("sles", Distro::OpenSUSE)
+                    .Case("opensuse", Distro::OpenSUSE)
+                    .Case("exherbo", Distro::Exherbo)
+                    .Default(Distro::UnknownDistro);
+  return Version;
+}
 
+static Distro::DistroType DetectLsbRelease(llvm::vfs::FileSystem &VFS) {
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> File =
       VFS.getBufferForFile("/etc/lsb-release");
-  if (File) {
-    StringRef Data = File.get()->getBuffer();
-    SmallVector<StringRef, 16> Lines;
-    Data.split(Lines, "\n");
-    Distro::DistroType Version = Distro::UnknownDistro;
-    for (StringRef Line : Lines)
-      if (Version == Distro::UnknownDistro && Line.startswith("DISTRIB_CODENAME="))
-        Version = llvm::StringSwitch<Distro::DistroType>(Line.substr(17))
-                      .Case("hardy", Distro::UbuntuHardy)
-                      .Case("intrepid", Distro::UbuntuIntrepid)
-                      .Case("jaunty", Distro::UbuntuJaunty)
-                      .Case("karmic", Distro::UbuntuKarmic)
-                      .Case("lucid", Distro::UbuntuLucid)
-                      .Case("maverick", Distro::UbuntuMaverick)
-                      .Case("natty", Distro::UbuntuNatty)
-                      .Case("oneiric", Distro::UbuntuOneiric)
-                      .Case("precise", Distro::UbuntuPrecise)
-                      .Case("quantal", Distro::UbuntuQuantal)
-                      .Case("raring", Distro::UbuntuRaring)
-                      .Case("saucy", Distro::UbuntuSaucy)
-                      .Case("trusty", Distro::UbuntuTrusty)
-                      .Case("utopic", Distro::UbuntuUtopic)
-                      .Case("vivid", Distro::UbuntuVivid)
-                      .Case("wily", Distro::UbuntuWily)
-                      .Case("xenial", Distro::UbuntuXenial)
-                      .Case("yakkety", Distro::UbuntuYakkety)
-                      .Case("zesty", Distro::UbuntuZesty)
-                      .Case("artful", Distro::UbuntuArtful)
-                      .Case("bionic", Distro::UbuntuBionic)
-                      .Case("cosmic", Distro::UbuntuCosmic)
-                      .Case("disco", Distro::UbuntuDisco)
-                      .Case("eoan", Distro::UbuntuEoan)
-                      .Case("focal", Distro::UbuntuFocal)
-                      .Case("groovy", Distro::UbuntuGroovy)
-                      .Default(Distro::UnknownDistro);
-    if (Version != Distro::UnknownDistro)
-      return Version;
-  }
+  if (!File)
+    return Distro::UnknownDistro;
+
+  File.get()->getBuffer().split(Lines, "\n");
+
+    if (Version == Distro::UnknownDistro &&
+        Line.startswith("DISTRIB_CODENAME="))
 
-  File = VFS.getBufferForFile("/etc/redhat-release");
+static Distro::DistroType DetectDistro(llvm::vfs::FileSystem &VFS) {
+  Distro::DistroType Version = Distro::UnknownDistro;
+
+  // Newer freedesktop.org's compilant systemd-based systems
+  // should provide /etc/os-release or /usr/lib/os-release.
+  Version = DetectOsRelease(VFS);
+  if (Version != Distro::UnknownDistro)
+    return Version;
+
+  // Older systems might provide /etc/lsb-release.
+  Version = DetectLsbRelease(VFS);
+  if (Version != Distro::UnknownDistro)
+    return Version;
+
+  // Otherwise try some distro-specific quirks for RedHat...
+  llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> File =
+      VFS.getBufferForFile("/etc/redhat-release");
+
   if (File) {
     StringRef Data = File.get()->getBuffer();
     if (Data.startswith("Fedora release"))
       return Distro::Fedora;
     if (Data.startswith("Red Hat Enterprise Linux") ||
-        Data.startswith("CentOS") ||
-        Data.startswith("Scientific Linux")) {
+        Data.startswith("CentOS") || Data.startswith("Scientific Linux")) {
       if (Data.find("release 7") != StringRef::npos)
         return Distro::RHEL7;
       else if (Data.find("release 6") != StringRef::npos)
         return Distro::RHEL6;
       else if (Data.find("release 5") != StringRef::npos)
         return Distro::RHEL5;
     }
     return Distro::UnknownDistro;
   }
 
+  // ...for Debian
   File = VFS.getBufferForFile("/etc/debian_version");
   if (File) {
     StringRef Data = File.get()->getBuffer();
     // Contents: < major.minor > or < codename/sid >
     int MajorVersion;
     if (!Data.split('.').first.getAsInteger(10, MajorVersion)) {
       switch (MajorVersion) {
       case 5:
return llvm::StringSwitch<Distro::DistroType>(Data.split("\n").first)
         .Case("wheezy/sid", Distro::DebianWheezy)
         .Case("jessie/sid", Distro::DebianJessie)
         .Case("stretch/sid", Distro::DebianStretch)
         .Case("buster/sid", Distro::DebianBuster)
         .Case("bullseye/sid", Distro::DebianBullseye)
         .Default(Distro::UnknownDistro);
   }
 
+  // ...for SUSE
   File = VFS.getBufferForFile("/etc/SuSE-release");
   if (File) {
     StringRef Data = File.get()->getBuffer();
     SmallVector<StringRef, 8> Lines;
     Data.split(Lines, "\n");
-    for (const StringRef& Line : Lines) {
+    for (const StringRef &Line : Lines) {
       if (!Line.trim().startswith("VERSION"))
         continue;
       std::pair<StringRef, StringRef> SplitLine = Line.split('=');
       // Old versions have split VERSION and PATCHLEVEL
       // Newer versions use VERSION = x.y
-      std::pair<StringRef, StringRef> SplitVer = SplitLine.second.trim().split('.');
+      std::pair<StringRef, StringRef> SplitVer =
+          SplitLine.second.trim().split('.');
       int Version;
 
       // OpenSUSE/SLES 10 and older are not supported and not compatible
       // with our rules, so just treat them as Distro::UnknownDistro.
       if (!SplitVer.first.getAsInteger(10, Version) && Version > 10)
         return Distro::OpenSUSE;
       return Distro::UnknownDistro;
     }
     return Distro::UnknownDistro;
   }
 
+  // ...and others.
   if (VFS.exists("/etc/exherbo-release"))
     return Distro::Exherbo;
 
   if (VFS.exists("/etc/alpine-release"))
     return Distro::AlpineLinux;
 
   if (VFS.exists("/etc/arch-release"))
     return Distro::ArchLinux;
 
   if (VFS.exists("/etc/gentoo-release"))
     return Distro::Gentoo;
 
   return Distro::UnknownDistro;
 }
 
+static Distro::DistroType GetDistro(llvm::vfs::FileSystem &VFS,
+                                    const llvm::Triple &TargetOrHost) {
+  // If we don't target Linux, no need to check the distro. This saves a few
+  // OS calls.
+  if (!TargetOrHost.isOSLinux())
+    return Distro::UnknownDistro;
+
+  // True if we're backed by a real file system.
+  const bool onRealFS = (llvm::vfs::getRealFileSystem() == &VFS);
+
+  // If the host is not running Linux, and we're backed by a real file
+  // system, no need to check the distro. This is the case where someone
+  // is cross-compiling from BSD or Windows to Linux, and it would be
+  // meaningless to try to figure out the "distro" of the non-Linux host.
+  llvm::Triple HostTriple(llvm::sys::getProcessTriple());
+  if (!HostTriple.isOSLinux() && onRealFS)
+    return Distro::UnknownDistro;
+
+  if (onRealFS) {
+    static Distro::DistroType LinuxDistro = Distro::UninitializedDistro;
+    static llvm::once_flag DistroDetectFlag;
+    // If we're backed by a real file system, perform
+    // the detection only once and save the result.
+    llvm::call_once(DistroDetectFlag,
+                    [&VFS]() { LinuxDistro = DetectDistro(VFS); });
+    return LinuxDistro;
+  }
+  // This is mostly for passing tests which uses llvm::vfs::InMemoryFileSystem,
+  // which is not "real".
+  return DetectDistro(VFS);
+}
+
 Distro::Distro(llvm::vfs::FileSystem &VFS, const llvm::Triple &TargetOrHost)
-    : DistroVal(DetectDistro(VFS, TargetOrHost)) {}
+    : DistroVal(GetDistro(VFS, TargetOrHost)) {}