Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

llvm: add config file, update caveats #192709

Closed
wants to merge 1 commit into from
Closed

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
Base automatically changed from llvm-lld-flang-19.1.1 to master October 3, 2024 14:37
@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 added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label Oct 4, 2024
@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 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.

@@ -445,10 +463,28 @@ def install
end
end

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

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.

4 participants