[Driver] Default to libc++ on FreeBSD

[emaste]

Summary

Downstream may naively translate between DSL and LLVM target triple. If OS version is lost in the process then Clang would default to a version that's no longer supported by OS vendor.

Example: https://bugzilla.mozilla.org/show_bug.cgi?id=1628567

$ cat a.cc
#include <type_traits>

$ clang++ -c a.cc
$ clang++ --target=x86_64-unknown-freebsd -c a.cc
a.cc:1:10: fatal error: 'type_traits' file not found
#include <type_traits>
         ^~~~~~~~~~~~~

[jbeich]

• Attempt to unbreak tests

[arichardson]

Alternatively, the checks could be changed to also handle OSMajorVersion == 0 and translate that to 10.
This seems to be what NetBSD.cpp does. Darwin.cpp also infers the version from the host when running on macos.

[dim]

In D77776#1971543, @arichardson wrote:

Alternatively, the checks could be changed to also handle OSMajorVersion == 0 and translate that to 10.
This seems to be what NetBSD.cpp does. Darwin.cpp also infers the version from the host when running on macos.

Yeah, I think that is much better. If people want to explicitly target foobar-freebsd9, they should be free to do so. But the default should indeed be either the host version, or if that cannot be determined, the lowest supported version, which is currently 10. For these particular features the exact version doesn't matter that much though, it's just about whether libc++ should be the default.

[myfreeweb]

But the default should indeed be either the host version

yep, I've had this patch applied (wrote yesterday to fix building Rust in Ports without seeing this issue, haha):

--- lib/Support/Triple.cpp.orig	2020-04-10 20:51:46 UTC
+++ lib/Support/Triple.cpp
@@ -14,6 +14,9 @@
 #include "llvm/Support/Host.h"
 #include "llvm/Support/TargetParser.h"
 #include <cstring>
