[llvm-dev] Sancov guard semantics for usage between comdats

[Leonard Chan via llvm-dev]

Forgot to mention, but was probably implied by the patch I linked: This is
reproducible only when using the *new pass manager*. The clang invocation
should be:

```
clang++ -fsanitize-coverage=trace-pc-guard /tmp/test.cpp -c -O1
-fexperimental-new-pass-manager
```

The clang we build uses it by default.

On Thu, May 14, 2020 at 12:08 PM Leonard Chan <leonardchan at google.com>
wrote:

> Given the following C++ code:
>
> ```
>
> // test.cpp
>
> struct Foo {
>
>   int public_foo();
>
>   int outside_foo();
>
>
>   [[gnu::always_inline]] int inline_foo() {
>
> int x = outside_foo();
>
>     if (x % 17) {
>
> x += 1;
>
>     }
>
>     return x;
>
>
>   }
>
>
>   [[gnu::noinline]] int inline_bar1() {
>
> int x = inline_foo();
>
>     if (x % 23) {
>
>       x += 2;
>
>     }
>
>     return x;
>
>   }
>
>   [[gnu::noinline]] int inline_bar2() {
>
> int x = inline_foo();
>
>     if (x % 37) {
>
>       x += 3;
>
>     }
>
>     return x;
>
>   }
>
>
> };
>
>
> int Foo::public_foo() {
>
> return inline_foo() ? inline_bar1() : inline_bar2();
>
> }
>
> ```
>
> Compiling this with `clang++ -fsanitize-coverage=trace-pc-guard
> /tmp/test.cpp -c -O1`  generates sancov guards (__sancov_gen_.X) that are
> used outside of their comdat group due to inlining:
>
> ```
>
> @__sancov_gen_.1 = private global [3 x i32] zeroinitializer, section
> "__sancov_guards", comdat($_ZN3Foo10inline_fooEv)
>
> define dso_local i32 @_ZN3Foo10public_fooEv(%struct.Foo* %0)
> local_unnamed_addr #0 comdat align 2 {
>
>   call void @__sanitizer_cov_trace_pc_guard(i32* getelementptr inbounds
> ([3 x i32], [3 x i32]* @__sancov_gen_, i64 0, i64 0))
>
>   call void asm sideeffect "", ""() #4
>
>   ; This is from inlining Foo::inline_foo into Foo::public_foo
>
>   call void @__sanitizer_cov_trace_pc_guard(i32* getelementptr inbounds
> ([3 x i32], [3 x i32]* @__sancov_gen_.1, i64 0, i64 0))
>
> ...
>
> ```
>
> This can lead to a discarded section error for `__sancov_guards` when
> linking this with another TU that contains the prevalent comdat for $
> _ZN3Foo10public_fooEv. We see this here
> <https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8880544155798188656/+/steps/build/0/steps/build_fuchsia/0/steps/ninja/0/steps/zircon/0/logs/raw_io.output_failure_raw_summary_/0>
> when building with sancov.
>
> The underlying issue seems to be that symbols in a comdat group that
> aren’t the key symbol are being referenced in other comdat groups. In this
> case, @__sancov_gen_.1 is a part of comdat group $_ZN3Foo10inline_fooEv
> but is being referenced by a symbol in comdat group $
> _ZN3Foo10public_fooEv.
>
> We’re seeing this when building libc on fuchsia which uses the new pass
> manager. The commit that seemed to trigger this is
> https://reviews.llvm.org/D79698 which (I believe) would run the Sancov
> pass before optimizations, and potentially cause inlining of @
> __sancov_gen_.1 into functions outside of the comdat group it’s defined
> in.
>
> Is this explanation correct? To be consistent with the compiler’s
> behavior, we could:
>
>
>    1.
>
>    Change the instrumentation semantics with a single sancov guard for
>    both all inlined instances, forcing it to be outside a comdat group:
>
>
> ```
>
> ; Not in a comdat
>
> @__sancov_gen_.local = private global [3 x i32] zeroinitializer, section
> "__sancov_guards"
>
> define dso_local i32 @_ZN3Foo10public_fooEv(%struct.Foo* %0)
> local_unnamed_addr #0 comdat align 2 {
>
>   call void @__sanitizer_cov_trace_pc_guard(i32* getelementptr inbounds
> ([3 x i32], [3 x i32]* @__sancov_gen_, i64 0, i64 0))
>
>   call void asm sideeffect "", ""() #4
>
>   ; This is from inlining Foo::inline_foo into Foo::public_foo
>
>   call void @__sanitizer_cov_trace_pc_guard(i32* getelementptr inbounds
> ([3 x i32], [3 x i32]* @__sancov_gen_.local, i64 0, i64 0))
>
> ...
>
> ```
>
> This is likely incompatible with the intended semantics of
> instrumentation. The sancov tool expects every instrumentation call site to
> be a potential PC sample, and the way the runtime works is to deliver at
> most one PC sample for each guard. If multiple call sites share a single
> guard, then the sancov tool will consider only one of those instrumentation
> sites to have been covered even when more were actually run.
>
> Or
>
>
>    1.
>
>    Have a separate sancov guard for each instantiation of the inlined
>    function and each inline gets its own separate object inside its group just
>    like it gets its own separate instruction stream inside its group:
>
>
> ```
>
> ; This was inlined from Foo::inline_foo, but will be in the same comdat
> as the function it’s inlined into
>
> @__sancov_gen_.inlined = private global [3 x i32] zeroinitializer,
> section "__sancov_guards", comdat($_ZN3Foo10public_fooEv)
>
> define dso_local i32 @_ZN3Foo10public_fooEv(%struct.Foo* %0)
> local_unnamed_addr #0 comdat align 2 {
>
>   call void @__sanitizer_cov_trace_pc_guard(i32* getelementptr inbounds
> ([3 x i32], [3 x i32]* @__sancov_gen_, i64 0, i64 0))
>
>   call void asm sideeffect "", ""() #4
>
>   ; This is from inlining Foo::inline_foo into Foo::public_foo
>
>   call void @__sanitizer_cov_trace_pc_guard(i32* getelementptr inbounds
> ([3 x i32], [3 x i32]* @__sancov_gen_.inlined, i64 0, i64 0))
>
> ...
>
> ```
>
> This would be consistent with the semantics of the instrumentation as they
> originally existed and that the runtime and tools were designed for.
>
> Thanks,
>
> Leonard Chan
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200514/b55467aa/attachment-0001.html>