Skip to content

Conversation

@carlocab
Copy link
Member

@carlocab carlocab commented Oct 3, 2024

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

Adding a system config file will allow us to stop baking in the
DEFAULT_SYSROOT and allow users on Xcode-only installations to use the
bottles. It will also make it easier to update this value when users
upgrade the major version of their macOS. Before this change, this
required a full reinstall. After, users will simply have to
brew postinstall llvm.

The config file change requires an upstream patch. We'll probably have
to wait a little bit for it to merge, and then wait another few days to
make sure it doesn't get reverted.

Following discussion at #192505, let's also update the caveats to notify
users of potential changes regarding LLVM_ENABLE_EH in LLVM 20.

Marked as syntax-only for now to avoid duplicating the build at #192505.

@carlocab carlocab added the CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI. label Oct 3, 2024
@github-actions github-actions bot added the python Python use is a significant feature of the PR or issue label Oct 3, 2024
@carlocab carlocab force-pushed the llvm-disable-eh-caveats branch from 3da97da to 24b7c0f Compare October 3, 2024 11:49
Base automatically changed from llvm-lld-flang-19.1.1 to master October 3, 2024 14:37
@carlocab carlocab force-pushed the llvm-disable-eh-caveats branch from 24b7c0f to 9bb969a Compare October 4, 2024 00:05
@carlocab carlocab added long build Set a long timeout for formula testing and removed CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI. labels Oct 4, 2024
@carlocab carlocab force-pushed the llvm-disable-eh-caveats branch from 9bb969a to 9b4bb2d Compare October 4, 2024 08:25
@carlocab carlocab added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label Oct 4, 2024
@carlocab carlocab force-pushed the llvm-disable-eh-caveats branch from 9b4bb2d to 89c1ea6 Compare October 4, 2024 14:36
@carlocab carlocab added the CI-no-fail-fast-deps Continue dependent tests despite failing GitHub Actions matrix tests. label Oct 5, 2024
Adding a system config file will allow us to stop baking in the
`DEFAULT_SYSROOT` and allow users on Xcode-only installations to use the
bottles. It will also make it easier to update this value when users
upgrade the major version of their macOS. Before this change, this
required a full reinstall. After, users will simply have to
`brew postinstall llvm`.

The config file change requires an upstream patch. We'll probably have
to wait a little bit for it to merge, and then wait another few days to
make sure it doesn't get reverted.

Following discussion at #192505, let's also update the caveats to notify
users of potential changes regarding `LLVM_ENABLE_EH` in LLVM 20.
@carlocab carlocab force-pushed the llvm-disable-eh-caveats branch from 89c1ea6 to 60603e8 Compare October 5, 2024 05:52
@carlocab carlocab marked this pull request as ready for review October 5, 2024 05:52
@carlocab carlocab added the ready to merge PR can be merged once CI is green label Oct 5, 2024
@carlocab carlocab requested a review from Bo98 October 6, 2024 03:17
Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postinstall kinda sucks to rely on for run-later code since it can go stale but is better than what we have at least.

return unless (macos_sdk = MacOS.sdk_path_if_needed)

clang_config_file_dir.mkpath
%w[clang.cfg clang++.cfg].each do |config_file|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does clang-cl, clang-cpp and flang work with this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to set --sysroot for clang-cl, do we? Need to check clang-cpp, but flang seems to work fine based on dependent testing.

Copy link
Member

@Bo98 Bo98 Oct 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No sorry, didn't mean to mention clang-cl. Not sure why I typed that.

clang-cpp is the main concern to check.


def post_install
return unless OS.mac?
return unless (macos_sdk = MacOS.sdk_path_if_needed)
Copy link
Member

@Bo98 Bo98 Oct 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is best effort and might point to a newer SDK if CLT is not installed which wouldn't be great.

There's also the Xcode-beta.app -> Xcode.app case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, but note this is the path we set DEFAULT_SYSROOT to so this is strictly an improvement over existing builds (because users can now fix a broken --sysroot flag when previously they couldn't).