+#ifdef __FreeBSD__
+#include <sys/param.h>
+#endif
 using namespace llvm;
 
 StringRef Triple::getArchTypeName(ArchType Kind) {
@@ -1073,6 +1076,13 @@ void Triple::getOSVersion(unsigned &Major, unsigned &M
     OSName.consume_front("macos");
 
   parseVersionFromName(OSName, Major, Minor, Micro);
+
+  if (getOS() == FreeBSD && Major == 0)
+#ifdef __FreeBSD__
+	  Major = __FreeBSD_version / 100000;
+#else
+	  Major = 12;
+#endif
 }
 
 bool Triple::getMacOSXVersion(unsigned &Major, unsigned &Minor,

I don't see any other OS headers included in this file, so idk about code style, but the general idea should be like this.

// btw, reported to cc-rs a while ago https://github.com/alexcrichton/cc-rs/issues/463 and came to the conclusion that this unversioned target must be coming from Ports somewhere o_0

[jbeich]

• Limit the scope to -stdlib

In D77776#1972543, @dim wrote:

the lowest supported version, which is currently 10

Where is this defined? Does someone build LLVM master on FreeBSD < 11.3?

ports/ (for dependencies) quickly cut off EOL support to avoid bitrot due to lack of QA.

[emaste]

In D77776#1981271, @jbeich wrote:

• Limit the scope to -stdlib

In D77776#1972543, @dim wrote:

the lowest supported version, which is currently 10

Where is this defined? Does someone build LLVM master on FreeBSD < 11.3?

Not certain, but there are a number of FreeBSD consumers still shipping products based on FreeBSD 10 who might be trying to use newer toolchains. I don't think we need to be too concerned about those cases but if it's trivial to avoid breaking them we might as well.

[emaste]

Looks ok to me

[arichardson]

I don't like the fact that this only changes one of the users of getTriple().getOSMajorVersion(). Could you add a new member function such as

void FreeBSD::getMajorVersion() const {
  unsigned Major = getTriple().getOSMajorVersion();
  if (Major == 0)
     return 10; 
  return Major
}

and replace all uses of getTriple().getOSMajorVersion() with getMajorVersion().
We could also use the host version instead of 10?

+#ifdef __FreeBSD__
+	  return __FreeBSD_version / 100000;
+#else
+	  return 10;
+#endif

[myfreeweb]

ping. can we get *some* solution to this included in llvm 11.0 final release?

[dim]

In D77776#1993211, @arichardson wrote:

I don't like the fact that this only changes one of the users of getTriple().getOSMajorVersion().

Why not, if this is a one-off case, it's perfectly OK to put it in here. Maybe add a comment as to why it is needed?

Could you add a new member function such as

void FreeBSD::getMajorVersion() const {
  unsigned Major = getTriple().getOSMajorVersion();
  if (Major == 0)
     return 10; 
  return Major
}

and replace all uses of getTriple().getOSMajorVersion() with getMajorVersion().

Maybe other OSes would also have this same issue, but then you'd have to replace this kind of logic for *all* of them, not only FreeBSD. That said, maybe there aren't so many places where the major version is checked, and where features are enabled or disabled depending on this version?

We could also use the host version instead of 10?

+#ifdef __FreeBSD__
+	  return __FreeBSD_version / 100000;
+#else
+	  return 10;
+#endif

No, that would hardcode something at build time. We need to respect whatever triple the user is passing in.

[myfreeweb]

In D77776#2187273, @dim wrote:

+#ifdef __FreeBSD__
+	  return __FreeBSD_version / 100000;
+#else
+	  return 10;
+#endif

No, that would hardcode something at build time. We need to respect whatever triple the user is passing in.

Nobody has proposed not respecting that, this is all in the context of the if (Major == 0) fallback

[dim]

In D77776#2187334, @myfreeweb wrote:

In D77776#2187273, @dim wrote:

+#ifdef __FreeBSD__
+	  return __FreeBSD_version / 100000;
+#else
+	  return 10;
+#endif

No, that would hardcode something at build time. We need to respect whatever triple the user is passing in.

Nobody has proposed not respecting that, this is all in the context of the if (Major == 0) fallback

Aha, then there is no need to do arithmetic with __FreeBSD_version, as __FreeBSD__ already contains just the major version.

[myfreeweb]

It's been over a year and no solutions have been merged! The problem just popped up again in Firefox after a pull. With clang 13.0, and the discussion above was before 11.0 release >_<

Can someone commit something already?!

[emaste]

rebase tests

[emaste]

clang/test/Driver/freebsd.cpp

13–14

In main this is amd64-unknown-freebsd14.0, I have a fix for that staged before this diff

commit d99411fe6e0375041b80ba4f397a139218a3754d
Author: Ed Maste <[e-mail]>
Date:   Mon Nov 22 11:26:55 2021 -0500

    [Driver] correct typo in FreeBSD 14 test
    
    The test specified amd64-unknown-freebsd40.0 rather than 14.0.  Since
    40 is greater than 14 the test (for behaviour new in FreeBSD 14) worked
    despite the typo.
    
    Fixes:          699d47472c3f

diff --git a/clang/test/Driver/freebsd.cpp b/clang/test/Driver/freebsd.cpp
index fde888902e12..d199f6e2367a 100644
--- a/clang/test/Driver/freebsd.cpp
+++ b/clang/test/Driver/freebsd.cpp
@@ -5,7 +5,7 @@
 // CHECK-TEN: "-lc++" "-lm"
 // CHECK-NINE: "-lstdc++" "-lm"
 
-// RUN: %clangxx %s -### -pg -o %t.o -target amd64-unknown-freebsd40.0 -stdlib=platform 2>&1 \
+// RUN: %clangxx %s -### -pg -o %t.o -target amd64-unknown-freebsd14.0 -stdlib=platform 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-PG-FOURTEEN %s
 // RUN: %clangxx %s -### -pg -o %t.o -target amd64-unknown-freebsd10.0 -stdlib=platform 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-PG-TEN %s

[dim]

LGTM

clang/lib/Driver/ToolChains/FreeBSD.cpp

if ((Triple.getArch() == llvm::Triple::x86 || Triple.isMIPS32() ||
        Triple.isPPC32()) &&
       D.getVFS().exists(getDriver().SysRoot + "/usr/lib32/crt1.o"))
     getFilePaths().push_back(getDriver().SysRoot + "/usr/lib32");
   else
     getFilePaths().push_back(getDriver().SysRoot + "/usr/lib");
 }
 
 ToolChain::CXXStdlibType FreeBSD::GetDefaultCXXStdlibType() const {
-  if (getTriple().getOSMajorVersion() >= 10)
+  unsigned Major = getTriple().getOSMajorVersion();
+  if (Major >= 10 || Major == 0)
     return ToolChain::CST_Libcxx;
   return ToolChain::CST_Libstdcxx;
 }
 
 unsigned FreeBSD::GetDefaultDwarfVersion() const {
   if (getTriple().getOSMajorVersion() < 12)
     return 2;
   return 4;

clang/test/Driver/freebsd.cpp

+// RUN: %clangxx %s -### -o %t.o -target amd64-unknown-freebsd -stdlib=platform 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-DEFAULT %s
 // RUN: %clangxx %s -### -o %t.o -target amd64-unknown-freebsd10.0 -stdlib=platform 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-TEN %s
 // RUN: %clangxx %s -### -o %t.o -target amd64-unknown-freebsd9.2 -stdlib=platform 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NINE %s
+// CHECK-DEFAULT: "-lc++" "-lm"
 // CHECK-TEN: "-lc++" "-lm"
 // CHECK-NINE: "-lstdc++" "-lm"
 
+// RUN: %clangxx %s -### -pg -o %t.o -target amd64-unknown-freebsd -stdlib=platform 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-PG-DEFAULT %s
 // RUN: %clangxx %s -### -pg -o %t.o -target amd64-unknown-freebsd14.0 -stdlib=platform 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-PG-FOURTEEN %s
 // RUN: %clangxx %s -### -pg -o %t.o -target amd64-unknown-freebsd10.0 -stdlib=platform 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-PG-TEN %s
 // RUN: %clangxx %s -### -pg -o %t.o -target amd64-unknown-freebsd9.2 -stdlib=platform 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-PG-NINE %s
+// CHECK-PG-DEFAULT: "-lc++_p" "-lm_p"
 // CHECK-PG-FOURTEEN: "-lc++" "-lm"
 // CHECK-PG-TEN: "-lc++_p" "-lm_p"
 // CHECK-PG-NINE: "-lstdc++_p" "-lm_p"