[Clang][compiler-rt][UBSan] Improve __ubsan_handle_invalid_builtin

[dtcxzyw]

This patch improves error message, and fixes a copy-paste
mistake in GET_REPORT_OPTIONS argument.

Address comment #104741 (comment).

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

@llvm/pr-subscribers-clang-codegen

Author: Yingwei Zheng (dtcxzyw)

Changes

This patch removes unneeded enum BuiltinCheckKind and fixes a copy-paste mistake in __ubsan_handle_invalid_builtin.

Address comment #104741 (comment).

---

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

5 Files Affected:

• (modified) clang/lib/CodeGen/CGBuiltin.cpp (+6-14)
• (modified) clang/lib/CodeGen/CodeGenFunction.h (+2-9)
• (modified) compiler-rt/lib/ubsan/ubsan_handlers.cpp (+2-3)
• (modified) compiler-rt/lib/ubsan/ubsan_handlers.h (-8)
• (modified) compiler-rt/test/ubsan/TestCases/Misc/builtins.cpp (+7-7)

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);
 }
 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[vitalybuka]

Sorry, I realized the my request does not make much sense.

Still PR contain message improvement and a bug fix.

LGTM.