[clang][driver] Expose a frontend option for trap-unreachable - #174894
[clang][driver] Expose a frontend option for trap-unreachable#174894ilovepi wants to merge 9 commits into
Conversation
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang-driver Author: Paul Kirth (ilovepi) ChangesWe have several issues that list surprising behavior around UB. In many This patch adds a new driver option that does just that. For now, we're Fixes #174844 Full diff: https://github.com/llvm/llvm-project/pull/174894.diff 7 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 932b9fad40c42..e5cbc3eecb25e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -348,6 +348,8 @@ New Compiler Flags
- New options for enabling allocation token instrumentation: ``-fsanitize=alloc-token``, ``-falloc-token-max=``, ``-fsanitize-alloc-token-fast-abi``, ``-fsanitize-alloc-token-extended``.
- The ``-resource-dir`` option is now displayed in the list of options shown by ``--help``.
- New option ``-fmatrix-memory-layout`` added to control the memory layout of Clang matrix types. (e.g. ``-fmatrix-memory-layout=column-major`` or ``-fmatrix-memory-layout=row-major``).
+- New option ``-ftrap-unreachable`` added to enable the existing backend option: TrapUnreachable.
+ This behavior is off by default (e.g. no change in the compiler's behavior) for now.
Lanai Support
^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Basic/CodeGenOptions.def b/clang/include/clang/Basic/CodeGenOptions.def
index a6b1acdcf5ea9..9e84e22ff8d46 100644
--- a/clang/include/clang/Basic/CodeGenOptions.def
+++ b/clang/include/clang/Basic/CodeGenOptions.def
@@ -428,6 +428,9 @@ CODEGENOPT(DirectAccessExternalData, 1, 0, Benign)
/// paths that reach the end of a function without executing a required return.
CODEGENOPT(StrictReturn, 1, 1, Benign)
+/// Whether we should use make unreachable trap or not.
+CODEGENOPT(TrapUnreachable, 1, 0, Benign)
+
/// Whether emit pseudo probes for sample pgo profile collection.
CODEGENOPT(PseudoProbeForProfiling, 1, 0, Benign)
diff --git a/clang/include/clang/Options/Options.td b/clang/include/clang/Options/Options.td
index f336542992993..33be1569b7e8c 100644
--- a/clang/include/clang/Options/Options.td
+++ b/clang/include/clang/Options/Options.td
@@ -4685,6 +4685,12 @@ defm strict_return : BoolFOption<"strict-return",
" of a non-void function as unreachable">,
PosFlag<SetTrue>>;
+defm trap_unreachable : BoolFOption<"trap-unreachable",
+ CodeGenOpts<"TrapUnreachable">, DefaultFalse,
+ PosFlag<SetTrue, [], [ClangOption, CC1Option],
+ "Treat unreachable instructions as traps">,
+ NegFlag<SetFalse>>;
+
let Flags = [TargetSpecific] in {
defm ptrauth_intrinsics : OptInCC1FFlag<"ptrauth-intrinsics", "Enable pointer authentication intrinsics">;
defm ptrauth_calls : OptInCC1FFlag<"ptrauth-calls", "Enable signing and authentication of all indirect calls">;
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index c5ea8c9e8c3de..5f57705ffc77e 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -466,6 +466,7 @@ static bool initTargetOptions(const CompilerInstance &CI,
Options.Hotpatch = CodeGenOpts.HotPatch;
Options.JMCInstrument = CodeGenOpts.JMCInstrument;
Options.XCOFFReadOnlyPointers = CodeGenOpts.XCOFFReadOnlyPointers;
+ Options.TrapUnreachable = CodeGenOpts.TrapUnreachable;
switch (CodeGenOpts.getVecLib()) {
case llvm::driver::VectorLibrary::NoLibrary:
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 699fc31f23946..f54af613e72b8 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -5808,6 +5808,9 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
Args.addOptInFlag(CmdArgs, options::OPT_fsplit_stack,
options::OPT_fno_split_stack);
+ Args.addOptInFlag(CmdArgs, options::OPT_ftrap_unreachable,
+ options::OPT_ftrap_unreachable);
+
// -fprotect-parens=0 is default.
if (Args.hasFlag(options::OPT_fprotect_parens,
options::OPT_fno_protect_parens, false))
diff --git a/clang/test/CodeGen/X86/unreachable-trap.c b/clang/test/CodeGen/X86/unreachable-trap.c
new file mode 100644
index 0000000000000..bea34e80dd873
--- /dev/null
+++ b/clang/test/CodeGen/X86/unreachable-trap.c
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 %s -O1 -triple=x86_64-unknown-linux-gnu -ftrap-unreachable -S -o - 2>&1 | FileCheck %s --check-prefix=TRAP
+// RUN: %clang_cc1 %s -O1 -triple=x86_64-unknown-linux-gnu -S -o - 2>&1 | FileCheck %s --check-prefix=NOTRAP
+
+// TRAP: ud2
+// NOTRAP-NOT: ud2
+
+[[noreturn]]
+void exit(int);
+
+#define NULL 0
+
+static void test(void) {
+ int *ptr = NULL;
+ *ptr = 0;
+ exit(0);
+}
+
+void foo() { test(); }
diff --git a/clang/test/Driver/clang_f_opts.c b/clang/test/Driver/clang_f_opts.c
index 5871f1580d6b7..aedbef393abcc 100644
--- a/clang/test/Driver/clang_f_opts.c
+++ b/clang/test/Driver/clang_f_opts.c
@@ -651,3 +651,8 @@
// RUN: %clang -### --target=x86_64-pc-windows-msvc -fno-strict-aliasing %s 2>&1 | FileCheck -check-prefix=CHECK-NO-STRICT-ALIASING %s
// CHECK-STRICT-ALIASING-NOT: -relaxed-aliasing
// CHECK-NO-STRICT-ALIASING: -relaxed-aliasing
+
+// RUN: %clang -### -ftrap-unreachable %s 2>&1 | FileCheck %s -check-prefix=UNREACHABLE-TRAP
+// RUN: %clang -### -fno-trap-unreachable %s 2>&1 | FileCheck %s -check-prefix=NO-UNREACHABLE-TRAP
+// UNREACHABLE-TRAP: "-ftrap-unreachable"
+// NO-UNREACHABLE-TRAP-NOT: "-ftrap-unreachable"
|
|
@llvm/pr-subscribers-backend-x86 Author: Paul Kirth (ilovepi) ChangesWe have several issues that list surprising behavior around UB. In many This patch adds a new driver option that does just that. For now, we're Fixes #174844 Full diff: https://github.com/llvm/llvm-project/pull/174894.diff 7 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 932b9fad40c42..e5cbc3eecb25e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -348,6 +348,8 @@ New Compiler Flags
- New options for enabling allocation token instrumentation: ``-fsanitize=alloc-token``, ``-falloc-token-max=``, ``-fsanitize-alloc-token-fast-abi``, ``-fsanitize-alloc-token-extended``.
- The ``-resource-dir`` option is now displayed in the list of options shown by ``--help``.
- New option ``-fmatrix-memory-layout`` added to control the memory layout of Clang matrix types. (e.g. ``-fmatrix-memory-layout=column-major`` or ``-fmatrix-memory-layout=row-major``).
+- New option ``-ftrap-unreachable`` added to enable the existing backend option: TrapUnreachable.
+ This behavior is off by default (e.g. no change in the compiler's behavior) for now.
Lanai Support
^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Basic/CodeGenOptions.def b/clang/include/clang/Basic/CodeGenOptions.def
index a6b1acdcf5ea9..9e84e22ff8d46 100644
--- a/clang/include/clang/Basic/CodeGenOptions.def
+++ b/clang/include/clang/Basic/CodeGenOptions.def
@@ -428,6 +428,9 @@ CODEGENOPT(DirectAccessExternalData, 1, 0, Benign)
/// paths that reach the end of a function without executing a required return.
CODEGENOPT(StrictReturn, 1, 1, Benign)
+/// Whether we should use make unreachable trap or not.
+CODEGENOPT(TrapUnreachable, 1, 0, Benign)
+
/// Whether emit pseudo probes for sample pgo profile collection.
CODEGENOPT(PseudoProbeForProfiling, 1, 0, Benign)
diff --git a/clang/include/clang/Options/Options.td b/clang/include/clang/Options/Options.td
index f336542992993..33be1569b7e8c 100644
--- a/clang/include/clang/Options/Options.td
+++ b/clang/include/clang/Options/Options.td
@@ -4685,6 +4685,12 @@ defm strict_return : BoolFOption<"strict-return",
" of a non-void function as unreachable">,
PosFlag<SetTrue>>;
+defm trap_unreachable : BoolFOption<"trap-unreachable",
+ CodeGenOpts<"TrapUnreachable">, DefaultFalse,
+ PosFlag<SetTrue, [], [ClangOption, CC1Option],
+ "Treat unreachable instructions as traps">,
+ NegFlag<SetFalse>>;
+
let Flags = [TargetSpecific] in {
defm ptrauth_intrinsics : OptInCC1FFlag<"ptrauth-intrinsics", "Enable pointer authentication intrinsics">;
defm ptrauth_calls : OptInCC1FFlag<"ptrauth-calls", "Enable signing and authentication of all indirect calls">;
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index c5ea8c9e8c3de..5f57705ffc77e 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -466,6 +466,7 @@ static bool initTargetOptions(const CompilerInstance &CI,
Options.Hotpatch = CodeGenOpts.HotPatch;
Options.JMCInstrument = CodeGenOpts.JMCInstrument;
Options.XCOFFReadOnlyPointers = CodeGenOpts.XCOFFReadOnlyPointers;
+ Options.TrapUnreachable = CodeGenOpts.TrapUnreachable;
switch (CodeGenOpts.getVecLib()) {
case llvm::driver::VectorLibrary::NoLibrary:
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 699fc31f23946..f54af613e72b8 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -5808,6 +5808,9 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
Args.addOptInFlag(CmdArgs, options::OPT_fsplit_stack,
options::OPT_fno_split_stack);
+ Args.addOptInFlag(CmdArgs, options::OPT_ftrap_unreachable,
+ options::OPT_ftrap_unreachable);
+
// -fprotect-parens=0 is default.
if (Args.hasFlag(options::OPT_fprotect_parens,
options::OPT_fno_protect_parens, false))
diff --git a/clang/test/CodeGen/X86/unreachable-trap.c b/clang/test/CodeGen/X86/unreachable-trap.c
new file mode 100644
index 0000000000000..bea34e80dd873
--- /dev/null
+++ b/clang/test/CodeGen/X86/unreachable-trap.c
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 %s -O1 -triple=x86_64-unknown-linux-gnu -ftrap-unreachable -S -o - 2>&1 | FileCheck %s --check-prefix=TRAP
+// RUN: %clang_cc1 %s -O1 -triple=x86_64-unknown-linux-gnu -S -o - 2>&1 | FileCheck %s --check-prefix=NOTRAP
+
+// TRAP: ud2
+// NOTRAP-NOT: ud2
+
+[[noreturn]]
+void exit(int);
+
+#define NULL 0
+
+static void test(void) {
+ int *ptr = NULL;
+ *ptr = 0;
+ exit(0);
+}
+
+void foo() { test(); }
diff --git a/clang/test/Driver/clang_f_opts.c b/clang/test/Driver/clang_f_opts.c
index 5871f1580d6b7..aedbef393abcc 100644
--- a/clang/test/Driver/clang_f_opts.c
+++ b/clang/test/Driver/clang_f_opts.c
@@ -651,3 +651,8 @@
// RUN: %clang -### --target=x86_64-pc-windows-msvc -fno-strict-aliasing %s 2>&1 | FileCheck -check-prefix=CHECK-NO-STRICT-ALIASING %s
// CHECK-STRICT-ALIASING-NOT: -relaxed-aliasing
// CHECK-NO-STRICT-ALIASING: -relaxed-aliasing
+
+// RUN: %clang -### -ftrap-unreachable %s 2>&1 | FileCheck %s -check-prefix=UNREACHABLE-TRAP
+// RUN: %clang -### -fno-trap-unreachable %s 2>&1 | FileCheck %s -check-prefix=NO-UNREACHABLE-TRAP
+// UNREACHABLE-TRAP: "-ftrap-unreachable"
+// NO-UNREACHABLE-TRAP-NOT: "-ftrap-unreachable"
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| // RUN: %clang_cc1 %s -O1 -triple=x86_64-unknown-linux-gnu -ftrap-unreachable -S -o - 2>&1 | FileCheck %s --check-prefix=TRAP | ||
| // RUN: %clang_cc1 %s -O1 -triple=x86_64-unknown-linux-gnu -S -o - 2>&1 | FileCheck %s --check-prefix=NOTRAP |
There was a problem hiding this comment.
I don't really like testing the codegen option like this, but IDK another way to check that the right bit got set in the backend. Is this fine? Is there a better pattern I should follow for cases like this?
8d9929c to
7f7feb3
Compare
|
Also, I mentioned several issues in the description, but it wasn't immediately obvious which of those would actually be fixed by this. If folks know which of those (or additional) bugs should be closed with this landed let me know. Though I guess there's an argument to leave them open until we change the default? |
| Options.Hotpatch = CodeGenOpts.HotPatch; | ||
| Options.JMCInstrument = CodeGenOpts.JMCInstrument; | ||
| Options.XCOFFReadOnlyPointers = CodeGenOpts.XCOFFReadOnlyPointers; | ||
| Options.TrapUnreachable = CodeGenOpts.TrapUnreachable; |
There was a problem hiding this comment.
Should we also enable NoTrapAfterNoreturn? (Alternatively we could have multiple modes like -ftrap-unreachable=all and -ftrap-unreachable=except-noreturn or so.)
There was a problem hiding this comment.
That may be a good idea. Let me see how that looks.
There was a problem hiding this comment.
That does seem better, so I uploaded the new version.
7f7feb3 to
815adb8
Compare
| : Joined<["-"], "ftrap-unreachable=">, | ||
| Group<f_Group>, | ||
| Visibility<[ClangOption, CC1Option]>, | ||
| HelpText<"Treat unreachable instruction as traps.">, |
There was a problem hiding this comment.
I think we need to be careful with the documentation, so users don't expect the option to do more than it actually does. Even with trap-unreachable, the compiler will prune impossible codepaths... which can lead to wild jumps, in general. It's just less likely. Really, the key here is that trap-unreachable inserts traps in places where it's "free": it doesn't increase the number of dynamically executed instructions for well-defined code, just codesize.
There was a problem hiding this comment.
Thanks for pointing that out. I'll come up w/ some more accurate description tomorrow.
There was a problem hiding this comment.
I've updated the wording a bit. I also considered when it is supported and does not increase the dynamic instruction count for well-defined code. but that seemed a bit verbose. Happy to adjust the text if there is something specific you'd like to see instead.
1f698eb to
b864741
Compare
efriedma-quic
left a comment
There was a problem hiding this comment.
How does this interact with LTO?
| : Joined<["-"], "ftrap-unreachable=">, | ||
| Group<f_Group>, | ||
| Visibility<[ClangOption, CC1Option]>, | ||
| HelpText<"Replace ``llvm.unreachable`` instructions with traps, when it is supported and profitable.">, |
There was a problem hiding this comment.
We omit the trailing .. If more text is needed, consider adding it to https://clang.llvm.org/docs/UsersManual.html#controlling-code-generation near https://clang.llvm.org/docs/UsersManual.html#cmdoption-ftrap-function
There was a problem hiding this comment.
done. Thanks for pointing that out.
shafik
left a comment
There was a problem hiding this comment.
Thank you for doing this legwork, I wish we could be more aggressive but this at least moves us a little further along. Once you land this it might be nice to leave comments in the more central issues that this is now an option.
b864741 to
aa02129
Compare
Thanks for pointing that out. I've added options to make sure the right backend options get passed to the linker. Ideally, we wouldn't be relying on target options, but for now its no worse than other options. Hopefully we can get a better mechanism for tracking these types of options in the IR. |
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
We have several issues that list surprising behavior around UB. In many cases, this causes undesirable control flow, such as execution falling through to the next function (or whatever is in memory) instead of remaining within the bounds of the procedure. #174844, #48943, #146791, and #137741 all discuss a host of related issues. In #174844, it was mentioned that we have backend support for this for Rust, and at least one big class of these issues could be addressed by exposing the option to clang. This patch adds a new driver option that does just that. For now, we're leaving this option off by default, though we expect only small differences in code size or performance as a result if it were to be enabled. There will be an RFC in the future when we have more confidence this should be the default configuration. Fixes #174844
34cabe0 to
946a068
Compare
|
No objection. |
|
@nikic Can you please take a look? |

We have several issues that list surprising behavior around UB. In many
cases, this causes undesirable control flow, such as execution falling
through to the next function (or whatever is in memory) instead of
remaining within the bounds of the procedure. #174844, #48943, #146791,
and #137741 all discuss a host of related issues. In #174844, it was
mentioned that we have backend support for this for Rust, and at least
one big class of these issues could be addressed by exposing the option
to clang.
This patch adds a new driver option that does just that. For now, we're
leaving this option off by default, though we expect only small
differences in code size or performance as a result if it were to be
enabled. There will be an RFC in the future when we have more confidence
this should be the default configuration.
Fixes #174844