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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: clang/docs/SanitizerCoverage.rst:338
+
+With ``-fsanitize-coverage=control-flow`` the compiler will create a table to collect
+control flow for each function. More specifically, for each basic block in the function,
----------------
This paragraph doesn't describe how the table is encoded.

The description about null-separated is incorrect as it is actually null-ended.


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:1048
+
+void ModuleSanitizerCoverage::CollectFunctionControlFlow(Function &F) {
+  SmallVector<Constant *, 32> CFs;
----------------
Use `collectFunctionControlFlow` for new functions. `CamelCaseFunction` is legacy.

I'd prefer `createXXX` since `collectXXX` implies returning the object while the function actually creates the needed metadata.


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:1080
+
+  size_t N = CFs.size();
+  auto CFArray = CreateFunctionLocalArrayInSection(CFs.size(), F, IntptrPtrTy, SanCovCFsSectionName);
----------------
-Wunused-variable


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:1081
+  size_t N = CFs.size();
+  auto CFArray = CreateFunctionLocalArrayInSection(CFs.size(), F, IntptrPtrTy, SanCovCFsSectionName);
+  CFArray->setInitializer(ConstantArray::get(ArrayType::get(IntptrPtrTy, CFs.size()), CFs));
----------------
I'd avoid the variable in favor of `FunctionCFsArray`


================
Comment at: llvm/test/Instrumentation/SanitizerCoverage/control-flow.ll:1
+; Test -sanitizer-coverage-control-flow
+; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=3 -sanitizer-coverage-control-flow -S | FileCheck %s
----------------
End complete sentences in comments with `.`


================
Comment at: llvm/test/Instrumentation/SanitizerCoverage/control-flow.ll:6
+target triple = "x86_64-unknown-linux-gnu"
+define void @foo(i32* %a) sanitize_address {
+entry:
----------------
Avoid `i32*`. Use opaque pointers `ptr` for new tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133157