Skip to content

Conversation

@vitalybuka
Copy link
Collaborator

@vitalybuka vitalybuka commented Apr 24, 2025

It was introduced with original "minimal runtime"
patch without explanation:
https://reviews.llvm.org/D36810#:~:text=if%20(-,NonTrappingCfi,-)

Note, the same commit contains cfi_check_fail handler,
which can not be reached with -fsanitize-trap=cfi.

This patch makes CFI min runtime behavior consistent with UBSAN:
-fsanitize-trap= ignores -fsanitize-minimal-runtime,
-fno-sanitize-trap= with -fsanitize-minimal-runtime compiles and link
minimal runtime.

Created using spr 1.3.4
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Apr 24, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 24, 2025

@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-clangir
@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-backend-x86
@llvm/pr-subscribers-backend-directx
@llvm/pr-subscribers-mlir
@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Vitaly Buka (vitalybuka)

Changes

It was introduced with original "minimal runtime"
patch without explanation:
https://reviews.llvm.org/D36810#:~:text=if%20(-,NonTrappingCfi,-)

The same commit contains cfi_check_fail handler,
which can not be reachable with -fsanitize-trap=cfi.


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

3 Files Affected:

  • (modified) clang/lib/Driver/SanitizerArgs.cpp (-6)
  • (modified) clang/test/Driver/fsanitize.c (+5-6)
  • (modified) clang/test/Driver/sanitizer-ld.c (+3-6)
