Page MenuHomePhabricator

[cmake] By default do not build compiler-rt with PGO instrumentation or use
ClosedPublic

Authored by zhizhouy on Mar 2 2020, 6:04 PM.

Details

Summary

Currently compiler-rt doesn't officially support either PGO instrumentation or use PGO profdata to build it.

PGO related flags are passed into compiler-rt since rL372209, and causing
bugs: 45022, crbug:1018840

This patch adds several checks in compiler-rt to disable PGO related flags and provides a flag to turn on PGO for compiler-rt if needed.

Diff Detail

Event Timeline

zhizhouy created this revision.Mar 2 2020, 6:04 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 2 2020, 6:04 PM
Herald added subscribers: llvm-commits, Restricted Project, mgorny, dberris. · View Herald Transcript
xur added a comment.Mar 4 2020, 2:28 PM

From crbug:1018840, it looks libfuzzer does not work with instrumentation. Do you know the reason.
I tried clang PGO bootstrap that enables PGO instrumentation in compiler-rt for stage2 compiler. Everything works fine -- i.e. profile runtime functions in compiel-rt generate a correct profile.

Can we put this change under an option? I can image someone might want the profile for the compiler-rt runtime.

zhizhouy updated this revision to Diff 248566.Mar 5 2020, 12:00 PM
zhizhouy retitled this revision from [cmake] Do not build compiler-rt with PGO instrumentation or use to [cmake] By default do not build compiler-rt with PGO instrumentation or use.
zhizhouy edited the summary of this revision. (Show Details)
zhizhouy added reviewers: russell.gallop, hans.

From crbug:1018840, it looks libfuzzer does not work with instrumentation. Do you know the reason.

Discussed with Rong offline, we can reproduce it with a instrumented clang+compiler-rt and run clang -fsanitize=fuzzer test.c.

The compiler cannot resolve the __llvm_profile_instrument_target symbol inside libclang_rt.fuzzer-x86_64.a and we need to manually pass -fprofile-generate to fuzzer.

xur added a comment.Mar 5 2020, 2:39 PM

The problem here is libclang_rt.fuzzer-x86_64.a is instrumented and contains calls to profile runtime functions like llvm_profile_instrument_target.
When building with fuzzing, libclang_rt.fuzzer-x86_64.a is linked in. We also have the definition of
llvm_profile_instrument_target in the compiler.
But I don't think the compiler knows how to resolve this symbol to itself.

I think this could happen to any library, not just compiler runtime.

It seems to me if one builds an library with instrumentation, one needs to link the profile-runtime library manually.
For this case, if I add -fprofile-generate to the fuzzer, the build would pass.

Do you have to build compiler_rt in instrumentation build? Is it a way to avoid?

If not, this patch certainly would help. Especially this is under an option and sets to false by default.

Do you have to build compiler_rt in instrumentation build? Is it a way to avoid?

Specifically in ChromeOS, due to build system limit for now, we have to build compiler_rt with clang at the same time and only once.

If we only build an instrumented clang, ChromeOS will remove the bootstrap version compiler_rt after the build, thus lead to a broken toolchain.

Because of the behavior of ChromeOS, I would say this patch would help.

xur accepted this revision.Mar 5 2020, 3:36 PM

One correction to my previous comment: the default of NOT COMPILER_RT_ENABLE_PGO is true. But I think it's OK, as far as we have a way to build the instrumented version of compiler_rt.

Please do indicate this (the change of default behavior) in your commit message.

This patch looks good to me.

This revision is now accepted and ready to land.Mar 5 2020, 3:36 PM
This revision was automatically updated to reflect the committed changes.