This is an archive of the discontinued LLVM Phabricator instance.

Add -fsanitizer-coverage=control-flow
ClosedPublic

Authored by Navidem on Sep 1 2022, 2:30 PM.

Diff Detail

Unit TestsFailed

Event Timeline

Navidem created this revision.Sep 1 2022, 2:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2022, 2:30 PM
Navidem requested review of this revision.Sep 1 2022, 2:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2022, 2:30 PM
Navidem updated this revision to Diff 457420.Sep 1 2022, 3:10 PM

Bring in the missing commits.

Navidem added a reviewer: kcc.Sep 1 2022, 3:11 PM
Navidem retitled this revision from Add test for -sanitizer-coverage-control-flow to Add -sanitizer-coverage-control-flow.Sep 1 2022, 3:19 PM
kcc added a comment.Sep 1 2022, 4:11 PM

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
1055

remove if not needed any more

1057

declare at the first point of use

1061

"can not" ?

1068

hmmm... is it even possible?

1077–1078

typical LLVM code uses this:

if (CallBase *CB = ...)) { ...}
Navidem marked 4 inline comments as done.Sep 1 2022, 4:43 PM
Navidem added inline comments.
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
1061

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

1068

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.

Navidem marked an inline comment as done.Sep 1 2022, 4:43 PM
kcc added inline comments.Sep 1 2022, 4:45 PM
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
1068

yea, please simplify. ok to leave an assert

Navidem marked an inline comment as done.Sep 1 2022, 4:52 PM
Navidem added inline comments.
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
1068

Happy to!

Navidem marked an inline comment as done.Sep 1 2022, 4:53 PM
Navidem updated this revision to Diff 457466.Sep 1 2022, 5:47 PM

Apply comments.

vitalybuka added inline comments.Sep 1 2022, 6:01 PM
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
1055

something more specific, or just remove it?

1061

what does happen?

1062

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

1063

static_cast or dyn_cast

1092–1093
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

the test is gone?

the test is gone?

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

Navidem updated this revision to Diff 457734.Sep 2 2022, 5:33 PM

Updated docs, lit test, and added clang option.

Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2022, 5:33 PM
Navidem marked 3 inline comments as done.Sep 2 2022, 5:36 PM
Navidem added inline comments.Sep 2 2022, 5:43 PM
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
1061

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

1063

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

Navidem added inline comments.Sep 2 2022, 6:22 PM
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 updated this revision to Diff 457754.Sep 2 2022, 7:17 PM

Added an initial run-time test

Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2022, 7:17 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vitalybuka added inline comments.Sep 2 2022, 8:47 PM
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

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

1069

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

1070
1083

getNullValue

MaskRay added inline comments.Sep 3 2022, 2:15 PM
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
1054

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.

1086

-Wunused-variable

1087

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.

Add -sanitizer-coverage-control-flow

Add -fsanitizer-coverage=control-flow

Navidem retitled this revision from Add -sanitizer-coverage-control-flow to Add -sanitizer-coverage=control-flow.Sep 6 2022, 2:39 PM
Navidem retitled this revision from Add -sanitizer-coverage=control-flow to Add -fsanitizer-coverage=control-flow.
Navidem updated this revision to Diff 458321.Sep 6 2022, 5:04 PM

Apply comments.

Navidem marked 6 inline comments as done.Sep 6 2022, 5:05 PM
vitalybuka added inline comments.Sep 6 2022, 10:09 PM
clang/lib/Driver/SanitizerArgs.cpp
807

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

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

Navidem updated this revision to Diff 458958.Sep 8 2022, 9:52 PM

Add __sanitizer_cov_cfs_init

Navidem updated this revision to Diff 459157.Sep 9 2022, 11:58 AM

Update doc

Navidem marked an inline comment as done and an inline comment as not done.Sep 9 2022, 11:59 AM
Navidem added inline comments.
clang/lib/Driver/SanitizerArgs.cpp
807

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

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

vitalybuka added inline comments.Sep 9 2022, 2:09 PM
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);
this was can avoid this magic encoding completely?

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.

vitalybuka added inline comments.Sep 9 2022, 2:30 PM
clang/docs/SanitizerCoverage.rst
382

Works for me either way. @kcc WDYT?

Navidem updated this revision to Diff 459201.Sep 9 2022, 2:31 PM

Bring back the runtime test.

clang/lib/Driver/SanitizerArgs.cpp
807

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

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.

kcc added inline comments.Sep 13 2022, 4:16 PM
clang/docs/SanitizerCoverage.rst
382

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 added inline comments.Sep 13 2022, 5:23 PM
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 updated this revision to Diff 460215.Sep 14 2022, 1:57 PM

Update the rt test

Navidem marked an inline comment as done.Sep 14 2022, 1:57 PM
vitalybuka added inline comments.Sep 14 2022, 2:04 PM
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 accepted this revision.Sep 14 2022, 2:09 PM

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.

This revision is now accepted and ready to land.Sep 14 2022, 2:09 PM
Navidem updated this revision to Diff 460232.Sep 14 2022, 2:26 PM

Enhance rt test.

Navidem marked an inline comment as done.Sep 14 2022, 2:26 PM
Navidem added inline comments.
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 added inline comments.Sep 14 2022, 2:35 PM
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 added inline comments.Sep 14 2022, 2:43 PM
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 updated this revision to Diff 460251.Sep 14 2022, 3:22 PM

Use CHECK-DAG and separate function for checking.

Navidem marked 3 inline comments as done.Sep 14 2022, 3:23 PM
Navidem added inline comments.
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 accepted this revision.Sep 14 2022, 4:39 PM

LGTM

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

clang-format

MaskRay accepted this revision.Sep 15 2022, 3:52 PM
MaskRay added inline comments.
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
155

false can be removed

vitalybuka added inline comments.Sep 15 2022, 3:54 PM
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
155

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

This revision was landed with ongoing or failed builds.Sep 15 2022, 3:56 PM
This revision was automatically updated to reflect the committed changes.