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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits