This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Default to libc++ on FreeBSD
ClosedPublic

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

Details

Reviewers
dim
brooks
jbeich
Group Reviewers
Restricted Project
Commits
rG2dec2aa3ad08: [Driver] Default to libc++ on FreeBSD
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

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

dim added a comment.Jul 31 2020, 5:43 AM

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.

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 added a comment.Jul 31 2020, 6:12 AM
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.

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 commandeered this revision.Nov 22 2021, 8:36 AM
emaste edited reviewers, added: jbeich; removed: emaste.
This revision now requires review to proceed.Nov 22 2021, 8:36 AM
emaste updated this revision to Diff 388931.Nov 22 2021, 8:36 AM

rebase tests

emaste added inline comments.Nov 22 2021, 8:38 AM
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 <emaste@FreeBSD.org>
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 accepted this revision.Nov 22 2021, 8:43 AM

LGTM

This revision is now accepted and ready to land.Nov 22 2021, 8:43 AM
This revision was landed with ongoing or failed builds.Nov 22 2021, 1:47 PM
Closed by commit rG2dec2aa3ad08: [Driver] Default to libc++ on FreeBSD (authored by jbeich, committed by emaste). · Explain Why
This revision was automatically updated to reflect the committed changes.