Skip to content

Conversation

nico
Copy link
Contributor

@nico nico commented Feb 10, 2025

We now process all 6 options left-to-right and pick whatever is active at the end.

Fixes #124868.

…r like gcc

We now process all 6 options left-to-right and pick whatever is active
at the end.

Fixes llvm#124868.
@nico nico requested review from nikic and AaronBallman February 10, 2025 14:42
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Feb 10, 2025
@nico nico changed the title [clang] Handle f(no-)strict-overflow, f(no-)wrapv, f(no-)wrapv-pointe… [clang] Handle f(no-)strict-overflow, f(no-)wrapv, f(no-)wrapv-pointer like gcc Feb 10, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 10, 2025

@llvm/pr-subscribers-clang

Author: Nico Weber (nico)

Changes

We now process all 6 options left-to-right and pick whatever is active at the end.

Fixes #124868.


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

2 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/CommonArgs.cpp (+33-15)
  • (modified) clang/test/Driver/clang_wrapv_opts.c (+12-12)
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 61917db4d780d5..9a4d3f55c911ce 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -3096,21 +3096,39 @@ bool tools::shouldRecordCommandLine(const ToolChain &TC,
 
 void tools::renderCommonIntegerOverflowOptions(const ArgList &Args,
                                                ArgStringList &CmdArgs) {
-  // -fno-strict-overflow implies -fwrapv if it isn't disabled, but
-  // -fstrict-overflow won't turn off an explicitly enabled -fwrapv.
-  bool StrictOverflow = Args.hasFlag(options::OPT_fstrict_overflow,
-                                     options::OPT_fno_strict_overflow, true);
-  if (Arg *A = Args.getLastArg(options::OPT_fwrapv, options::OPT_fno_wrapv)) {
-    if (A->getOption().matches(options::OPT_fwrapv))
-      CmdArgs.push_back("-fwrapv");
-  } else if (!StrictOverflow) {
-    CmdArgs.push_back("-fwrapv");
+  bool use_fwrapv = false;
+  bool use_fwrapv_pointer = false;
+  for (const Arg *A : Args.filtered(
+           options::OPT_fstrict_overflow, options::OPT_fno_strict_overflow,
+           options::OPT_fwrapv, options::OPT_fno_wrapv,
+           options::OPT_fwrapv_pointer, options::OPT_fno_wrapv_pointer)) {
+    A->claim();
+    switch (A->getOption().getID()) {
+    case options::OPT_fstrict_overflow:
+      use_fwrapv = false;
+      use_fwrapv_pointer = false;
+      break;
+    case options::OPT_fno_strict_overflow:
+      use_fwrapv = true;
+      use_fwrapv_pointer = true;
+      break;
+    case options::OPT_fwrapv:
+      use_fwrapv = true;
+      break;
+    case options::OPT_fno_wrapv:
+      use_fwrapv = false;
+      break;
+    case options::OPT_fwrapv_pointer:
+      use_fwrapv_pointer = true;
+      break;
+    case options::OPT_fno_wrapv_pointer:
+      use_fwrapv_pointer = false;
+      break;
+    }
   }
-  if (Arg *A = Args.getLastArg(options::OPT_fwrapv_pointer,
-                               options::OPT_fno_wrapv_pointer)) {
-    if (A->getOption().matches(options::OPT_fwrapv_pointer))
-      CmdArgs.push_back("-fwrapv-pointer");
-  } else if (!StrictOverflow) {
+
+  if (use_fwrapv)
+    CmdArgs.push_back("-fwrapv");
+  if (use_fwrapv_pointer)
     CmdArgs.push_back("-fwrapv-pointer");
-  }
 }
diff --git a/clang/test/Driver/clang_wrapv_opts.c b/clang/test/Driver/clang_wrapv_opts.c
index 9f3a884324dcdd..3de928d9a419a4 100644
--- a/clang/test/Driver/clang_wrapv_opts.c
+++ b/clang/test/Driver/clang_wrapv_opts.c
@@ -1,20 +1,20 @@
 // RUN: %clang -### -S -fwrapv -fno-wrapv -fwrapv -Werror %s 2>&1 | FileCheck -check-prefix=CHECK1 %s
 // CHECK1: "-fwrapv"
-//
+
 // RUN: %clang -### -S -fwrapv-pointer -fno-wrapv-pointer -fwrapv-pointer -Werror %s 2>&1 | FileCheck -check-prefix=CHECK1-POINTER %s
 // CHECK1-POINTER: "-fwrapv-pointer"
-//
+
 // RUN: %clang -### -S -fstrict-overflow -fno-strict-overflow -Werror %s 2>&1 | FileCheck -check-prefix=CHECK2 %s
 // CHECK2: "-fwrapv"{{.*}}"-fwrapv-pointer"
-//
+
 // RUN: %clang -### -S -fwrapv -fstrict-overflow -Werror -Werror %s 2>&1 | FileCheck -check-prefix=CHECK3 %s --implicit-check-not="-fwrapv-pointer"
-// CHECK3: "-fwrapv"
-//
+// CHECK3-NOT: "-fwrapv"
+
 // RUN: %clang -### -S -fwrapv-pointer -fstrict-overflow -Werror %s 2>&1 | FileCheck -check-prefix=CHECK3-POINTER %s --implicit-check-not="-fwrapv"
-// CHECK3-POINTER: "-fwrapv-pointer"
-//
-// RUN: %clang -### -S -fno-wrapv -fno-strict-overflow -Werror %s 2>&1 | FileCheck -check-prefix=CHECK4 %s --implicit-check-not="-fwrapv"
-// CHECK4: "-fwrapv-pointer"
-//
-// RUN: %clang -### -S -fno-wrapv-pointer -fno-strict-overflow -Werror %s 2>&1 | FileCheck -check-prefix=CHECK4-POINTER %s --implicit-check-not="-fwrapv-pointer"
-// CHECK4-POINTER: "-fwrapv"
+// CHECK3-POINTER-NOT: "-fwrapv-pointer"
+
+// RUN: %clang -### -S -fno-wrapv -fno-strict-overflow -fno-wrapv-pointer -Werror %s 2>&1 | FileCheck -check-prefix=CHECK4 %s --implicit-check-not="-fwrapv"
+// CHECK4: "-fwrapv"
+
+// RUN: %clang -### -S -fno-wrapv-pointer -fno-strict-overflow -fno-wrapv -Werror %s 2>&1 | FileCheck -check-prefix=CHECK4-POINTER %s --implicit-check-not="-fwrapv-pointer"
+// CHECK4-POINTER: "-fwrapv-pointer"

@llvmbot
Copy link
Member

llvmbot commented Feb 10, 2025

@llvm/pr-subscribers-clang-driver

Author: Nico Weber (nico)

Changes

We now process all 6 options left-to-right and pick whatever is active at the end.

Fixes #124868.


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

2 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/CommonArgs.cpp (+33-15)
  • (modified) clang/test/Driver/clang_wrapv_opts.c (+12-12)
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 61917db4d780d5..9a4d3f55c911ce 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -3096,21 +3096,39 @@ bool tools::shouldRecordCommandLine(const ToolChain &TC,
 
 void tools::renderCommonIntegerOverflowOptions(const ArgList &Args,
                                                ArgStringList &CmdArgs) {
-  // -fno-strict-overflow implies -fwrapv if it isn't disabled, but
-  // -fstrict-overflow won't turn off an explicitly enabled -fwrapv.
-  bool StrictOverflow = Args.hasFlag(options::OPT_fstrict_overflow,
-                                     options::OPT_fno_strict_overflow, true);
-  if (Arg *A = Args.getLastArg(options::OPT_fwrapv, options::OPT_fno_wrapv)) {
-    if (A->getOption().matches(options::OPT_fwrapv))
-      CmdArgs.push_back("-fwrapv");
-  } else if (!StrictOverflow) {
-    CmdArgs.push_back("-fwrapv");
+  bool use_fwrapv = false;
+  bool use_fwrapv_pointer = false;
+  for (const Arg *A : Args.filtered(
+           options::OPT_fstrict_overflow, options::OPT_fno_strict_overflow,
+           options::OPT_fwrapv, options::OPT_fno_wrapv,
+           options::OPT_fwrapv_pointer, options::OPT_fno_wrapv_pointer)) {
+    A->claim();
+    switch (A->getOption().getID()) {
+    case options::OPT_fstrict_overflow:
+      use_fwrapv = false;
+      use_fwrapv_pointer = false;
+      break;
+    case options::OPT_fno_strict_overflow:
+      use_fwrapv = true;
+      use_fwrapv_pointer = true;
+      break;
+    case options::OPT_fwrapv:
+      use_fwrapv = true;
+      break;
+    case options::OPT_fno_wrapv:
+      use_fwrapv = false;
+      break;
+    case options::OPT_fwrapv_pointer:
+      use_fwrapv_pointer = true;
+      break;
+    case options::OPT_fno_wrapv_pointer:
+      use_fwrapv_pointer = false;
+      break;
+    }
   }
-  if (Arg *A = Args.getLastArg(options::OPT_fwrapv_pointer,
-                               options::OPT_fno_wrapv_pointer)) {
-    if (A->getOption().matches(options::OPT_fwrapv_pointer))
-      CmdArgs.push_back("-fwrapv-pointer");
-  } else if (!StrictOverflow) {
+
+  if (use_fwrapv)
+    CmdArgs.push_back("-fwrapv");
+  if (use_fwrapv_pointer)
     CmdArgs.push_back("-fwrapv-pointer");
-  }
 }
diff --git a/clang/test/Driver/clang_wrapv_opts.c b/clang/test/Driver/clang_wrapv_opts.c
index 9f3a884324dcdd..3de928d9a419a4 100644
--- a/clang/test/Driver/clang_wrapv_opts.c
+++ b/clang/test/Driver/clang_wrapv_opts.c
@@ -1,20 +1,20 @@
 // RUN: %clang -### -S -fwrapv -fno-wrapv -fwrapv -Werror %s 2>&1 | FileCheck -check-prefix=CHECK1 %s
 // CHECK1: "-fwrapv"
-//
+
 // RUN: %clang -### -S -fwrapv-pointer -fno-wrapv-pointer -fwrapv-pointer -Werror %s 2>&1 | FileCheck -check-prefix=CHECK1-POINTER %s
 // CHECK1-POINTER: "-fwrapv-pointer"
-//
+
 // RUN: %clang -### -S -fstrict-overflow -fno-strict-overflow -Werror %s 2>&1 | FileCheck -check-prefix=CHECK2 %s
 // CHECK2: "-fwrapv"{{.*}}"-fwrapv-pointer"
-//
+
 // RUN: %clang -### -S -fwrapv -fstrict-overflow -Werror -Werror %s 2>&1 | FileCheck -check-prefix=CHECK3 %s --implicit-check-not="-fwrapv-pointer"
-// CHECK3: "-fwrapv"
-//
+// CHECK3-NOT: "-fwrapv"
+
 // RUN: %clang -### -S -fwrapv-pointer -fstrict-overflow -Werror %s 2>&1 | FileCheck -check-prefix=CHECK3-POINTER %s --implicit-check-not="-fwrapv"
-// CHECK3-POINTER: "-fwrapv-pointer"
-//
-// RUN: %clang -### -S -fno-wrapv -fno-strict-overflow -Werror %s 2>&1 | FileCheck -check-prefix=CHECK4 %s --implicit-check-not="-fwrapv"
-// CHECK4: "-fwrapv-pointer"
-//
-// RUN: %clang -### -S -fno-wrapv-pointer -fno-strict-overflow -Werror %s 2>&1 | FileCheck -check-prefix=CHECK4-POINTER %s --implicit-check-not="-fwrapv-pointer"
-// CHECK4-POINTER: "-fwrapv"
+// CHECK3-POINTER-NOT: "-fwrapv-pointer"
+
+// RUN: %clang -### -S -fno-wrapv -fno-strict-overflow -fno-wrapv-pointer -Werror %s 2>&1 | FileCheck -check-prefix=CHECK4 %s --implicit-check-not="-fwrapv"
+// CHECK4: "-fwrapv"
+
+// RUN: %clang -### -S -fno-wrapv-pointer -fno-strict-overflow -fno-wrapv -Werror %s 2>&1 | FileCheck -check-prefix=CHECK4-POINTER %s --implicit-check-not="-fwrapv-pointer"
+// CHECK4-POINTER: "-fwrapv-pointer"

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@nico nico merged commit 783275e into llvm:main Feb 10, 2025
8 checks passed
@nico nico deleted the clang-wrap branch February 10, 2025 15:57
@nico nico added this to the LLVM 20.X Release milestone Feb 10, 2025
@nico
Copy link
Contributor Author

nico commented Feb 10, 2025

/cherry-pick 783275e

@llvmbot
Copy link
Member

llvmbot commented Feb 10, 2025

/pull-request #126535

jhuber6 pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 11, 2025
…r like gcc (llvm#126524)

We now process all 6 options left-to-right and pick whatever is active
at the end.

Fixes llvm#124868.

(cherry picked from commit 783275e)
Icohedron pushed a commit to Icohedron/llvm-project that referenced this pull request Feb 11, 2025
…r like gcc (llvm#126524)

We now process all 6 options left-to-right and pick whatever is active
at the end.

Fixes llvm#124868.
sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Feb 24, 2025
…r like gcc (llvm#126524)

We now process all 6 options left-to-right and pick whatever is active
at the end.

Fixes llvm#124868.
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
Development

Successfully merging this pull request may close these issues.

[Clang] Inconsistencies with -fstrict-overflow between Clang and GCC
3 participants