diff --git a/clang/lib/Driver/SanitizerArgs.cpp b/clang/lib/Driver/SanitizerArgs.cpp
index 94a036172d0b6..2edb200688418 100644
--- a/clang/lib/Driver/SanitizerArgs.cpp
+++ b/clang/lib/Driver/SanitizerArgs.cpp
@@ -849,12 +849,6 @@ SanitizerArgs::SanitizerArgs(const ToolChain &TC,
       D.Diag(clang::diag::err_drv_argument_not_allowed_with)
           << "-fsanitize-minimal-runtime"
           << lastArgumentForMask(D, Args, IncompatibleMask);
-
-    SanitizerMask NonTrappingCfi = Kinds & SanitizerKind::CFI & ~TrappingKinds;
-    if (NonTrappingCfi && DiagnoseErrors)
-      D.Diag(clang::diag::err_drv_argument_only_allowed_with)
-          << "-fsanitize-minimal-runtime"
-          << "-fsanitize-trap=cfi";
   }
 
   for (const auto *Arg : Args.filtered(
diff --git a/clang/test/Driver/fsanitize.c b/clang/test/Driver/fsanitize.c
index af00492314ff7..76de0a94ec591 100644
--- a/clang/test/Driver/fsanitize.c
+++ b/clang/test/Driver/fsanitize.c
@@ -961,19 +961,18 @@
 // CHECK-CFI-ABORT-MINIMAL: "-fsanitize-trap=cfi-derived-cast,cfi-icall,cfi-mfcall,cfi-unrelated-cast,cfi-nvcall,cfi-vcall"
 // CHECK-CFI-ABORT-MINIMAL: "-fsanitize-minimal-runtime"
 
-// RUN: not %clang --target=x86_64-linux-gnu -fsanitize=cfi -flto -fvisibility=hidden -fsanitize-minimal-runtime -fno-sanitize-trap=cfi -fsanitize-recover=cfi -resource-dir=%S/Inputs/resource_dir %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-NOTRAP-RECOVER-MINIMAL --
-// CHECK-CFI-NOTRAP-RECOVER-MINIMAL: '-fsanitize-minimal-runtime' only allowed with '-fsanitize-trap=cfi'
+// RUN: %clang --target=x86_64-linux-gnu -fsanitize=cfi -flto -fvisibility=hidden -fsanitize-minimal-runtime -fno-sanitize-trap=cfi -fsanitize-recover=cfi -resource-dir=%S/Inputs/resource_dir %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-NOTRAP-RECOVER-MINIMAL --
 // CHECK-CFI-NOTRAP-RECOVER-MINIMAL: "-fsanitize=cfi-derived-cast,cfi-icall,cfi-mfcall,cfi-unrelated-cast,cfi-nvcall,cfi-vcall"
 // CHECK-CFI-NOTRAP-RECOVER-MINIMAL: "-fsanitize-recover=cfi-derived-cast,cfi-icall,cfi-mfcall,cfi-unrelated-cast,cfi-nvcall,cfi-vcall"
 // CHECK-CFI-NOTRAP-RECOVER-MINIMAL: "-fsanitize-minimal-runtime"
 
-// RUN: not %clang --target=x86_64-linux-gnu -fsanitize=cfi -flto -fvisibility=hidden -fsanitize-minimal-runtime -fno-sanitize-trap=cfi -fno-sanitize-recover=cfi -resource-dir=%S/Inputs/resource_dir %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-NOTRAP-ABORT-MINIMAL
-// CHECK-CFI-NOTRAP-ABORT-MINIMAL: '-fsanitize-minimal-runtime' only allowed with '-fsanitize-trap=cfi'
+// RUN: %clang --target=x86_64-linux-gnu -fsanitize=cfi -flto -fvisibility=hidden -fsanitize-minimal-runtime -fno-sanitize-trap=cfi -fno-sanitize-recover=cfi -resource-dir=%S/Inputs/resource_dir %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-NOTRAP-ABORT-MINIMAL
 // CHECK-CFI-NOTRAP-ABORT-MINIMAL: "-fsanitize=cfi-derived-cast,cfi-icall,cfi-mfcall,cfi-unrelated-cast,cfi-nvcall,cfi-vcall"
 // CHECK-CFI-NOTRAP-ABORT-MINIMAL: "-fsanitize-minimal-runtime"
 
-// RUN: not %clang --target=x86_64-linux-gnu -fsanitize=cfi -fno-sanitize-trap=cfi-icall -flto -fvisibility=hidden -fsanitize-minimal-runtime %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-NOTRAP-MINIMAL
-// CHECK-CFI-NOTRAP-MINIMAL: error: invalid argument '-fsanitize-minimal-runtime' only allowed with '-fsanitize-trap=cfi'
+// RUN: %clang --target=x86_64-linux-gnu -fsanitize=cfi -fno-sanitize-trap=cfi-icall -flto -fvisibility=hidden -fsanitize-minimal-runtime %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-NOTRAP-MINIMAL
+// CHECK-CFI-NOTRAP-MINIMAL: "-fsanitize=cfi-derived-cast,cfi-icall,cfi-mfcall,cfi-unrelated-cast,cfi-nvcall,cfi-vcall"
+// CHECK-CFI-NOTRAP-MINIMAL: "-fsanitize-trap=cfi-derived-cast,cfi-mfcall,cfi-unrelated-cast,cfi-nvcall,cfi-vcall"
 
 // RUN: %clang --target=x86_64-linux-gnu -fsanitize=cfi -fno-sanitize-trap=cfi-icall -fno-sanitize=cfi-icall -flto -fvisibility=hidden -fsanitize-minimal-runtime -resource-dir=%S/Inputs/resource_dir %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-NOICALL-MINIMAL
 // CHECK-CFI-NOICALL-MINIMAL: "-fsanitize=cfi-derived-cast,cfi-mfcall,cfi-unrelated-cast,cfi-nvcall,cfi-vcall"
diff --git a/clang/test/Driver/sanitizer-ld.c b/clang/test/Driver/sanitizer-ld.c
index a5a4fc2320bd2..befd322d027c9 100644
--- a/clang/test/Driver/sanitizer-ld.c
+++ b/clang/test/Driver/sanitizer-ld.c
@@ -920,7 +920,7 @@
 // CHECK-CFI-MINRT-LINUX: "{{.*}}ld{{(.exe)?}}"
 
 // CFI with diagnostics links the UBSan runtime.
-// RUN: not %clang -fsanitize=cfi -fno-sanitize-trap=cfi -fsanitize-recover=cfi \
+// RUN: %clang -fsanitize=cfi -fno-sanitize-trap=cfi -fsanitize-recover=cfi \
 // RUN:     -flto -fvisibility=hidden \
 // RUN:     -fsanitize-minimal-runtime \
 // RUN:     --target=x86_64-unknown-linux -fuse-ld=ld \
@@ -928,7 +928,6 @@
 // RUN:     --sysroot=%S/Inputs/basic_linux_tree \
 // RUN:     -### %s 2>&1 \
 // RUN:   | %{filecheck} --check-prefix=CHECK-CFI-MINRT-DIAG-LINUX
-// CHECK-CFI-MINRT-DIAG-LINUX: '-fsanitize-minimal-runtime' only allowed with '-fsanitize-trap=cfi'
 // CHECK-CFI-MINRT-DIAG-LINUX: "{{.*}}ld{{(.exe)?}}"
 // CHECK-CFI-MINRT-DIAG-LINUX: "--whole-archive" "{{[^"]*}}libclang_rt.ubsan_minimal.a" "--no-whole-archive"
 
@@ -946,7 +945,7 @@
 // CHECK-CFI-MINRT-CROSS-DSO-LINUX: -export-dynamic
 
 // Cross-DSO CFI with diagnostics links just the CFI runtime.
-// RUN: not %clang -fsanitize=cfi -fsanitize-cfi-cross-dso \
+// RUN: %clang -fsanitize=cfi -fsanitize-cfi-cross-dso \
 // RUN:     -flto -fvisibility=hidden \
 // RUN:     -fsanitize-minimal-runtime \
 // RUN:     -fno-sanitize-trap=cfi -fsanitize-recover=cfi \
@@ -955,7 +954,6 @@
 // RUN:     --sysroot=%S/Inputs/basic_linux_tree \
 // RUN:     -### %s 2>&1 \
 // RUN:   | %{filecheck} --check-prefix=CHECK-CFI-MINRT-CROSS-DSO-DIAG-LINUX
-// CHECK-CFI-MINRT-CROSS-DSO-DIAG-LINUX: '-fsanitize-minimal-runtime' only allowed with '-fsanitize-trap=cfi'
 // CHECK-CFI-MINRT-CROSS-DSO-DIAG-LINUX: "{{.*}}ld{{(.exe)?}}"
 // CHECK-CFI-MINRT-CROSS-DSO-DIAG-LINUX: "--whole-archive" "{{[^"]*}}libclang_rt.cfi_diag.a" "--no-whole-archive"
 // CHECK-CFI-MINRT-CROSS-DSO-DIAG-LINUX: -export-dynamic
@@ -972,7 +970,7 @@
 // CHECK-CFI-MINRT-CROSS-DSO-ANDROID: "{{.*}}ld{{(.exe)?}}"
 
 // Cross-DSO CFI with diagnostics on Android links just the UBSAN runtime.
-// RUN: not %clang -fsanitize=cfi -fsanitize-cfi-cross-dso \
+// RUN: %clang -fsanitize=cfi -fsanitize-cfi-cross-dso \
 // RUN:     -flto -fvisibility=hidden \
 // RUN:     -fsanitize-minimal-runtime \
 // RUN:     -fno-sanitize-trap=cfi -fsanitize-recover=cfi \
@@ -981,7 +979,6 @@
 // RUN:     --sysroot=%S/Inputs/basic_android_tree \
 // RUN:     -### %s 2>&1 \
 // RUN:   | %{filecheck} --check-prefix=CHECK-CFI-MINRT-CROSS-DSO-DIAG-ANDROID
-// CHECK-CFI-MINRT-CROSS-DSO-DIAG-ANDROID: '-fsanitize-minimal-runtime' only allowed with '-fsanitize-trap=cfi'
 // CHECK-CFI-MINRT-CROSS-DSO-DIAG-ANDROID: "{{.*}}ld{{(.exe)?}}"
 // CHECK-CFI-MINRT-CROSS-DSO-DIAG-ANDROID: "{{[^"]*}}libclang_rt.ubsan_minimal.so"
 // CHECK-CFI-MINRT-CROSS-DSO-DIAG-ANDROID: "--export-dynamic-symbol=__cfi_check"

@vitalybuka vitalybuka requested review from fmayer and pcc April 24, 2025 01:17
@vitalybuka
Copy link
Collaborator Author

TODO: In a separate PRs I will add Codegen and runtime tests.

Created using spr 1.3.4

[skip ci]
Created using spr 1.3.4
@vitalybuka vitalybuka changed the base branch from users/vitalybuka/spr/main.drivercfi-allow-cfi-with-minimal-runtime to main April 24, 2025 04:13
Created using spr 1.3.4
Created using spr 1.3.4
Created using spr 1.3.4
@vitalybuka
Copy link
Collaborator Author

TODO: In a separate PRs I will add Codegen and runtime tests.

Runtime #137245
Codegen #137225

@llvmbot llvmbot added cmake Build system in general and CMake in particular backend:X86 clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:headers Headers provided by Clang, e.g. for intrinsics clang:codegen IR generation bugs: mangling, exceptions, etc. labels Apr 25, 2025
Created using spr 1.3.4
Created using spr 1.3.4
Created using spr 1.3.4
@vitalybuka vitalybuka removed cmake Build system in general and CMake in particular backend:X86 clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:headers Headers provided by Clang, e.g. for intrinsics clang:codegen IR generation bugs: mangling, exceptions, etc. libc BOLT backend:DirectX HLSL HLSL Language Support backend:SPIR-V llvm:ir ClangIR Anything related to the ClangIR project labels Apr 25, 2025
Created using spr 1.3.4
@vitalybuka vitalybuka marked this pull request as ready for review April 25, 2025 07:54
@vitalybuka vitalybuka merged commit 7aa8ece into main Apr 25, 2025
6 of 8 checks passed
@vitalybuka vitalybuka deleted the users/vitalybuka/spr/drivercfi-allow-cfi-with-minimal-runtime branch April 25, 2025 07:55
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
It was introduced with original "minimal runtime"
patch without explanation:
https://reviews.llvm.org/D36810#:~:text=if%20(-,NonTrappingCfi,-)

Note, the same commit contains `cfi_check_fail` handler,
which can not be reached with `-fsanitize-trap=cfi`.

This patch makes CFI min runtime behavior consistent with UBSAN:
`-fsanitize-trap=` ignores `-fsanitize-minimal-runtime`,
`-fno-sanitize-trap=` with `-fsanitize-minimal-runtime` compiles and
link
minimal runtime.
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 mlir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants