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

[clang] Do not pass -canonical-system-headers on Windows by default #71097

Closed
wants to merge 1 commit into from

Conversation

aeubanks
Copy link
Contributor

@aeubanks aeubanks commented Nov 2, 2023

Canonicalizing paths on Windows leads to unexpected things like changing
drive letters. As a short term fix, do not canonicalize system headers
on Windows by default.

Fixes #70011

Canonicalizing paths on Windows leads to unexpected things like changing
drive letters. As a short term fix, do not canonicalize system headers
on Windows by default.

Fixes llvm#70011
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Nov 2, 2023
@aeubanks aeubanks requested review from rnk and mstorsjo November 2, 2023 20:03
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 2, 2023

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Arthur Eubanks (aeubanks)

Changes

Canonicalizing paths on Windows leads to unexpected things like changing
drive letters. As a short term fix, do not canonicalize system headers
on Windows by default.

Fixes #70011


Full diff: https://github.com/llvm/llvm-project/pull/71097.diff

3 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/Clang.cpp (+12-1)
  • (added) clang/test/Driver/canonical-system-headers-win.c (+9)
  • (modified) clang/test/Driver/canonical-system-headers.c (+3)
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 79f7fba22570746..b94728f79e0a175 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -1180,8 +1180,19 @@ void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
     if (ArgM->getOption().matches(options::OPT_M) ||
         ArgM->getOption().matches(options::OPT_MD))
       CmdArgs.push_back("-sys-header-deps");
+
+      // #70011: Canonicalization on Windows does unexpected things like change
+      // drive letters.
+      // FIXME: find and use Windows API that canonicalizes paths except for
+      // drive letter.
+#if defined(_WIN32) || defined(_WIN64)
+    bool CanonicalSystemHeadersDefault = false;
+#else
+    bool CanonicalSystemHeadersDefault = true;
+#endif
     if (Args.hasFlag(options::OPT_canonical_prefixes,
-                     options::OPT_no_canonical_prefixes, true))
+                     options::OPT_no_canonical_prefixes,
+                     CanonicalSystemHeadersDefault))
       CmdArgs.push_back("-canonical-system-headers");
     if ((isa<PrecompileJobAction>(JA) &&
          !Args.hasArg(options::OPT_fno_module_file_deps)) ||
diff --git a/clang/test/Driver/canonical-system-headers-win.c b/clang/test/Driver/canonical-system-headers-win.c
new file mode 100644
index 000000000000000..cc3ec57ff4de614
--- /dev/null
+++ b/clang/test/Driver/canonical-system-headers-win.c
@@ -0,0 +1,9 @@
+// REQUIRES: system-windows
+// The default on Windows is false due to #70011.
+
+// RUN: %clang -MD -no-canonical-prefixes -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-NO
+// RUN: %clang -MD -canonical-prefixes -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-YES
+// RUN: %clang -MD -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-NO
+
+// CHECK-YES: "-canonical-system-headers"
+// CHECK-NO-NOT: "-canonical-system-headers"
diff --git a/clang/test/Driver/canonical-system-headers.c b/clang/test/Driver/canonical-system-headers.c
index a7ab57521fc2249..d37b5925c73035d 100644
--- a/clang/test/Driver/canonical-system-headers.c
+++ b/clang/test/Driver/canonical-system-headers.c
@@ -1,3 +1,6 @@
+// REQUIRES: !system-windows
+// The default on Windows is false due to #70011.
+
 // RUN: %clang -MD -no-canonical-prefixes -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-NO
 // RUN: %clang -MD -canonical-prefixes -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-YES
 // RUN: %clang -MD -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-YES

// drive letters.
// FIXME: find and use Windows API that canonicalizes paths except for
// drive letter.
#if defined(_WIN32) || defined(_WIN64)
Copy link
Member

Choose a reason for hiding this comment

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

_WIN32 is defined on 64-bit Windows.

@@ -1,3 +1,6 @@
// REQUIRES: !system-windows
Copy link
Member

Choose a reason for hiding this comment

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

UNSUPPORTED: system-windows

@aeubanks
Copy link
Contributor Author

aeubanks commented Nov 8, 2023

closing in favor of #71697

@aeubanks aeubanks closed this Nov 8, 2023
@aeubanks aeubanks deleted the canon branch November 8, 2023 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

After 578a4716f Clang -MD by default produces different dependecies file.
3 participants