This is an archive of the discontinued LLVM Phabricator instance.

[clang][deps] Prevent unintended modifications of the original TU command-line
ClosedPublic

Authored by jansvoboda11 on Jun 10 2021, 7:54 AM.

Details

Summary

One of the goals of the dependency scanner is to generate command-lines that can be used to explicitly build modular dependencies of a translation unit. The only modifications to these command-lines should be for the purposes of explicit modular build.

However, the current version of dependency scanner leaks its implementation details into the command-lines.

The first problem is that the clang-scan-deps tool adjusts the original textual command-line (adding -Eonly -M -MT <target> -sys-header-deps -Wno-error -o /dev/null , removing --serialize-diagnostics) in order to set up the DependencyScanning library. This has been addressed in D103461, D104012, D104030, D104031, D104033. With these patches, the DependencyScanning library receives the unmodified CompilerInvocation, sets it up and uses it for the implicit modular build.

Finally, to prevent leaking the implementation details to the resulting command-lines, this patch generates them from the original unmodified CompilerInvocation rather than from the one that drives the implicit build.

Diff Detail

Event Timeline

jansvoboda11 requested review of this revision.Jun 10 2021, 7:54 AM
jansvoboda11 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2021, 7:54 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jansvoboda11 edited the summary of this revision. (Show Details)Jun 10 2021, 7:59 AM
jansvoboda11 edited the summary of this revision. (Show Details)
dexonsmith accepted this revision.Jun 13 2021, 1:02 PM

LGTM, with one suggestion inline.

clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
194

I wonder if it'd be better to take CompilerInvocation&& here. Then the caller is required to either pass std::move or make a deep copy at the call site, and it's perhaps more clear that there's a deep copy being made.

This revision is now accepted and ready to land.Jun 13 2021, 1:02 PM
jansvoboda11 added inline comments.Jun 14 2021, 4:29 AM
clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
194

That's much clearer indeed. Thanks for the suggestion!

This revision was landed with ongoing or failed builds.Jun 14 2021, 5:05 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jun 14 2021, 5:11 AM

Either this or your concurrent commit broke check-clang: http://45.33.8.238/linux/48839/step_7.txt

Please take a look and revert for now if it takes a while to fix.

Either this or your concurrent commit broke check-clang: http://45.33.8.238/linux/48839/step_7.txt

Please take a look and revert for now if it takes a while to fix.

This test has been intermittently failing since yesterday: https://lab.llvm.org/buildbot/#/builders/109/builds/16653
I've notified the author here: https://reviews.llvm.org/rG673c5ba58497298a684f8b8dfddbfb11cd89950e