Details
- Reviewers
- kcc - vitalybuka - MaskRay 
- Commits
- rG3e52c0926c22: Add -fsanitizer-coverage=control-flow
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Cool!
please add the documentation and the run-time test to the same CL.
A'll let Vitaly do the next pass of the code, but will review the documentation.
| llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp | ||
|---|---|---|
| 1049 | remove if not needed any more | |
| 1051 | declare at the first point of use | |
| 1055 | "can not" ? | |
| 1062 | hmmm... is it even possible? | |
| 1071–1072 | typical LLVM code uses this: if (CallBase *CB = ...)) { ...} | |
| llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp | ||
|---|---|---|
| 1055 | Actually I started with "can not", but saw the error message from opt saying "may not". Changing anyways :) | |
| 1062 | I was being cautious here for the branches (like goto) to the beginning of the function. something like the following code breaks the entry basic block: int foo (int x) {
top:
  x += 5;
  if (x > 5)
    bar(x);
  else
    goto top;
  return x;
}define dso_local i32 @foo(i32 noundef %x) #0 {
entry:
  %x.addr = alloca i32, align 4
  store i32 %x, ptr %x.addr, align 4
  br label %top
top:                                              ; preds = %if.else, %entry
  %0 = load i32, ptr %x.addr, align 4
  %add = add nsw i32 %0, 5
  store i32 %add, ptr %x.addr, align 4
  %1 = load i32, ptr %x.addr, align 4
  %cmp = icmp sgt i32 %1, 5
  br i1 %cmp, label %if.then, label %if.else
if.then:                                          ; preds = %top
  %2 = load i32, ptr %x.addr, align 4
  call void @bar(i32 noundef %2)
  br label %if.end
if.else:                                          ; preds = %top
  br label %top