Copy link
Member

@Bo98 Bo98 Oct 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a relocatable bottle. Anybody building from source are long past the days of updating Xcode and are required by brew to have the CLT installed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should we just require the CLT regardless?

Copy link
Member

@Bo98 Bo98 Oct 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably. If we pursue a patch for target matching then we'll probably have to in the end anyway as we'll ship config files for all OS versions (pkg-config style except we don't need to bake in the OS version to clang).

@Bo98
Copy link
Member

Bo98 commented Oct 6, 2024

Additional thought to upstream: clang does support and read e.g. arm64-apple-darwin23.4.0-clang.cfg.
Ideally it could special case Darwin to also support truncating that to major versions arm64-apple-darwin23-clang.cfg and then we can have a config file per SDK. Maybe even a arm64-apple-darwin-clang.cfg fallback too if it can't find a versioned one.

I can probably look into this more.

@carlocab
Copy link
Member Author

carlocab commented Oct 6, 2024

Additional thought to upstream: clang does support and read e.g. arm64-apple-darwin23.4.0-clang.cfg. Ideally it could special case Darwin to also support truncating that to major versions arm64-apple-darwin23-clang.cfg and then we can have a config file per SDK. Maybe even a arm64-apple-darwin-clang.cfg fallback too if it can't find a versioned one.

diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 2aaa52072b03..ae25b4a71c6a 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1222,6 +1222,23 @@ bool Driver::loadDefaultConfigFiles(llvm::cl::ExpansionContext &ExpCtx) {
   if (ExpCtx.findConfigFile(CfgFileName, CfgFilePath))
     return readConfigFile(CfgFilePath, ExpCtx);
 
+  auto pos = Triple.find("-apple-darwin");
+  if (pos != Triple.npos) {
+    // First, find the major-versioned triple.
+    // E.g. Triple = arm64-apple-darwin24.0.0; T = arm64-apple-darwin24
+    auto T = Triple.substr(0, Triple.find(".", pos));
+    CfgFileName = T + ".cfg";
+    if (ExpCtx.findConfigFile(CfgFileName, CfgFilePath))
+      return readConfigFile(CfgFilePath, ExpCtx);
+
+    // If that is not available, try an unversioned triple.
+    // E.g. Triple = x86_64-apple-darwin24.0.0; T = arm64-apple-darwin
+    T = Triple.substr(0, pos + 13); // pos + strlen("-apple-darwin")
+    CfgFileName = T + ".cfg";
+    if (ExpCtx.findConfigFile(CfgFileName, CfgFilePath))
+      return readConfigFile(CfgFilePath, ExpCtx);
+  }
+
   // If we were unable to find a config file deduced from executable name,
   // that is not an error.
   return false;

Will probably need some tests.

@Bo98
Copy link
Member

Bo98 commented Oct 6, 2024

You can probably actually make that not Darwin specific:

diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 2aaa52072b03..142b8bb6b1a8 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1147,6 +1147,34 @@ bool Driver::loadConfigFiles() {
   return false;
 }
 
+static bool findTripleConfigFile(llvm::cl::ExpansionContext &ExpCtx,
+                                 SmallString<128> &ConfigFilePath,
+                                 llvm::Triple Triple, std::string Suffix) {
+  // First, try the full unmodified triple.
+  if (ExpCtx.findConfigFile(Triple.str() + Suffix, ConfigFilePath))
+    return true;
+
+  // Don't continue if we didn't find a parsable version in the triple.
+  VersionTuple OSVersion = Triple.getOSVersion();
+  if (!OSVersion.getMinor().has_value())
+    return false;
+
+  std::string BaseOSName = Triple.getOSTypeName(Triple.getOS()).str();
+
+  // Next try strip the version to only include the major component.
+  // e.g. arm64-apple-darwin23.6.0 -> arm64-apple-darwin23
+  if (OSVersion.getMajor() != 0) {
+    Triple.setOSName(BaseOSName + llvm::utostr(OSVersion.getMajor()));
+    if (ExpCtx.findConfigFile(Triple.str() + Suffix, ConfigFilePath))
+      return true;
+  }
+
+  // Finally, try without any version suffix at all.
+  // e.g. arm64-apple-darwin23.6.0 -> arm64-apple-darwin
+  Triple.setOSName(BaseOSName);
+  return ExpCtx.findConfigFile(Triple.str() + Suffix, ConfigFilePath);
+}
+
 bool Driver::loadDefaultConfigFiles(llvm::cl::ExpansionContext &ExpCtx) {
   // Disable default config if CLANG_NO_DEFAULT_CONFIG is set to a non-empty
   // value.
@@ -1158,7 +1186,7 @@ bool Driver::loadDefaultConfigFiles(llvm::cl::ExpansionContext &ExpCtx) {
     return false;
 
   std::string RealMode = getExecutableForDriverMode(Mode);
-  std::string Triple;
+  llvm::Triple Triple;
 
   // If name prefix is present, no --target= override was passed via CLOptions
   // and the name prefix is not a valid triple, force it for backwards
@@ -1169,15 +1197,13 @@ bool Driver::loadDefaultConfigFiles(llvm::cl::ExpansionContext &ExpCtx) {
     llvm::Triple PrefixTriple{ClangNameParts.TargetPrefix};
     if (PrefixTriple.getArch() == llvm::Triple::UnknownArch ||
         PrefixTriple.isOSUnknown())
-      Triple = PrefixTriple.str();
+      Triple = PrefixTriple;
   }
 
   // Otherwise, use the real triple as used by the driver.
-  if (Triple.empty()) {
-    llvm::Triple RealTriple =
-        computeTargetTriple(*this, TargetTriple, *CLOptions);
-    Triple = RealTriple.str();
-    assert(!Triple.empty());
+  if (Triple.str().empty()) {
+    Triple = computeTargetTriple(*this, TargetTriple, *CLOptions);
+    assert(!Triple.str().empty());
   }
 
   // Search for config files in the following order:
@@ -1192,21 +1218,21 @@ bool Driver::loadDefaultConfigFiles(llvm::cl::ExpansionContext &ExpCtx) {
 
   // Try loading <triple>-<mode>.cfg, and return if we find a match.
   SmallString<128> CfgFilePath;
-  std::string CfgFileName = Triple + '-' + RealMode + ".cfg";
-  if (ExpCtx.findConfigFile(CfgFileName, CfgFilePath))
+  if (findTripleConfigFile(ExpCtx, CfgFilePath, Triple,
+                           "-" + RealMode + ".cfg"))
     return readConfigFile(CfgFilePath, ExpCtx);
 
   bool TryModeSuffix = !ClangNameParts.ModeSuffix.empty() &&
                        ClangNameParts.ModeSuffix != RealMode;
   if (TryModeSuffix) {
-    CfgFileName = Triple + '-' + ClangNameParts.ModeSuffix + ".cfg";
-    if (ExpCtx.findConfigFile(CfgFileName, CfgFilePath))
+    if (findTripleConfigFile(ExpCtx, CfgFilePath, Triple,
+                             "-" + ClangNameParts.ModeSuffix + ".cfg"))
       return readConfigFile(CfgFilePath, ExpCtx);
   }
 
   // Try loading <mode>.cfg, and return if loading failed.  If a matching file
   // was not found, still proceed on to try <triple>.cfg.
-  CfgFileName = RealMode + ".cfg";
+  std::string CfgFileName = RealMode + ".cfg";
   if (ExpCtx.findConfigFile(CfgFileName, CfgFilePath)) {
     if (readConfigFile(CfgFilePath, ExpCtx))
       return true;
@@ -1218,8 +1244,7 @@ bool Driver::loadDefaultConfigFiles(llvm::cl::ExpansionContext &ExpCtx) {
   }
 
   // Try loading <triple>.cfg and return if we find a match.
-  CfgFileName = Triple + ".cfg";
-  if (ExpCtx.findConfigFile(CfgFileName, CfgFilePath))
+  if (findTripleConfigFile(ExpCtx, CfgFilePath, Triple, ".cfg"))
     return readConfigFile(CfgFilePath, ExpCtx);
 
   // If we were unable to find a config file deduced from executable name,
diff --git a/clang/test/Driver/config-file3.c b/clang/test/Driver/config-file3.c
index a0b8062c60ce..395c31ce04b6 100644
--- a/clang/test/Driver/config-file3.c
+++ b/clang/test/Driver/config-file3.c
@@ -226,3 +226,26 @@
 //
 // RUN: HOME=%S/Inputs/config %clang -### --config-user-dir=~ -v 2>&1 | FileCheck %s --check-prefix=CHECK-TILDE
 // CHECK-TILDE: User configuration file directory: {{.*}}/Inputs/config
+
+//--- Fallback to stripping OS versions
+//
+// RUN: touch %t/testdmode/x86_64-apple-darwin23.6.0-clang.cfg
+// RUN: touch %t/testdmode/x86_64-apple-darwin23-clang.cfg
+// RUN: touch %t/testdmode/x86_64-apple-darwin-clang.cfg
+// RUN: %clang -target x86_64-apple-darwin23.6.0 --config-system-dir=%t/testdmode --config-user-dir= -no-canonical-prefixes --version 2>&1 | FileCheck %s -check-prefix DARWIN --implicit-check-not 'Configuration file:'
+//
+// DARWIN: Configuration file: {{.*}}/testdmode/x86_64-apple-darwin23.6.0-clang.cfg
+
+//--- DARWIN + no full version
+//
+// RUN: rm %t/testdmode/x86_64-apple-darwin23.6.0-clang.cfg
+// RUN: %clang -target x86_64-apple-darwin23.6.0 --config-system-dir=%t/testdmode --config-user-dir= -no-canonical-prefixes --version 2>&1 | FileCheck %s -check-prefix DARWIN-MAJOR --implicit-check-not 'Configuration file:'
+//
+// DARWIN-MAJOR: Configuration file: {{.*}}/testdmode/x86_64-apple-darwin23-clang.cfg
+
+//--- DARWIN + no version
+//
+// RUN: rm %t/testdmode/x86_64-apple-darwin23-clang.cfg
+// RUN: %clang -target x86_64-apple-darwin23.6.0 --config-system-dir=%t/testdmode --config-user-dir= -no-canonical-prefixes --version 2>&1 | FileCheck %s -check-prefix DARWIN-VERSIONLESS --implicit-check-not 'Configuration file:'
+//
+// DARWIN-VERSIONLESS: Configuration file: {{.*}}/testdmode/x86_64-apple-darwin-clang.cfg

@carlocab
Copy link
Member Author

carlocab commented Oct 6, 2024

Just saw this, but I've already opened a PR. Feel free to send that upstream and I can close my PR.

@carlocab
Copy link
Member Author

carlocab commented Oct 7, 2024

Might also be nicer to pass Triple as const llvm::Triple &Triple

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Oct 28, 2024
@chenrui333 chenrui333 added in progress Stale bot should stay away and removed stale No recent activity labels Oct 28, 2024
@cho-m cho-m removed the ready to merge PR can be merged once CI is green label Oct 29, 2024
@carlocab
Copy link
Member Author

Moved to #196094.

@carlocab carlocab closed this Oct 30, 2024
@github-actions github-actions bot deleted the llvm-disable-eh-caveats branch October 30, 2024 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. CI-no-fail-fast-deps Continue dependent tests despite failing GitHub Actions matrix tests. in progress Stale bot should stay away long build Set a long timeout for formula testing python Python use is a significant feature of the PR or issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants