[clang] [compiler-rt] [Clang][compiler-rt][UBSan] Improve `__ubsan_handle_invalid_builtin` (PR #109088)
Vitaly Buka via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 24 15:31:04 PDT 2024
https://github.com/vitalybuka updated https://github.com/llvm/llvm-project/pull/109088
>From 255c4af822ea327b51547c5c666b172bb81c6454 Mon Sep 17 00:00:00 2001
From: Yingwei Zheng <dtcxzyw2333 at gmail.com>
Date: Wed, 18 Sep 2024 13:58:24 +0800
Subject: [PATCH 1/4] [Clang][compiler-rt][UBSan] Remove `BuiltinCheckKind`
---
clang/lib/CodeGen/CGBuiltin.cpp | 20 ++++++-------------
clang/lib/CodeGen/CodeGenFunction.h | 11 ++--------
compiler-rt/lib/ubsan/ubsan_handlers.cpp | 5 ++---
compiler-rt/lib/ubsan/ubsan_handlers.h | 8 --------
.../test/ubsan/TestCases/Misc/builtins.cpp | 14 ++++++-------
5 files changed, 17 insertions(+), 41 deletions(-)
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index a52e880a764252..7509d0eb097bb6 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -1994,11 +1994,7 @@ struct CallObjCArcUse final : EHScopeStack::Cleanup {
};
}
-Value *CodeGenFunction::EmitCheckedArgForBuiltin(const Expr *E,
- BuiltinCheckKind Kind) {
- assert((Kind == BCK_CLZPassedZero || Kind == BCK_CTZPassedZero)
- && "Unsupported builtin check kind");
-
+Value *CodeGenFunction::EmitCheckedArgForBuiltin(const Expr *E) {
Value *ArgValue = EmitScalarExpr(E);
if (!SanOpts.has(SanitizerKind::Builtin))
return ArgValue;
@@ -2008,9 +2004,7 @@ Value *CodeGenFunction::EmitCheckedArgForBuiltin(const Expr *E,
ArgValue, llvm::Constant::getNullValue(ArgValue->getType()));
EmitCheck(std::make_pair(Cond, SanitizerKind::Builtin),
SanitizerHandler::InvalidBuiltin,
- {EmitCheckSourceLocation(E->getExprLoc()),
- llvm::ConstantInt::get(Builder.getInt8Ty(), Kind)},
- std::nullopt);
+ {EmitCheckSourceLocation(E->getExprLoc())}, std::nullopt);
return ArgValue;
}
@@ -3228,9 +3222,8 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
bool HasFallback = BuiltinIDIfNoAsmLabel == Builtin::BI__builtin_ctzg &&
E->getNumArgs() > 1;
- Value *ArgValue =
- HasFallback ? EmitScalarExpr(E->getArg(0))
- : EmitCheckedArgForBuiltin(E->getArg(0), BCK_CTZPassedZero);
+ Value *ArgValue = HasFallback ? EmitScalarExpr(E->getArg(0))
+ : EmitCheckedArgForBuiltin(E->getArg(0));
llvm::Type *ArgType = ArgValue->getType();
Function *F = CGM.getIntrinsic(Intrinsic::cttz, ArgType);
@@ -3260,9 +3253,8 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
bool HasFallback = BuiltinIDIfNoAsmLabel == Builtin::BI__builtin_clzg &&
E->getNumArgs() > 1;
- Value *ArgValue =
- HasFallback ? EmitScalarExpr(E->getArg(0))
- : EmitCheckedArgForBuiltin(E->getArg(0), BCK_CLZPassedZero);
+ Value *ArgValue = HasFallback ? EmitScalarExpr(E->getArg(0))
+ : EmitCheckedArgForBuiltin(E->getArg(0));
llvm::Type *ArgType = ArgValue->getType();
Function *F = CGM.getIntrinsic(Intrinsic::ctlz, ArgType);
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index 6802dc7f0c1598..69bb58f0e617c5 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -5078,16 +5078,9 @@ class CodeGenFunction : public CodeGenTypeCache {
bool IsSubtraction, SourceLocation Loc,
CharUnits Align, const Twine &Name = "");
- /// Specifies which type of sanitizer check to apply when handling a
- /// particular builtin.
- enum BuiltinCheckKind {
- BCK_CTZPassedZero,
- BCK_CLZPassedZero,
- };
-
/// Emits an argument for a call to a builtin. If the builtin sanitizer is
- /// enabled, a runtime check specified by \p Kind is also emitted.
- llvm::Value *EmitCheckedArgForBuiltin(const Expr *E, BuiltinCheckKind Kind);
+ /// enabled, a runtime zero check is also emitted.
+ llvm::Value *EmitCheckedArgForBuiltin(const Expr *E);
/// Emit a description of a type in a format suitable for passing to
/// a runtime sanitizer handler.
diff --git a/compiler-rt/lib/ubsan/ubsan_handlers.cpp b/compiler-rt/lib/ubsan/ubsan_handlers.cpp
index 27d01653f088da..ffefbded64ad14 100644
--- a/compiler-rt/lib/ubsan/ubsan_handlers.cpp
+++ b/compiler-rt/lib/ubsan/ubsan_handlers.cpp
@@ -634,12 +634,11 @@ static void handleInvalidBuiltin(InvalidBuiltinData *Data, ReportOptions Opts) {
ScopedReport R(Opts, Loc, ET);
Diag(Loc, DL_Error, ET,
- "passing zero to %0, which is not a valid argument")
- << ((Data->Kind == BCK_CTZPassedZero) ? "ctz()" : "clz()");
+ "passing zero to __builtin_ctz/clz, which is not a valid argument");
}
void __ubsan::__ubsan_handle_invalid_builtin(InvalidBuiltinData *Data) {
- GET_REPORT_OPTIONS(true);
+ GET_REPORT_OPTIONS(false);
handleInvalidBuiltin(Data, Opts);
}
void __ubsan::__ubsan_handle_invalid_builtin_abort(InvalidBuiltinData *Data) {
diff --git a/compiler-rt/lib/ubsan/ubsan_handlers.h b/compiler-rt/lib/ubsan/ubsan_handlers.h
index bae661a56833dd..4b37f9916f3da1 100644
--- a/compiler-rt/lib/ubsan/ubsan_handlers.h
+++ b/compiler-rt/lib/ubsan/ubsan_handlers.h
@@ -154,16 +154,8 @@ struct ImplicitConversionData {
RECOVERABLE(implicit_conversion, ImplicitConversionData *Data, ValueHandle Src,
ValueHandle Dst)
-/// Known builtin check kinds.
-/// Keep in sync with the enum of the same name in CodeGenFunction.h
-enum BuiltinCheckKind : unsigned char {
- BCK_CTZPassedZero,
- BCK_CLZPassedZero,
-};
-
struct InvalidBuiltinData {
SourceLocation Loc;
- unsigned char Kind;
};
/// Handle a builtin called in an invalid way.
diff --git a/compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp b/compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
index f8f564cb7baae2..2993c7a4ce9933 100644
--- a/compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
+++ b/compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
@@ -6,25 +6,25 @@
// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
void check_ctz(int n) {
- // ABORT: builtins.cpp:[[@LINE+2]]:17: runtime error: passing zero to ctz(), which is not a valid argument
- // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to ctz(), which is not a valid argument
+ // ABORT: builtins.cpp:[[@LINE+2]]:17: runtime error: passing zero to __builtin_ctz/clz, which is not a valid argument
+ // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to __builtin_ctz/clz, which is not a valid argument
__builtin_ctz(n);
- // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to ctz(), which is not a valid argument
+ // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to __builtin_ctz/clz, which is not a valid argument
__builtin_ctzl(n);
- // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to ctz(), which is not a valid argument
+ // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to __builtin_ctz/clz, which is not a valid argument
__builtin_ctzll(n);
}
void check_clz(int n) {
- // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to clz(), which is not a valid argument
+ // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to __builtin_ctz/clz, which is not a valid argument
__builtin_clz(n);
- // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to clz(), which is not a valid argument
+ // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to __builtin_ctz/clz, which is not a valid argument
__builtin_clzl(n);
- // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to clz(), which is not a valid argument
+ // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to __builtin_ctz/clz, which is not a valid argument
__builtin_clzll(n);
}
>From c66becf3c8e1782a7cc3ee5848d259dde6d789d0 Mon Sep 17 00:00:00 2001
From: Vitaly Buka <vitalybuka at google.com>
Date: Tue, 24 Sep 2024 15:03:10 -0700
Subject: [PATCH 2/4] Undo most of the PR
---
clang/lib/CodeGen/CGBuiltin.cpp | 20 +++++++++++++------
clang/lib/CodeGen/CodeGenFunction.h | 11 ++++++++--
compiler-rt/lib/ubsan/ubsan_handlers.cpp | 3 ++-
compiler-rt/lib/ubsan/ubsan_handlers.h | 8 ++++++++
.../test/ubsan/TestCases/Misc/builtins.cpp | 14 ++++++-------
5 files changed, 40 insertions(+), 16 deletions(-)
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 7509d0eb097bb6..a52e880a764252 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -1994,7 +1994,11 @@ struct CallObjCArcUse final : EHScopeStack::Cleanup {
};
}
-Value *CodeGenFunction::EmitCheckedArgForBuiltin(const Expr *E) {
+Value *CodeGenFunction::EmitCheckedArgForBuiltin(const Expr *E,
+ BuiltinCheckKind Kind) {
+ assert((Kind == BCK_CLZPassedZero || Kind == BCK_CTZPassedZero)
+ && "Unsupported builtin check kind");
+
Value *ArgValue = EmitScalarExpr(E);
if (!SanOpts.has(SanitizerKind::Builtin))
return ArgValue;
@@ -2004,7 +2008,9 @@ Value *CodeGenFunction::EmitCheckedArgForBuiltin(const Expr *E) {
ArgValue, llvm::Constant::getNullValue(ArgValue->getType()));
EmitCheck(std::make_pair(Cond, SanitizerKind::Builtin),
SanitizerHandler::InvalidBuiltin,
- {EmitCheckSourceLocation(E->getExprLoc())}, std::nullopt);
+ {EmitCheckSourceLocation(E->getExprLoc()),
+ llvm::ConstantInt::get(Builder.getInt8Ty(), Kind)},
+ std::nullopt);
return ArgValue;
}
@@ -3222,8 +3228,9 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
bool HasFallback = BuiltinIDIfNoAsmLabel == Builtin::BI__builtin_ctzg &&
E->getNumArgs() > 1;
- Value *ArgValue = HasFallback ? EmitScalarExpr(E->getArg(0))
- : EmitCheckedArgForBuiltin(E->getArg(0));
+ Value *ArgValue =
+ HasFallback ? EmitScalarExpr(E->getArg(0))
+ : EmitCheckedArgForBuiltin(E->getArg(0), BCK_CTZPassedZero);
llvm::Type *ArgType = ArgValue->getType();
Function *F = CGM.getIntrinsic(Intrinsic::cttz, ArgType);
@@ -3253,8 +3260,9 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
bool HasFallback = BuiltinIDIfNoAsmLabel == Builtin::BI__builtin_clzg &&
E->getNumArgs() > 1;
- Value *ArgValue = HasFallback ? EmitScalarExpr(E->getArg(0))
- : EmitCheckedArgForBuiltin(E->getArg(0));
+ Value *ArgValue =
+ HasFallback ? EmitScalarExpr(E->getArg(0))
+ : EmitCheckedArgForBuiltin(E->getArg(0), BCK_CLZPassedZero);
llvm::Type *ArgType = ArgValue->getType();
Function *F = CGM.getIntrinsic(Intrinsic::ctlz, ArgType);
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index 69bb58f0e617c5..6802dc7f0c1598 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -5078,9 +5078,16 @@ class CodeGenFunction : public CodeGenTypeCache {
bool IsSubtraction, SourceLocation Loc,
CharUnits Align, const Twine &Name = "");
+ /// Specifies which type of sanitizer check to apply when handling a
+ /// particular builtin.
+ enum BuiltinCheckKind {
+ BCK_CTZPassedZero,
+ BCK_CLZPassedZero,
+ };
+
/// Emits an argument for a call to a builtin. If the builtin sanitizer is
- /// enabled, a runtime zero check is also emitted.
- llvm::Value *EmitCheckedArgForBuiltin(const Expr *E);
+ /// enabled, a runtime check specified by \p Kind is also emitted.
+ llvm::Value *EmitCheckedArgForBuiltin(const Expr *E, BuiltinCheckKind Kind);
/// Emit a description of a type in a format suitable for passing to
/// a runtime sanitizer handler.
diff --git a/compiler-rt/lib/ubsan/ubsan_handlers.cpp b/compiler-rt/lib/ubsan/ubsan_handlers.cpp
index ffefbded64ad14..cb17f6f3aec3f8 100644
--- a/compiler-rt/lib/ubsan/ubsan_handlers.cpp
+++ b/compiler-rt/lib/ubsan/ubsan_handlers.cpp
@@ -634,7 +634,8 @@ static void handleInvalidBuiltin(InvalidBuiltinData *Data, ReportOptions Opts) {
ScopedReport R(Opts, Loc, ET);
Diag(Loc, DL_Error, ET,
- "passing zero to __builtin_ctz/clz, which is not a valid argument");
+ "passing zero to __builtin_%0(), which is not a valid argument")
+ << ((Data->Kind == BCK_CTZPassedZero) ? "ctz" : "clz");
}
void __ubsan::__ubsan_handle_invalid_builtin(InvalidBuiltinData *Data) {
diff --git a/compiler-rt/lib/ubsan/ubsan_handlers.h b/compiler-rt/lib/ubsan/ubsan_handlers.h
index 4b37f9916f3da1..bae661a56833dd 100644
--- a/compiler-rt/lib/ubsan/ubsan_handlers.h
+++ b/compiler-rt/lib/ubsan/ubsan_handlers.h
@@ -154,8 +154,16 @@ struct ImplicitConversionData {
RECOVERABLE(implicit_conversion, ImplicitConversionData *Data, ValueHandle Src,
ValueHandle Dst)
+/// Known builtin check kinds.
+/// Keep in sync with the enum of the same name in CodeGenFunction.h
+enum BuiltinCheckKind : unsigned char {
+ BCK_CTZPassedZero,
+ BCK_CLZPassedZero,
+};
+
struct InvalidBuiltinData {
SourceLocation Loc;
+ unsigned char Kind;
};
/// Handle a builtin called in an invalid way.
diff --git a/compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp b/compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
index 2993c7a4ce9933..a635f7fcc686ed 100644
--- a/compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
+++ b/compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp
@@ -6,25 +6,25 @@
// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
void check_ctz(int n) {
- // ABORT: builtins.cpp:[[@LINE+2]]:17: runtime error: passing zero to __builtin_ctz/clz, which is not a valid argument
- // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to __builtin_ctz/clz, which is not a valid argument
+ // ABORT: builtins.cpp:[[@LINE+2]]:17: runtime error: passing zero to __builtin_ctz(), which is not a valid argument
+ // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to __builtin_ctz(), which is not a valid argument
__builtin_ctz(n);
- // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to __builtin_ctz/clz, which is not a valid argument
+ // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to __builtin_ctz(), which is not a valid argument
__builtin_ctzl(n);
- // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to __builtin_ctz/clz, which is not a valid argument
+ // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to __builtin_ctz(), which is not a valid argument
__builtin_ctzll(n);
}
void check_clz(int n) {
- // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to __builtin_ctz/clz, which is not a valid argument
+ // RECOVER: builtins.cpp:[[@LINE+1]]:17: runtime error: passing zero to __builtin_clz(), which is not a valid argument
__builtin_clz(n);
- // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to __builtin_ctz/clz, which is not a valid argument
+ // RECOVER: builtins.cpp:[[@LINE+1]]:18: runtime error: passing zero to __builtin_clz(), which is not a valid argument
__builtin_clzl(n);
- // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to __builtin_ctz/clz, which is not a valid argument
+ // RECOVER: builtins.cpp:[[@LINE+1]]:19: runtime error: passing zero to __builtin_clz(), which is not a valid argument
__builtin_clzll(n);
}
>From 8018a3a3973e38ba2a05341017876cc94c3505ea Mon Sep 17 00:00:00 2001
From: Vitaly Buka <vitalybuka at google.com>
Date: Tue, 24 Sep 2024 15:11:18 -0700
Subject: [PATCH 3/4] clangformat
---
compiler-rt/lib/ubsan/ubsan_handlers.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/compiler-rt/lib/ubsan/ubsan_handlers.cpp b/compiler-rt/lib/ubsan/ubsan_handlers.cpp
index cb17f6f3aec3f8..9dbe8e6c0c1745 100644
--- a/compiler-rt/lib/ubsan/ubsan_handlers.cpp
+++ b/compiler-rt/lib/ubsan/ubsan_handlers.cpp
@@ -635,7 +635,7 @@ static void handleInvalidBuiltin(InvalidBuiltinData *Data, ReportOptions Opts) {
Diag(Loc, DL_Error, ET,
"passing zero to __builtin_%0(), which is not a valid argument")
- << ((Data->Kind == BCK_CTZPassedZero) ? "ctz" : "clz");
+ << ((Data->Kind == BCK_CTZPassedZero) ? "ctz" : "clz");
}
void __ubsan::__ubsan_handle_invalid_builtin(InvalidBuiltinData *Data) {
>From 72f42c8696825fd6cf5f534ca8d7c3029b9a2bfe Mon Sep 17 00:00:00 2001
From: Vitaly Buka <vitalybuka at google.com>
Date: Tue, 24 Sep 2024 15:30:45 -0700
Subject: [PATCH 4/4] add test
---
.../TestCases/Integer/suppressions-builtin.cpp | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
create mode 100644 compiler-rt/test/ubsan/TestCases/Integer/suppressions-builtin.cpp
diff --git a/compiler-rt/test/ubsan/TestCases/Integer/suppressions-builtin.cpp b/compiler-rt/test/ubsan/TestCases/Integer/suppressions-builtin.cpp
new file mode 100644
index 00000000000000..60377c492e8cc7
--- /dev/null
+++ b/compiler-rt/test/ubsan/TestCases/Integer/suppressions-builtin.cpp
@@ -0,0 +1,18 @@
+// RUN: %clangxx -fsanitize=builtin -g0 %s -o %t
+
+// Suppression by symbol name requires the compiler-rt runtime to be able to
+// symbolize stack addresses.
+// REQUIRES: can-symbolize
+// UNSUPPORTED: android
+
+// RUN: echo "invalid-builtin-use:do_ctz" > %t.func-supp
+// RUN: %env_ubsan_opts=halt_on_error=1:suppressions='"%t.func-supp"' %run %t
+
+#include <stdint.h>
+
+extern "C" void do_ctz(int n) { __builtin_ctz(0); }
+
+int main() {
+ do_ctz(0);
+ return 0;
+}
More information about the cfe-commits
mailing list