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

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added inline comments.


================
Comment at: compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp:4
+// RUN: %clangxx -O0 -std=c++11 -fsanitize-coverage=control-flow %s -o %t
+// RUN: %run %t 2>&1 | FileCheck %s --implicit-check-not Assertion{{.*}}failed
+
----------------
this code does not use assertions, so this this flag is not needed


================
Comment at: compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp:4
+// RUN: %clangxx -O0 -std=c++11 -fsanitize-coverage=control-flow %s -o %t
+// RUN: %run %t 2>&1 | FileCheck %s --implicit-check-not Assertion{{.*}}failed
+
----------------
vitalybuka wrote:
> this code does not use assertions, so this this flag is not needed
similar tests set

// REQUIRES: has_sancovcc,stable-runtime
// UNSUPPORTED: i386-darwin, x86_64-darwin


================
Comment at: compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp:11
+    foo(recurse - 1);
+}
+
----------------
Could you please move this test into a separate patch, I suspect this one likely than the rest will be reverted  of platforms with limited runtime


================
Comment at: compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp:38
+// TODO(navidem): make sure this is all to be checked for the collected control flow
\ No newline at end of file

----------------
you can use
// RUN: llvm-objdump ... | FIleCheck %s


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:1063
+    }
+
+    CFs.push_back((Constant *)IRB.CreateIntToPtr(ConstantInt::get(IntptrTy, 0), IntptrPtrTy));
----------------
wouldn't be better to prefix with count instead of separator?


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:1064
+
+    CFs.push_back((Constant *)IRB.CreateIntToPtr(ConstantInt::get(IntptrTy, 0), IntptrPtrTy));
+    
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:1077
+
+    CFs.push_back((Constant *)IRB.CreateIntToPtr(ConstantInt::get(IntptrTy, 0), IntptrPtrTy));
+  }
----------------
getNullValue


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp:1057
+    if (&BB == &F.getEntryBlock())
+      CFs.push_back((Constant *)IRB.CreatePointerCast(&F, IntptrPtrTy));
+    else
----------------
Navidem wrote:
> vitalybuka wrote:
> > static_cast or dyn_cast
> Please check lines 739-746. This usage pattern is already there. Should we change those as well?
Then keep as-is in this patch
you are welcome to fix this in followup patch :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133157