[cfi] Fix one -fno-sanitize-merge case, and add two TODOs

[thurstond]

-fno-sanitize-merge (introduced in
#120464) nearly works for CFI: code that calls EmitCheck will already check the merge options. This patch fixes one EmitTrapCheck call, which did not check the merge options, and for two other EmitTrapChecks, adds two TODOs that explain why it is difficult to fix them.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Thurston Dang (thurstond)

Changes

-fno-sanitize-merge (introduced in
#120464) nearly works for CFI: code that calls EmitCheck will already check the merge options. This patch fixes one EmitTrapCheck call, which did not check the merge options, and for two other EmitTrapChecks, adds two TODOs that explain why it is difficult to fix them.

---

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

2 Files Affected:

• (modified) clang/lib/CodeGen/CGClass.cpp (+2-1)
• (modified) clang/lib/CodeGen/CGExpr.cpp (+9-2)

diff --git a/clang/lib/CodeGen/CGClass.cpp b/clang/lib/CodeGen/CGClass.cpp
index 0f2422a6a665a..e7c704f9da188 100644
--- a/clang/lib/CodeGen/CGClass.cpp
+++ b/clang/lib/CodeGen/CGClass.cpp
@@ -2961,7 +2961,8 @@ void CodeGenFunction::EmitVTablePtrCheck(const CXXRecordDecl *RD,
   }
 
   if (CGM.getCodeGenOpts().SanitizeTrap.has(M)) {
-    EmitTrapCheck(TypeTest, SanitizerHandler::CFICheckFail);
+    bool NoMerge = !CGM.getCodeGenOpts().SanitizeMergeHandlers.has(M);
+    EmitTrapCheck(TypeTest, SanitizerHandler::CFICheckFail, NoMerge);
     return;
   }
 
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 4d6e776f42660..91ab542579282 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -3887,7 +3887,10 @@ void CodeGenFunction::EmitCfiCheckFail() {
   // Data == nullptr means the calling module has trap behaviour for this check.
   llvm::Value *DataIsNotNullPtr =
       Builder.CreateICmpNE(Data, llvm::ConstantPointerNull::get(Int8PtrTy));
-  EmitTrapCheck(DataIsNotNullPtr, SanitizerHandler::CFICheckFail);
+  // TODO: since there is no data, we don't know the CheckKind, and therefore
+  // cannot inspect CGM.getCodeGenOpts().SanitizeMergeHandlers. We default to
+  // NoMerge = false. Users can disable merging by disabling optimization.
+  EmitTrapCheck(DataIsNotNullPtr, SanitizerHandler::CFICheckFail, /*NoMerge=*/ false);
 
   llvm::StructType *SourceLocationTy =
       llvm::StructType::get(VoidPtrTy, Int32Ty, Int32Ty);
@@ -3926,7 +3929,11 @@ void CodeGenFunction::EmitCfiCheckFail() {
       EmitCheck(std::make_pair(Cond, Ordinal), SanitizerHandler::CFICheckFail,
                 {}, {Data, Addr, ValidVtable});
     else
-      EmitTrapCheck(Cond, SanitizerHandler::CFICheckFail);
+      // TODO: we can't rely on CGM.getCodeGenOpts().SanitizeMergeHandlers.
+      // Although the compiler allows SanitizeMergeHandlers to be set
+      // independently of CGM.getLangOpts().Sanitize, Driver/SanitizerArgs.cpp
+      // requires that SanitizeMergeHandlers is a subset of Sanitize.
+      EmitTrapCheck(Cond, SanitizerHandler::CFICheckFail, /*NoMerge=*/ false);
   }
 
   FinishFunction();

[github-actions]

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