Page MenuHomePhabricator

Add a callback for __has_include and use it for dependency scanning
ClosedPublic

Authored by vsapsai on Mar 13 2017, 1:01 AM.

Details

Summary

This adds a PP callback for the has_include and has_include_next directives.

Checking for the presence of a header should add it to the list of header dependencies so this overrides the callback in the dependency scanner.

I kept the callback intentionally simple for now. If we want to extend it to track things like missing headers, just as PP->FileNotFound() does today, then that wouldn't be hard to add later.

Event Timeline

pete created this revision.Mar 13 2017, 1:01 AM
bruno accepted this revision.Mar 13 2017, 1:32 PM

Hi Pete, thanks for working on this!

LGTM with the minor comments below.

include/clang/Lex/PPCallbacks.h
264 ↗(On Diff #91520)

clang-format this one!

test/Frontend/dependency-gen-has-include.c
17 ↗(On Diff #91520)

Can you also add a testcase for __has_include_next?

This revision is now accepted and ready to land.Mar 13 2017, 1:32 PM
dexonsmith requested changes to this revision.Apr 22 2018, 6:52 PM
dexonsmith added a subscriber: dexonsmith.

I don't think this is quite right. I know at least make-based incremental builds wouldn't deal well with this.

Given t.cpp:

#if __has_include("missing.h")
#endif

t.d will be:

t.o: missing.h

Since the build system doesn't know how to generate missing.h, when t.cpp changes the build system will stop dead in its tracks.

Knowing the list of files that are potential inputs to t.o seems useful, but we should probably do that under a separate flag. And if it's a separate flag, we might consider giving it a better name than -MF...

This revision now requires changes to proceed.Apr 22 2018, 6:52 PM

I don't think this is quite right. I know at least make-based incremental builds wouldn't deal well with this.

This is actually not a novel problem w.r.t. this patch. The exact same situation comes up with Makefile-included .d files and when one of the referenced headers is removed.

This is typically solved somewhere in the build system, for example Make has -include, and Ninja and llbuild have explicit support for this situation.

I agree we might want to tread cautiously on how we change the .d output, but in this case I think it might be safe.

If we decide this isn't safe, then we may want to consider a new flag which tracks *all* anti-dependencies (file's for which Clang checked existence but did not exist), and include that here. The major concern with that approach is it is a much larger list, and while it would support being substantially more correct, it is also well beyond what people currently expect out of the build system + compiler generated deps approaches.

Given t.cpp:

#if __has_include("missing.h")
#endif

t.d will be:

t.o: missing.h

Since the build system doesn't know how to generate missing.h, when t.cpp changes the build system will stop dead in its tracks.

Knowing the list of files that are potential inputs to t.o seems useful, but we should probably do that under a separate flag. And if it's a separate flag, we might consider giving it a better name than -MF...

I don't think this is quite right. I know at least make-based incremental builds wouldn't deal well with this.

This is actually not a novel problem w.r.t. this patch. The exact same situation comes up with Makefile-included .d files and when one of the referenced headers is removed.

This is typically solved somewhere in the build system, for example Make has -include, and Ninja and llbuild have explicit support for this situation.

Of course, yes, I was wrong. Thanks for the correction.

I agree we might want to tread cautiously on how we change the .d output, but in this case I think it might be safe.

If we decide this isn't safe, then we may want to consider a new flag which tracks *all* anti-dependencies (file's for which Clang checked existence but did not exist), and include that here. The major concern with that approach is it is a much larger list, and while it would support being substantially more correct, it is also well beyond what people currently expect out of the build system + compiler generated deps approaches.

Even though this is likely safe, it seems really noisy. Consider the case where someone has __has_include(<missing>) -- we'll get an entry for every -I, -F, -isystem, and -iframework. If we're going to add that noise, I prefer a separate flag that covers all anti-dependencies.

test/Frontend/dependency-gen-has-include.c
11–15 ↗(On Diff #91520)

This covers quoted includes when there is a single -I. I think there are a couple of other cases worth testing:

  • Multiple -I arguments: all should be listed.
  • Angle includes: all -isystem should be listed too.
  • Also -F and -iframework.
pete added a comment.Apr 23 2018, 9:50 AM

I don't think this is quite right. I know at least make-based incremental builds wouldn't deal well with this.

This is actually not a novel problem w.r.t. this patch. The exact same situation comes up with Makefile-included .d files and when one of the referenced headers is removed.

This is typically solved somewhere in the build system, for example Make has -include, and Ninja and llbuild have explicit support for this situation.

Of course, yes, I was wrong. Thanks for the correction.

I agree we might want to tread cautiously on how we change the .d output, but in this case I think it might be safe.

If we decide this isn't safe, then we may want to consider a new flag which tracks *all* anti-dependencies (file's for which Clang checked existence but did not exist), and include that here. The major concern with that approach is it is a much larger list, and while it would support being substantially more correct, it is also well beyond what people currently expect out of the build system + compiler generated deps approaches.

Even though this is likely safe, it seems really noisy. Consider the case where someone has __has_include(<missing>) -- we'll get an entry for every -I, -F, -isystem, and -iframework. If we're going to add that noise, I prefer a separate flag that covers all anti-dependencies.

Oh, that actually wasn't my intention when I wrote it.

Honestly I didn't expect it to log anything for missing paths at all, as we don't currently log all the missing (but attempted) paths for regular #include's.