if.end:                                           ; preds = %if.then
  %3 = load i32, ptr %x.addr, align 4
  ret i32 %3
}If you think it is impossible, I am fine with simplifying the code here. | |
| llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp | ||
|---|---|---|
| 1062 | yea, please simplify. ok to leave an assert | |
| llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp | ||
|---|---|---|
| 1062 | Happy to! | |
| llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp | ||
|---|---|---|
| 1049 | something more specific, or just remove it? | |
| 1055 | what does happen? | |
| 1056 | Here, I guess, "C ? A : B" operator is nicer | |
| 1057 | static_cast or dyn_cast | |
| 1086–1087 | ||
| llvm/test/Instrumentation/SanitizerCoverage/control-flow.ll | ||
| 3 | you don't have to, but may try to generate test with It could be reasonable, or not | |
| 12 | the test does not cover CallBase | |
Sorry, first time user of arcanist here! The test file is missed from this upgrade somehow :/
| llvm/test/Instrumentation/SanitizerCoverage/control-flow.ll | ||
|---|---|---|
| 3 | Thanks! I tried it and it added many more checks, which does not seem very useful? Please let me know if you see some value in having these: +; CHECK-LABEL: @foo( +; CHECK-NEXT: entry: +; CHECK-NEXT: call void @__sanitizer_cov_trace_pc_guard(i32* getelementptr inbounds ([3 x i32], [3 x i32]* @__sancov_gen_.1, i32 0, i32 0)) #[[ATTR2:[0-9]+]] +; CHECK-NEXT: [[TOBOOL:%.*]] = icmp eq i32* [[A:%.*]], null +; CHECK-NEXT: br i1 [[TOBOOL]], label [[ENTRY_IF_END_CRIT_EDGE:%.*]], label [[IF_THEN:%.*]] +; CHECK: entry.if.end_crit_edge: +; CHECK-NEXT: call void @__sanitizer_cov_trace_pc_guard(i32* inttoptr (i64 add (i64 ptrtoint ([3 x i32]* @__sancov_gen_.1 to i64), i64 4) to i32*)) #[[ATTR2]] +; CHECK-NEXT: br label [[IF_END:%.*]] +; CHECK: if.then: +; CHECK-NEXT: call void @__sanitizer_cov_trace_pc_guard(i32* inttoptr (i64 add (i64 ptrtoint ([3 x i32]* @__sancov_gen_.1 to i64), i64 8) to i32*)) #[[ATTR2]] +; CHECK-NEXT: store i32 0, i32* [[A]], align 4 +; CHECK-NEXT: call void @foo(i32* [[A]]) +; CHECK-NEXT: br label [[IF_END]] +; CHECK: if.end: +; CHECK-NEXT: ret void +; | |
| compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp | ||
|---|---|---|
| 4 | this code does not use assertions, so this this flag is not needed | |
| 4 | similar tests set  REQUIRES: has_sancovcc,stable-runtime | |
| 11 | 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 | |
| 38 | you can use | |
| llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp | ||
| 1057 | Then keep as-is in this patch | |
| 1063 | wouldn't be better to prefix with count instead of separator? | |
| 1064 | ||
| 1077 | getNullValue | |
| clang/docs/SanitizerCoverage.rst | ||
|---|---|---|
| 338 | This paragraph doesn't describe how the table is encoded. The description about null-separated is incorrect as it is actually null-ended. | |
| llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp | ||
| 1048 | 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. | |
| 1080 | -Wunused-variable | |
| 1081 | I'd avoid the variable in favor of FunctionCFsArray | |
| llvm/test/Instrumentation/SanitizerCoverage/control-flow.ll | ||
| 1 | End complete sentences in comments with . | |
| 6 | Avoid i32*. Use opaque pointers ptr for new tests. | |
| clang/lib/Driver/SanitizerArgs.cpp | ||
|---|---|---|
| 807 | shouldn't this be: probably even: | |
| clang/lib/Driver/SanitizerArgs.cpp | ||
|---|---|---|
| 807 | 
 | |
| clang/docs/SanitizerCoverage.rst | ||
|---|---|---|
| 382 | we can also generate normal structs, per function, and pass them into __sanitizer_cov_cfs_init(const MyStruct* begin, const MyStruct* end); | |
| clang/lib/Driver/SanitizerArgs.cpp | ||
| 814 | why do you need CoverageEdge if enabled CoverageControlFlow? | |
we need to update
llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_coverage_interface.inc
llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_interface_internal.h
you can search by __sanitizer_cov_8bit_counters_init if anything missing
Something like SingletonCounterCoverage could be useful.
Bring back the runtime test.
| clang/lib/Driver/SanitizerArgs.cpp | ||
|---|---|---|
| 807 | 
 This makes sense, thanks! | |
| 814 | No essential need, but here I am implicitly enabling edge when instrumentation type is not specified. This was the way that I could get -fsanitizer-coverage=control-flow working without passing -fsanitizer-coverage=bb,control-flow. | |
| clang/docs/SanitizerCoverage.rst | ||
|---|---|---|
| 382 | The struct will still have some variable length arrays, right? MyStruct will have to be defined there, and there is no header file for sancov for users to include, and I don't think I want one. | |
| compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp | ||
|---|---|---|
| 2 | I suggest to make this test smaller: 
 
 
 
 | |
LGTM as-is
| compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp | ||
|---|---|---|
| 16 | actually printing can be don't in follow up patch, but it may simplify the test, you will not need iterate it in the test. | |
| compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp | ||
|---|---|---|
| 16 | 
 I'm not sure I follow what you are suggesting. | |
| compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp | ||
|---|---|---|
| 25 | syntax nit: auto main_ptr = &main | |
| 34 | I suggest you move this to a separate function, called from main() | |
| 68 | I don't think you are guaranteed to have main and foo in this order, and similarly dir vs indir call in this order.  | |
| llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp | ||
|---|---|---|
| 154 | false can be removed | |
| llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp | ||
|---|---|---|
| 154 | I'll keep it for consistency with the rest of file | |
This paragraph doesn't describe how the table is encoded.
The description about null-separated is incorrect as it is actually null-ended.