This is an archive of the discontinued LLVM Phabricator instance.

[PCH] Fixed bugs with preamble invalidation when files change (on Windows)
ClosedPublic

Authored by cameron314 on May 10 2016, 2:48 PM.

Details

Summary

There was a bug related to remapped files, that is not strictly specific to Windows but happens more often there:
When remapped files were changed, they would not always cause the preamble's PCH to be invalidated, because the remapped path didn't necessarily match the include path (e.g. slash direction). I fixed this by moving to a llvm::sys::fs::UniqueID-based map instead of using strings for paths and hoping they match.

Diff Detail

Event Timeline

cameron314 updated this revision to Diff 56820.May 10 2016, 2:48 PM
cameron314 retitled this revision from to [PCH] Fixed bugs with preamble invalidation when files change (on Windows).
cameron314 updated this object.
cameron314 added a reviewer: rsmith.
cameron314 added a subscriber: cfe-commits.

Updated patch to include later fixes I had made after the initial change.

rsmith added inline comments.May 13 2016, 11:32 AM
lib/Basic/FileManager.cpp
304–307 ↗(On Diff #57222)

It looks like this is unreachable: IsVirtual is only ever true when IsValid is also true, and we don't reach this line if UFE.isValid(). As this is the only consumer of FileEntry::IsValid, it looks like it does nothing. Am I missing something?

lib/Frontend/ASTUnit.cpp
1402–1406

If there are multiple file names with the same inode, RB.first is an arbitrarily-chosen one of those names, and stat'ing it isn't sufficient to check that none of the other names for the file have changed. Maybe we need to store a list of filenames used for each UniqueID?

cameron314 added inline comments.May 13 2016, 2:19 PM
lib/Basic/FileManager.cpp
304–307 ↗(On Diff #57222)

Hmm, interesting, good catch. Back when I made this change UFE.IsValid was not set to true in the fall-through after this if, which is why I had to introduce IsVirtual in the first place.

But looking at the history on this file, it seems it was set to true since the very beginning. I think we'd removed it in our local fork at one point to work around a different bug, but it's back to normal in ours as well. So I guess this part of the patch is unnecessary now. Yay! I'll remove it, it wasn't very pretty anyway.

lib/Frontend/ASTUnit.cpp
1402–1406

I'm not sure I understand -- if the file changed on disk, why would it matter which name we're accessing it by?

cameron314 updated this revision to Diff 57253.May 13 2016, 2:25 PM

Removed workaround for case that can no longer happen.

rsmith added inline comments.May 13 2016, 4:26 PM
lib/Frontend/ASTUnit.cpp
1402–1406

Suppose file names A and B refer to file (inode) X, and the map contains X -> (A, hash). If B is deleted (and maybe recreated pointing at a new inode), this approach won't detect that anything has changed.

cameron314 added inline comments.May 16 2016, 10:02 AM
lib/Frontend/ASTUnit.cpp
1402–1406

Ah, I understand. The problem isn't really with the overridden file map, which still has to be by inode in order for lookups to find the right file regardless of name, but with the FilesInPreamble map, which needs to stay as a list of independent filenames in case the inodes associated to those filenames changes in between parses.

I'll make the change, thanks!

cameron314 updated this object.

This version of the patch takes into account potential changes between filenames and their unique IDs between parses.

rsmith accepted this revision.May 16 2016, 4:20 PM
rsmith edited edge metadata.

LGTM

This revision is now accepted and ready to land.May 16 2016, 4:20 PM
This revision was automatically updated to reflect the committed changes.