This is an archive of the discontinued LLVM Phabricator instance.

[clang-scan-deps] Add proper dependency targets
AbandonedPublic

Authored by jkorous on Sep 4 2019, 3:30 PM.

Details

Summary

I'd like to get actual targets of dependencies in the output instead of fixed "clang-scan-deps dependency" string. My primary motivation is ease of testing the tool despite non-deterministic order of targets in its output but I assume this might be useful in general.

Diff Detail

Event Timeline

jkorous created this revision.Sep 4 2019, 3:30 PM

Instead of basing the new -MT name off the output, can you base it on the name of the input filename instead to simplify this logic?

clang/tools/clang-scan-deps/ClangScanDeps.cpp
265

Nit: Doesn't need zero initialization.

Why "We need at least one -MT equivalent for the generator to work."?

See DependencyFileGenerator::attachToPreprocessor for the current logic.

jkorous marked an inline comment as done.Sep 6 2019, 5:58 PM

Instead of basing the new -MT name off the output, can you base it on the name of the input filename instead to simplify this logic?

I can but I'd like to eventually use something more robust as this will be incorrect if there's -o specified in the compilation command.

clang/tools/clang-scan-deps/ClangScanDeps.cpp
190

Ooops, wrong default.

jkorous marked an inline comment as done.Sep 6 2019, 6:01 PM
jkorous added inline comments.
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
118–119

Now that Targets can remain empty before DependencyPrinterConsumer::printDependencies is called (https://reviews.llvm.org/D67308) I can remove this.

Instead of basing the new -MT name off the output, can you base it on the name of the input filename instead to simplify this logic?

I can but I'd like to eventually use something more robust as this will be incorrect if there's -o specified in the compilation command.

You can use the number of the compilation database entry as unique ID that won't collide.

jkorous marked an inline comment as done.Sep 9 2019, 4:29 PM
jkorous retitled this revision from [RFC][clang-scan-deps] Fix dependency targets to [clang-scan-deps] Add proper dependency targets.
jkorous edited the summary of this revision. (Show Details)
jkorous updated this revision to Diff 219452.Sep 9 2019, 4:31 PM
  • rebased
  • updated tests

TODO: use compilation commands database index as target

jkorous updated this revision to Diff 219458.Sep 9 2019, 5:11 PM
  • prefix dependency targets with CDB index
arphaman accepted this revision.Sep 9 2019, 5:54 PM

LGTM, but please: add a CHECK somewhere that a CompilationComandIdx is actually emitted as a prefix (I don't see one now).

This revision is now accepted and ready to land.Sep 9 2019, 5:54 PM
tschuett added a comment.EditedSep 10 2019, 5:13 AM

Thanks for doing that! But it looks odd to attach the dependencies to the output file.

clang -MD -c foo.c -o foo.o

will attach the headers/dependencies to foo.c. foo.c includes the headers. It does not attach the headers to foo.o
It will work for make-like build systems, but it looks odd.

Thanks for doing that! But it looks odd to attach the dependencies to the output file.

clang -MD -c foo.c -o foo.o

will attach the headers/dependencies to foo.c. foo.c includes the headers. It does not attach the headers to foo.o
It will work for make-like build systems, but it looks odd.

That's a good point. I didn't think about headers and modules at all. Would you be fine with me addressing this in a separate patch?

I realized there was an issue with adding the -o /dev/null to arguemnts in case when there was a -MD flag as Driver later adds -MT with the last -o value in case there's no -MT or -MQ. That means that for headers the target would always be /dev/null.
@dexonsmith pointed out that we could just conditionally add -MT in the adjustment. Also, we can use build directory instead of CDB index.

jkorous abandoned this revision.Sep 11 2019, 5:33 PM

Abandoned in favor of: https://reviews.llvm.org/D67475

...which is both cleaner and uses appropriate target value for header file dependencies.