Page MenuHomePhabricator

Fix emission of phony dependency targets when adding extra deps
ClosedPublic

Authored by dstenb on Mar 16 2018, 8:12 AM.

Details

Summary

This commit fixes a bug where passing extra dependency entries
(using -fdepfile-entry) would result in -MP incorrectly emitting
a phony target for the input file, and no phony target for the
first extra dependency.

The extra dependencies are added first to the filename vector in
DFGImpl. That clashed with the emission of the phony targets, as
the code assumed that the first index always correspond to the
input file.

Diff Detail

Repository
rC Clang

Event Timeline

dstenb created this revision.Mar 16 2018, 8:12 AM

A small caveat with this patch is that it does not fix the case where the input file as also added as an extra dependency with -fdepfile-entry; however, I reasoned that it shouldn't really be a problem in practice. I thought that it was a good trade-off ignoring that for slightly simpler code.

dstenb added subscribers: bruno, vsapsai.

@bruno, @vsapsai: I added you since you I saw that you recently reviewed, respectively delivered, D30881. That is the only DependencyFile commit since October; although, this feels a bit orthogonal from that, so feel free to remove yourselves as reviewers (and I'm sorry for adding you in that case)!

vsapsai added inline comments.May 24 2018, 3:27 PM
lib/Frontend/DependencyFile.cpp
183–186

This is incorrect if there are duplicates in Opts.ExtraDeps. Also please update the test to have duplicate extra dependencies.

test/Frontend/dependency-gen-extradeps-phony.c
7–8

I think it would be better to have a check

// CHECK:   dependency-gen-extradeps-phony.c

Because you expect it to be there and it's not that simple to notice the colon in .c:, so it's not immediately clear how CHECK-NOT is applied here.

10–12

For these repeated CHECK-NOT please consider using FileCheck --implicit-check-not. In this case it's not that important as the test is small but it can still help to capture your intention more clearly.

dstenb updated this revision to Diff 148819.May 28 2018, 8:17 AM
dstenb marked an inline comment as done.

Addressed vsapsai's comments.

dstenb added inline comments.May 28 2018, 8:19 AM
test/Frontend/dependency-gen-extradeps-phony.c
7–8

Do you mean replace the two checks with that? Isn't there a risk that that would match with dependency-gen-extradeps-phony.c:, which the not-checks would not pick up then?

I added a CHECK-NEXT check for the input file so that we match that whole dependency entry at least.

10–12

I'll add that in the next patch (and I'll keep that in mind for future changes). Thanks!

vsapsai accepted this revision.May 28 2018, 1:11 PM

Looks good to me. Just watch the build bots in case some of them are strict with warnings and require (void)AddFilename(Filename).

As for the case when the input file is also an extra dependency, I think we can ignore that for now because extra dependencies are supposed to be sanitizer blacklists. Even if that changes in the future, I expect extra dependencies to stay orthogonal to the input.

test/Frontend/dependency-gen-extradeps-phony.c
7–8

You are right, you need to be careful to make sure that we match .c file only as a dependency and not as a target. Good solution with CHECK-NEXT. Now I'm happy with the test as expected output is clearly visible and we protect from undesired output.

This revision is now accepted and ready to land.May 28 2018, 1:11 PM

Looks good to me. Just watch the build bots in case some of them are strict with warnings and require (void)AddFilename(Filename).

Yes, I'll keep an eye out for that.

I'll submit this shortly. Thanks for the review!

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.