Revert "[Fix] Disable fdefine-target-os-macros for now"

[zixu-w]

llvm/llvm-test-suite#65 fixed the llvm-test-suite errors. Reapply the change to enable fdefine-target-os-macros by default for Darwin targets.

This reverts commit 63be986.

[llvmbot]

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Zixu Wang (zixu-w)

Changes

llvm/llvm-test-suite#65 fixed the llvm-test-suite errors. Reapply the change to enable fdefine-target-os-macros by default for Darwin targets.

This reverts commit 63be986.

---

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

2 Files Affected:

• (modified) clang/lib/Driver/ToolChains/Darwin.cpp (+4)
• (modified) clang/test/Driver/fdefine-target-os-macros.c (+10-19)

diff --git a/clang/lib/Driver/ToolChains/Darwin.cpp b/clang/lib/Driver/ToolChains/Darwin.cpp
index 65846cace461e3..8f24e5287e198c 100644
--- a/clang/lib/Driver/ToolChains/Darwin.cpp
+++ b/clang/lib/Driver/ToolChains/Darwin.cpp
@@ -2917,6 +2917,10 @@ void Darwin::addClangTargetOptions(const llvm::opt::ArgList &DriverArgs,
   // to fix the same problem with C++ headers, and is generally fragile.
   if (!sdkSupportsBuiltinModules(TargetPlatform, SDKInfo))
     CC1Args.push_back("-fbuiltin-headers-in-system-modules");
+
+  if (!DriverArgs.hasArgNoClaim(options::OPT_fdefine_target_os_macros,
+                                options::OPT_fno_define_target_os_macros))
+    CC1Args.push_back("-fdefine-target-os-macros");
 }
 
 void Darwin::addClangCC1ASTargetOptions(
diff --git a/clang/test/Driver/fdefine-target-os-macros.c b/clang/test/Driver/fdefine-target-os-macros.c
index 030d4ce34cb282..d7379dd3d5396b 100644
--- a/clang/test/Driver/fdefine-target-os-macros.c
+++ b/clang/test/Driver/fdefine-target-os-macros.c
@@ -1,12 +1,11 @@
 // RUN: %clang -### --target=arm64-apple-darwin %s 2>&1 | FileCheck %s --check-prefix=DARWIN-DEFAULT
-// DARWIN-DEFAULT-NOT: "-fdefine-target-os-macros"
+// DARWIN-DEFAULT: "-fdefine-target-os-macros"
 
 // RUN: %clang -### --target=arm-none-linux-gnu %s 2>&1 | FileCheck %s --check-prefix=NON-DARWIN-DEFAULT
 // RUN: %clang -### --target=x86_64-pc-win32 %s 2>&1 | FileCheck %s --check-prefix=NON-DARWIN-DEFAULT
 // NON-DARWIN-DEFAULT-NOT: "-fdefine-target-os-macros"
 
-// RUN: %clang -dM -E --target=arm64-apple-macos \
-// RUN:        -fdefine-target-os-macros %s 2>&1 \
+// RUN: %clang -dM -E --target=arm64-apple-macos %s 2>&1 \
 // RUN: | FileCheck %s -DMAC=1         \
 // RUN:                -DOSX=1         \
 // RUN:                -DIPHONE=0      \
@@ -21,8 +20,7 @@
 // RUN:                -DLINUX=0       \
 // RUN:                -DUNIX=0
 
-// RUN: %clang -dM -E --target=arm64-apple-ios \
-// RUN:        -fdefine-target-os-macros %s 2>&1 \
+// RUN: %clang -dM -E --target=arm64-apple-ios %s 2>&1 \
 // RUN: | FileCheck %s -DMAC=1         \
 // RUN:                -DOSX=0         \
 // RUN:                -DIPHONE=1      \
@@ -37,8 +35,7 @@
 // RUN:                -DLINUX=0       \
 // RUN:                -DUNIX=0
 
-// RUN: %clang -dM -E --target=arm64-apple-ios-macabi \
-// RUN:        -fdefine-target-os-macros %s 2>&1 \
+// RUN: %clang -dM -E --target=arm64-apple-ios-macabi %s 2>&1 \
 // RUN: | FileCheck %s -DMAC=1         \
 // RUN:                -DOSX=0         \
 // RUN:                -DIPHONE=1      \
@@ -53,8 +50,7 @@
 // RUN:                -DLINUX=0       \
 // RUN:                -DUNIX=0
 
-// RUN: %clang -dM -E --target=arm64-apple-ios-simulator \
-// RUN:        -fdefine-target-os-macros %s 2>&1 \
+// RUN: %clang -dM -E --target=arm64-apple-ios-simulator %s 2>&1 \
 // RUN: | FileCheck %s -DMAC=1         \
 // RUN:                -DOSX=0         \
 // RUN:                -DIPHONE=1      \
@@ -69,8 +65,7 @@
 // RUN:                -DLINUX=0       \
 // RUN:                -DUNIX=0
 
-// RUN: %clang -dM -E --target=arm64-apple-tvos \
-// RUN:        -fdefine-target-os-macros %s 2>&1 \
+// RUN: %clang -dM -E --target=arm64-apple-tvos %s 2>&1 \
 // RUN: | FileCheck %s -DMAC=1         \
 // RUN:                -DOSX=0         \
 // RUN:                -DIPHONE=1      \
@@ -85,8 +80,7 @@
 // RUN:                -DLINUX=0       \
 // RUN:                -DUNIX=0
 
-// RUN: %clang -dM -E --target=arm64-apple-tvos-simulator \
-// RUN:        -fdefine-target-os-macros %s 2>&1 \
+// RUN: %clang -dM -E --target=arm64-apple-tvos-simulator %s 2>&1 \
 // RUN: | FileCheck %s -DMAC=1         \
 // RUN:                -DOSX=0         \
 // RUN:                -DIPHONE=1      \
@@ -101,8 +95,7 @@
 // RUN:                -DLINUX=0       \
 // RUN:                -DUNIX=0
 
-// RUN: %clang -dM -E --target=arm64-apple-watchos \
-// RUN:        -fdefine-target-os-macros %s 2>&1 \
+// RUN: %clang -dM -E --target=arm64-apple-watchos %s 2>&1 \
 // RUN: | FileCheck %s -DMAC=1         \
 // RUN:                -DOSX=0         \
 // RUN:                -DIPHONE=1      \
@@ -117,8 +110,7 @@
 // RUN:                -DLINUX=0       \
 // RUN:                -DUNIX=0
 
-// RUN: %clang -dM -E --target=arm64-apple-watchos-simulator \
-// RUN:        -fdefine-target-os-macros %s 2>&1 \
+// RUN: %clang -dM -E --target=arm64-apple-watchos-simulator %s 2>&1 \
 // RUN: | FileCheck %s -DMAC=1         \
 // RUN:                -DOSX=0         \
 // RUN:                -DIPHONE=1      \
@@ -133,8 +125,7 @@
 // RUN:                -DLINUX=0       \
 // RUN:                -DUNIX=0
 
-// RUN: %clang -dM -E --target=arm64-apple-driverkit \
-// RUN:        -fdefine-target-os-macros %s 2>&1 \
+// RUN: %clang -dM -E --target=arm64-apple-driverkit %s 2>&1 \
 // RUN: | FileCheck %s -DMAC=1         \
 // RUN:                -DOSX=0         \
 // RUN:                -DIPHONE=0      \

[zmodem]

Could you add a release note about this, explaining how it changes the macros and what actions users should take?

We've already seen that it broke zlib builds, and we're having issues with libpng too in Chromium (https://crbug.com/1519899), so users are likely to run into this.

[zixu-w]

Hi @zmodem !

Could you add a release note about this, explaining how it changes the macros and what actions users should take?

That's a good idea, I should be able to put something up.

We've already seen that it broke zlib builds, and we're having issues with libpng too in Chromium (https://crbug.com/1519899), so users are likely to run into this.

FWIW, we've also noticed the issue and upstreamed a fix for libpng (pnggroup/libpng#529)