Would it be ok to turn this on by default, without a flag, only in the case of the path actually existing, and only the found path being the one we add to the .d?

Oh, that actually wasn't my intention when I wrote it.

Honestly I didn't expect it to log anything for missing paths at all, as we don't currently log all the missing (but attempted) paths for regular #include's.

Then I misunderstood the patch. Maybe add a test or two that confirms it only adds dependencies, not anti-dependencies?

Would it be ok to turn this on by default, without a flag, only in the case of the path actually existing, and only the found path being the one we add to the .d?

I think that pessimizes some incremental builds:

  • You have a __has_include("missing.h"), but don't include missing.h.
  • Change "missing.h" (but don't delete it).
  • An incremental build now has to rebuild the object file, even though nothing will have changed.

However, it's fixing an actual bug, so it makes sense to me to be more conservative.

Would it be ok to turn this on by default, without a flag, only in the case of the path actually existing, and only the found path being the one we add to the .d?

I think that pessimizes some incremental builds:

  • You have a __has_include("missing.h"), but don't include missing.h.
  • Change "missing.h" (but don't delete it).
  • An incremental build now has to rebuild the object file, even though nothing will have changed.

However, it's fixing an actual bug, so it makes sense to me to be more conservative.

To be clear, I meant "yes" by that!

rsmith added a subscriber: rsmith.Apr 30 2018, 7:09 PM
rsmith added inline comments.
include/clang/Lex/PPCallbacks.h
263 ↗(On Diff #91520)

This callback seems pretty unhelpful in the case where lookup of the file failed (you just get a source location). Could we pass in the FileName and IsAngled flag too (like we do for InclusionDirective)? I think that's what (eg) a callback tracking anti-dependencies (for a more sophisticated build system that can handle them) would want.

lib/Frontend/DependencyFile.cpp
325 ↗(On Diff #91520)

Have you thought about whether we should add a dependency even for a missing file under -MG (AddMissingHeaderDeps) mode? I think it's probably better to not do so (ie, the behavior in this patch), but it seems worth considering.

vsapsai commandeered this revision.Sep 14 2018, 4:04 PM
vsapsai added a reviewer: pete.

Taking over the change, I'll address the reviewers' comments.

vsapsai updated this revision to Diff 165610.Sep 14 2018, 4:06 PM
  • Improve tests, fix -MMD, address comments.
vsapsai marked 3 inline comments as done.Sep 14 2018, 4:20 PM
vsapsai added inline comments.
include/clang/Lex/PPCallbacks.h
263 ↗(On Diff #91520)

Added. Also added FileType so we can ignore system headers for -MMD. And tweaked doxygen comment to be more consistent with other callbacks.

test/Frontend/dependency-gen-has-include.c
11–15 ↗(On Diff #91520)

I've added a test with system headers. Overall, my testing strategy was to test all cases for the following dimensions:

  • file exists/file doesn't exist;
  • file is user/system header;
  • __has_include/__has_include_next is used.

But I don't test the full matrix as I don't think it provides enough value.

17 ↗(On Diff #91520)

Added.

vsapsai marked 2 inline comments as done.Sep 14 2018, 4:29 PM
vsapsai added inline comments.
lib/Frontend/DependencyFile.cpp
325 ↗(On Diff #91520)

Do you know how Make uses these missing files? Or maybe where I can find more. The only documentation I found says

This feature is used in automatic updating of makefiles.

Which is not particularly illuminating.

Currently I prefer not to include not found __has_include files because they aren't really missing, it's OK if they aren't there and nothing has to be done to fix that. But I'd like to confirm if my understanding aligns with Make behaviour.

rsmith added inline comments.Sep 14 2018, 4:44 PM
lib/Frontend/DependencyFile.cpp
325 ↗(On Diff #91520)

I believe the setup is this:

  • your project uses generated headers
  • in order to do the first compilation, you need to know which source files might include those generated headers

So you use -M -MG to build your make file dependencies as a separate step before your first "real" compilation, and the -MG flag causes dependencies on the not-yet-existing generated files to be emitted by the compiler.

Given that the purpose of -MG (as I understand it) is to cope with generated files, and that using __has_include to find a generated file would be peculiar (it's much more likely that the named file simply doesn't exist and shouldn't have a dependency generated for it), I think it's better to omit dependencies for nonexistent files for __has_include even under -MG.

rsmith added inline comments.Sep 14 2018, 4:49 PM
clang/lib/Frontend/DependencyFile.cpp
340

Should we really be using the (arbitrary) name of the file from the FileEntry rather than the name as written? It looks like the corresponding code for InclusionDirective uses the name as written instead.

vsapsai added inline comments.Sep 14 2018, 5:15 PM
clang/lib/Frontend/DependencyFile.cpp
300–304

This was the role model.

340

InclusionDirective covers only cases when file is not found. Existing files are handled in FileChanged. Advantage of using File->getName() is that for input with absolute paths file name will be absolute too.

rsmith accepted this revision.Sep 14 2018, 5:37 PM
rsmith added inline comments.
clang/lib/Frontend/DependencyFile.cpp
340

Ah right, that makes sense.

@dexonsmith, does my change address your concerns?

dexonsmith accepted this revision.Sep 17 2018, 2:59 PM

LGTM too.

This revision is now accepted and ready to land.Sep 17 2018, 2:59 PM
This revision was automatically updated to reflect the committed changes.