Page MenuHomePhabricator

clang-misexpect: a standalone tool for verifying the use of __builtin_expect with PGO data
Needs ReviewPublic

Authored by paulkirth on Sep 5 2019, 8:11 PM.

Details

Summary

This is a continuation of the work in D66324 that creates a standalone tool that can simply be run over an entire codebase through the use of a compile commands database. This simplifies the need to support custom build targets to check a project's use of __builtin_expect().

The design is fairly straightforward: use a libTooling executor to walk the compilation database, and run the compiler over each target after curating some compiler flags. Mainly, we remove any incompatible flags, and then configure the codegen options to support using the selected type of profile data. We also process the LLVMArgs, as libTooling executors seem to bypass the normal mechanisms for passing flags to the LLVM backend. Other than these few changes we rely on the existing compiler infrastructure to generate our diagnostics.

Currently the use of the CodeGenAction with the libTooling Executor seems to be surfacing some race conditions within the compiler infrastructure. For now I plan to limit the concurrency of the executor to run single threaded, but eventually, I would like to have this run with full parallelism. If a CodeGenAction can never be safely run in parallel, then it may become necessary to use an external driver script, similar to run_clang_tidy.py to take advantage of parallelism.

Diff Detail

Event Timeline

paulkirth created this revision.Sep 5 2019, 8:11 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 5 2019, 8:11 PM
clang-tools-extra/clang-misexpect/tool/ClangMisExpectMain.cpp
34

The using statements here should match clang-tools-extra/clang-misexpect/ClangMisExpect.cpp.

63

accidentally committed?

79–80

I recognize the shorthand, but it seems like it could be created earlier?

90

unnecessary

117

use OS

122

Capture/use OS

leonardchan added inline comments.Sep 6 2019, 11:11 AM
clang-tools-extra/clang-misexpect/ClangMisExpect.cpp
78

Nit: llvm_unreackable is unneeded since you cover all ProfileKinds here.

paulkirth updated this revision to Diff 219171.Sep 6 2019, 2:00 PM

Address code review

paulkirth updated this revision to Diff 219173.Sep 6 2019, 2:00 PM
paulkirth marked 5 inline comments as done.

Remove commented out code

paulkirth marked an inline comment as done.Sep 6 2019, 2:03 PM
paulkirth updated this revision to Diff 220239.Sep 14 2019, 8:47 PM

Addresses problems running the standalone tool w/ the libTooling executors.

When using the CodeGenAction and setting LLVM backend options, I found several places where data races occurred. This seems like a more significant architectural issue than mitigating access to a few global variables. To avoid these issues I've locked the executor concurrency to 1, i.e. single threaded. This prevents any data races when the executor is configuring the backends for each compiler invocation.

I've included a python script based on the run-clang-tidy.py that tidy uses. This allows the standalone tool to take advantage of parallellism without running into the data races between threads.

I have also added a new checking mechanism to ensure that PGO profiles and the command line options are compatible.

Lastly, I've included new documentation and tests for the standalone tool.

Layering feels weird here.
Can this be implemented as/limited to just a
run-clang-misexpect.py wrapper over clang itself?
That would be best IMHO.

Layering feels weird here.
Can this be implemented as/limited to just a
run-clang-misexpect.py wrapper over clang itself?
That would be best IMHO.

I discussed the concurrency issue with some folks who work on libTooling, notably Sam McCall & Dmitri Gribenko. This was the approach they suggested I follow. LibTooling also provides some nice mechanisms for curating compiler options, which is possible, but less than ideal to reimplement in a python script. There are probably more benefits that escape me at the moment, but that was the first one I thought of.

Out of curiosity, if the concurrency issue was fixed in the compiler and the python script was removed/deprecated, would you still feel the same way?

Re concurrency - you had standalone LLVMContext for each thread, right?

Layering feels weird here.
Can this be implemented as/limited to just a
run-clang-misexpect.py wrapper over clang itself?
That would be best IMHO.

