Add -fsanitizer-coverage=control-flow

[Navidem]

Bring in the missing commits.

[kcc]

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 ↗

(On Diff #457420)

remove if not needed any more

1051 ↗

(On Diff #457420)

declare at the first point of use

1055 ↗

(On Diff #457420)

"can not" ?

1062 ↗

(On Diff #457420)

hmmm... is it even possible?

1071–1072 ↗

(On Diff #457420)

typical LLVM code uses this:

if (CallBase *CB = ...)) { ...}

[Navidem]

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

1055 ↗

(On Diff #457420)

Actually I started with "can not", but saw the error message from opt saying "may not". Changing anyways :)

1062 ↗

(On Diff #457420)

I was being cautious here for the branches (like goto) to the beginning of the function.
But later once checked a concrete example, I was not able to produce such scenario.

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.

[kcc]

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

1062 ↗

(On Diff #457420)

yea, please simplify. ok to leave an assert

[Navidem]

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

1062 ↗

(On Diff #457420)

Happy to!

[Navidem]

Apply comments.

[vitalybuka]

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

1049 ↗

(On Diff #457420)

something more specific, or just remove it?

1055 ↗

(On Diff #457420)

what does happen?

1056 ↗

(On Diff #457420)

Here, I guess, "C ? A : B" operator is nicer

1057 ↗

(On Diff #457420)

static_cast or dyn_cast

1086–1087 ↗

(On Diff #457420)

llvm/test/Instrumentation/SanitizerCoverage/control-flow.ll

3

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

12

the test does not cover CallBase

[vitalybuka]

the test is gone?

[Navidem]

In D133157#3765866, @vitalybuka wrote:

the test is gone?

Sorry, first time user of arcanist here! The test file is missed from this upgrade somehow :/

[Navidem]

Updated docs, lit test, and added clang option.

[Navidem]

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

1055 ↗

(On Diff #457420)

Gives invalid IR error if blockaddress is used with function entry as arg.

1057 ↗

(On Diff #457420)

Please check lines 739-746. This usage pattern is already there. Should we change those as well?

[Navidem]

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
+;

[Navidem]

Added an initial run-time test

[vitalybuka]

compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp

4 ↗

(On Diff #457754)

this code does not use assertions, so this this flag is not needed

4 ↗

(On Diff #457754)

similar tests set

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

11 ↗

(On Diff #457754)

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 ↗

(On Diff #457754)

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

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

1063 ↗

(On Diff #457754)

wouldn't be better to prefix with count instead of separator?

1064 ↗

(On Diff #457754)

1077 ↗

(On Diff #457754)

getNullValue

1057 ↗

(On Diff #457420)

Then keep as-is in this patch
you are welcome to fix this in followup patch :)

[MaskRay]

clang/docs/SanitizerCoverage.rst

338 ↗

(On Diff #457754)

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 ↗

(On Diff #457754)

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 ↗

(On Diff #457754)

-Wunused-variable

1081 ↗

(On Diff #457754)

I'd avoid the variable in favor of FunctionCFsArray

llvm/test/Instrumentation/SanitizerCoverage/control-flow.ll

2

End complete sentences in comments with .

7

Avoid i32*. Use opaque pointers ptr for new tests.

[MaskRay]

Add -sanitizer-coverage-control-flow

Add -fsanitizer-coverage=control-flow

[Navidem]

Apply comments.

[vitalybuka]

clang/lib/Driver/SanitizerArgs.cpp

807 ↗

(On Diff #458321)

shouldn't this be:
,[trace-pc-guard|trace-pc],[control-flow]

probably even:
[,(trace-pc-guard|trace-pc)][,control-flow]

[Navidem]

Add __sanitizer_cov_cfs_init

[Navidem]

Update doc

[Navidem]

clang/lib/Driver/SanitizerArgs.cpp

807 ↗

(On Diff #458321)

shouldn't this be:
,[trace-pc-guard|trace-pc],[control-flow]

probably even:
[,(trace-pc-guard|trace-pc)][,control-flow]

[vitalybuka]

clang/docs/SanitizerCoverage.rst

382 ↗

(On Diff #459157)

we can also generate normal structs, per function, and pass them into __sanitizer_cov_cfs_init(const MyStruct* begin, const MyStruct* end);
this was can avoid this magic encoding completely?

clang/lib/Driver/SanitizerArgs.cpp

814 ↗

(On Diff #459157)

why do you need CoverageEdge if enabled CoverageControlFlow?

[vitalybuka]

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.

[vitalybuka]

clang/docs/SanitizerCoverage.rst

382 ↗

(On Diff #459157)

Works for me either way. @kcc WDYT?

[Navidem]

Bring back the runtime test.

clang/lib/Driver/SanitizerArgs.cpp

814 ↗

(On Diff #459157)

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.

807 ↗

(On Diff #458321)

shouldn't this be:
,[trace-pc-guard|trace-pc],[control-flow]

This makes sense, thanks!

[kcc]

clang/docs/SanitizerCoverage.rst

382 ↗

(On Diff #459157)

The struct will still have some variable length arrays, right?
Will it be any better?

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.

[kcc]

compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp

1 ↗

(On Diff #459201)

I suggest to make this test smaller:

• foo() can by empty (but avoid inlining)
• main() can have a single conditional call to foo

• main should also have a single conditional indirect call to test the -1 indir call marker.

• __sanitizer_cov_cfs_init should capture its arguments and return

• main should insepct the arguments captured in __sanitizer_cov_cfs_init and verify that it sees main() and foo(), and that main contains calls to foo() and indir function, outside of the entry BB.

[Navidem]

Update the rt test

[vitalybuka]

compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp

15 ↗

(On Diff #460215)

would you like to add some printing function as for other coverage types?

64 ↗

(On Diff #460215)

new line

[vitalybuka]

LGTM as-is

compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp

15 ↗

(On Diff #460215)

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.

[Navidem]

Enhance rt test.

[Navidem]

compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp

15 ↗

(On Diff #460215)

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.

I'm not sure I follow what you are suggesting.

[vitalybuka]

compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp

15 ↗

(On Diff #460215)

Check this out D108405
then you can hook your data into DumpCoverage()
then you can replace entire "while (pt < CFS_END) {" loop with set of CHECK:

[kcc]

compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp

24 ↗

(On Diff #460232)

syntax nit:

auto main_ptr = &main

33 ↗

(On Diff #460232)

I suggest you move this to a separate function, called from main()

67 ↗

(On Diff #460232)

I don't think you are guaranteed to have main and foo in this order, and similarly dir vs indir call in this order.
So, use CHECK-DAG instead of CHECK for these four lines.

[Navidem]

Use CHECK-DAG and separate function for checking.

[Navidem]

compiler-rt/test/sanitizer_common/TestCases/sanitizer_coverage_control_flow.cpp

15 ↗

(On Diff #460215)

Check this out D108405
then you can hook your data into DumpCoverage()
then you can replace entire "while (pt < CFS_END) {" loop with set of CHECK:

I see, thanks for the pointer. I think I leave it for later.

[kcc]

LGTM

[Navidem]

Thanks @kcc @vitalybuka, I do not have commit access.

[vitalybuka]

clang-format

[MaskRay]

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

155 ↗

(On Diff #460518)

false can be removed

[vitalybuka]

clang test

[vitalybuka]

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

155 ↗

(On Diff #460518)

I'll keep it for consistency with the rest of file

llvm/test/Instrumentation/SanitizerCoverage/control-flow.ll

+; Test -sanitizer-coverage-control-flow
+; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=3 -sanitizer-coverage-control-flow -S | FileCheck %s
+
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
+target triple = "x86_64-unknown-linux-gnu"
+define void @foo(i32* %a) sanitize_address {
+entry:
+  %tobool = icmp eq i32* %a, null
+  br i1 %tobool, label %if.end, label %if.then
+
+  if.then:                                          ; preds = %entry
+  store i32 0, i32* %a, align 4
+  br label %if.end
+
+  if.end:                                           ; preds = %entry, %if.then
+  ret void
+}
+
+; CHECK: private constant [16 x i64*] [{{.*}}@foo{{.*}}blockaddress{{.*}}blockaddress{{.*}}blockaddress{{.*}}blockaddress{{.*}}blockaddress{{.*}}blockaddress{{.*}}blockaddress{{.*}}null{{.*}}null], section "__sancov_cfs", comdat($foo), align 8
+; CHECK:      @__start___sancov_cfs = extern_weak hidden global i64*
+; CHECK-NEXT: @__stop___sancov_cfs = extern_weak hidden global i64*