This is an archive of the discontinued LLVM Phabricator instance.

Track skipped files in dependency scanning
ClosedPublic

Authored by vsapsai on Mar 13 2017, 12:10 AM.

Details

Summary

It's possible for a header to be a symlink to another header. In this
case both will be represented by clang::FileEntry with the same UID and
they'll use the same clang::HeaderFileInfo.

If you include both headers and use some single-inclusion mechanism
like a header guard or #import, one header will get a FileChanged
callback, and another FileSkipped.

So that we get an accurate dependency file, we therefore need to also
implement the FileSkipped callback in dependency scanning.

Patch by Pete Cooper.

Event Timeline

pete created this revision.Mar 13 2017, 12:10 AM
bruno edited edge metadata.Mar 13 2017, 1:42 PM

Hi Pete,

lib/Frontend/DependencyFile.cpp
191 ↗(On Diff #91518)

Is there any FileSkipped callback invocation that might trigger an unwanted file to be added as a dependency or it will only trigger for the symlink case?

test/Frontend/dependency-gen-symlink.c
11 ↗(On Diff #91518)

It seems that it works for hard links as well right? Is this intended or do you miss a -s?

I plan to look into cleaning this up and addressing the review comments.

Need to check if DepCollectorPPCallbacks has to be updated too.

lib/Frontend/DependencyFile.cpp
302 ↗(On Diff #91518)

I suspect we need to call FileMatchesDepCriteria here too.

vsapsai commandeered this revision.May 1 2018, 3:10 PM
vsapsai added a reviewer: pete.
vsapsai updated this revision to Diff 144802.May 1 2018, 3:13 PM
  • Fix FileSkipped adding system headers when it shouldn't.
vsapsai marked 2 inline comments as done.May 1 2018, 3:20 PM
vsapsai added inline comments.
lib/Frontend/DependencyFile.cpp
191 ↗(On Diff #91518)

FileSkipped is called when a file has a mechanism for single inclusion (header guards or #import) and it was already included. The callback will be triggered also for

// a.h
#ifndef A_H_
#define A_H_
#include "b.h"
#endif

// b.h
#ifndef B_H_
#define B_H_
#endif

// test.c
#include "a.h"
#include "b.h"

But AddFilename performs Filename uniqueness check, so there will be no duplicate b.h in dependencies.

If the same file is included by multiple paths, all paths can end up in dependency file. It is actually what we want to achieve and it shouldn't cause unnecessary rebuilds.

test/Frontend/dependency-gen-symlink.c
11 ↗(On Diff #91518)

It works when files have the same inode. Added -s to capture more common use case.

vsapsai edited the summary of this revision. (Show Details)May 1 2018, 3:21 PM
bruno accepted this revision.May 1 2018, 4:09 PM

Thanks for the detailed answers. LGTM

This revision is now accepted and ready to land.May 1 2018, 4:09 PM
This revision was automatically updated to reflect the committed changes.
vsapsai marked an inline comment as done.