I discussed the concurrency issue with some folks who work on libTooling, notably Sam McCall & Dmitri Gribenko. This was the approach they suggested I follow. LibTooling also provides some nice mechanisms for curating compiler options, which is possible, but less than ideal to reimplement in a python script. There are probably more benefits that escape me at the moment, but that was the first one I thought of.

Out of curiosity, if the concurrency issue was fixed in the compiler and the python script was removed/deprecated, would you still feel the same way?

I was actually talking/thinking about the opposite direction, only having the .py wrapper, no standalone tool;
so the opposite solution (no .py, only the tool) is tangential/has different direction.

clang-tools-extra/docs/clang-misexpect.rst
1–3

I was just thinking that i forgot to mention docs in the original review, so thanks.

That being said, i do wonder whether this docs should be reworked
into two separate pages as it is a bit misleading right now.
The bulk of this should talk about
clang's -Wmisexpect/llvm's -pgo-warn-misexpect.

Because right now it seems as-if one has to use the tool,
and as it was brought up in the lists multiple times,
[depending on the exact workflow] that is more involved
than simply passing one more warning flag to the compiler.

(Yes, after specifically looking for it, i can see that
there are mentions/hints that the tool isn't required,
but it's a bit too subtle.)

Re concurrency - you had standalone LLVMContext for each thread, right?

I believe there was, but this is just whatever happens when libtooling creates and executes a compiler invocation. It seems to me that there is some global state that ends up being shared between threads. I wasn’t able to fully trace the issue, but it at least partly involves the processing of backend options. I believe there is more, that needs to be addressed but this is probably a good place for me to start. Thanks for the suggestion.

Layering feels weird here.
Can this be implemented as/limited to just a
run-clang-misexpect.py wrapper over clang itself?
That would be best IMHO.

I discussed the concurrency issue with some folks who work on libTooling, notably Sam McCall & Dmitri Gribenko. This was the approach they suggested I follow. LibTooling also provides some nice mechanisms for curating compiler options, which is possible, but less than ideal to reimplement in a python script. There are probably more benefits that escape me at the moment, but that was the first one I thought of.

Out of curiosity, if the concurrency issue was fixed in the compiler and the python script was removed/deprecated, would you still feel the same way?

I was actually talking/thinking about the opposite direction, only having the .py wrapper, no standalone tool;
so the opposite solution (no .py, only the tool) is tangential/has different direction.

Right, I was asking if you still see this as an issue if the python script wasn’t necessary. I take from this comment that it would not make you feel differently.

I think ultimately keeping the implementation in c++ makes the most sense and can evolve with the rest of libtooling.

Re concurrency - you had standalone LLVMContext for each thread, right?

I believe there was, but this is just whatever happens when libtooling creates and executes a compiler invocation. It seems to me that there is some global state that ends up being shared between threads. I wasn’t able to fully trace the issue, but it at least partly involves the processing of backend options. I believe there is more, that needs to be addressed but this is probably a good place for me to start. Thanks for the suggestion.

Ah yes, i don't think this can work given existing interface.

Layering feels weird here.
Can this be implemented as/limited to just a
run-clang-misexpect.py wrapper over clang itself?
That would be best IMHO.

I discussed the concurrency issue with some folks who work on libTooling, notably Sam McCall & Dmitri Gribenko. This was the approach they suggested I follow. LibTooling also provides some nice mechanisms for curating compiler options, which is possible, but less than ideal to reimplement in a python script. There are probably more benefits that escape me at the moment, but that was the first one I thought of.

Out of curiosity, if the concurrency issue was fixed in the compiler and the python script was removed/deprecated, would you still feel the same way?

I was actually talking/thinking about the opposite direction, only having the .py wrapper, no standalone tool;
so the opposite solution (no .py, only the tool) is tangential/has different direction.

Right, I was asking if you still see this as an issue if the python script wasn’t necessary. I take from this comment that it would not make you feel differently.

Right. Not really a blocker, just seems counter-general - there already is a
similar script to run clang-tidy on compilation database, and this script/tool
will have the same purpose - to just ran clang -Wmisexpect+profile on compilation database.
It really seems like some generalization is missing.

I think ultimately keeping the implementation in c++ makes the most sense and can evolve with the rest of libtooling.