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

Thu May 14 12:40:44 PDT 2020

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