Page MenuHomePhabricator

[Driver] Default to libc++ on FreeBSD
AcceptedPublic

Authored by jbeich on Apr 8 2020, 9:37 PM.

Details

Reviewers
dim
emaste
brooks
Group Reviewers
Restricted Project
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>
         ^~~~~~~~~~~~~

Diff Detail

Event Timeline

jbeich created this revision.Apr 8 2020, 9:37 PM
jbeich updated this revision to Diff 256202.Apr 8 2020, 11:56 PM
  • Attempt to unbreak tests
arichardson added a comment.EditedApr 9 2020, 2:46 AM

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 added a comment.Apr 9 2020, 11:53 AM

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 added a subscriber: myfreeweb.EditedApr 11 2020, 3:17 AM

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 updated this revision to Diff 257403.Apr 14 2020, 10:29 AM
jbeich retitled this revision from [Driver] Drop support for FreeBSD < 10 to [Driver] Default to libc++ on FreeBSD.
  • 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.

  • 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 accepted this revision.Apr 20 2020, 11:41 AM

Looks ok to me

This revision is now accepted and ready to land.Apr 20 2020, 11:41 AM

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