Leonard Chan via llvm-dev
2020-May-14 19:08 UTC
[llvm-dev] Sancov guard semantics for usage between comdats
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/0f101c32/attachment.html>
Leonard Chan via llvm-dev
2020-May-14 19:13 UTC
[llvm-dev] Sancov guard semantics for usage between comdats
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>
Evgenii Stepanov via llvm-dev
2020-May-14 19:40 UTC
[llvm-dev] Sancov guard semantics for usage between comdats
Running coverage pass before inlining seems suboptimal. Perhaps the easiest solution is to run it later, at the end of the IR pipeline but still before the sanitizer passes. Alternatively, we could teach the inliner to merge comdat groups. I.e. if function A is inlined into function B, both with comdats, it makes sense for it to bring all other members of its comdat into B. On Thu, May 14, 2020 at 12:13 PM Leonard Chan via llvm-dev <llvm-dev at lists.llvm.org> wrote:> > 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 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: >> >> >> 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 >> >> >> 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 >> >> > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev