[PATCH] D133157: Add -sanitizer-coverage-control-flow

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:1049
+void ModuleSanitizerCoverage::CollectFunctionControlFlow(Function &F) {
+  LLVM_DEBUG(dbgs() << "I am here! " << F.getName() << "\n");
+  SmallVector<Constant *, 32> CFs;
----------------
something more specific, or just remove it?


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:1055
+  for (auto &BB: F) {
+    // blockaddress may not be used on function's entry block.
+    if (&BB == &F.getEntryBlock())
----------------
what does happen?


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:1056
+    // blockaddress may not be used on function's entry block.
+    if (&BB == &F.getEntryBlock())
+      CFs.push_back((Constant *)IRB.CreatePointerCast(&F, IntptrPtrTy));
----------------
Here, I guess, "C ? A : B" operator is nicer


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:1057
+    if (&BB == &F.getEntryBlock())
+      CFs.push_back((Constant *)IRB.CreatePointerCast(&F, IntptrPtrTy));
+    else
----------------
static_cast or dyn_cast


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:1086-1087
+  N = CFs.size();
+  auto CFArray = CreateFunctionLocalArrayInSection(N, F, IntptrPtrTy, SanCovCFsSectionName);
+  CFArray->setInitializer(ConstantArray::get(ArrayType::get(IntptrPtrTy, N), CFs));
+  CFArray->setConstant(true);
----------------



================
Comment at: llvm/test/Instrumentation/SanitizerCoverage/control-flow.ll:2
+; Test -sanitizer-coverage-control-flow
+; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=3 -sanitizer-coverage-control-flow -S | FileCheck %s
+
----------------
you don't have to, but may try to generate test with
llvm/utils/update_test_checks.py --opt-binary bin/opt llvm/test/Instrumentation/SanitizerCoverage/control-flow.ll

It could be reasonable, or not


================
Comment at: llvm/test/Instrumentation/SanitizerCoverage/control-flow.ll:11
+
+  if.then:                                          ; preds = %entry
+  store i32 0, i32* %a, align 4
----------------
the test does not cover CallBase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133157/new/

https://reviews.llvm.org/D133157