Page MenuHomePhabricator

[PCH] Fixed overridden files always invalidating preamble even when unchanged

Authored by cameron314 on May 17 2016, 2:51 PM.



Remapped files would always cause the preamble's PCH to be invalidated (even if they hadn't changed) because the file manager was replacing the remapped virtual entry (no modified time) with a real file entry (which has a modified time) when an include directive was processed. This happens when the include directive results in a slightly different path for the same file (e.g. different slash direction, which happens very often on Windows).

Note: This was initially part of D20137 but I had incorrectly applied my own patch, so the IsVirtual = true line was in the wrong spot and served no purpose (and was subsequently removed from the patch). Now it actually does something!

In order to test this, I also extended the support for using custom virtual filesystems not just for basic parses, but also PCH-enabled parses and reparses.

Diff Detail

Event Timeline

cameron314 updated this revision to Diff 57523.May 17 2016, 2:51 PM
cameron314 retitled this revision from to [PCH] Fixed overridden files always invalidating preamble even when unchanged.
cameron314 updated this object.
cameron314 added a reviewer: rsmith.
cameron314 set the repository for this revision to rL LLVM.
cameron314 added a subscriber: cfe-commits.
bruno added a subscriber: bruno.May 23 2016, 1:42 PM

Hi Cameron,

Can you add a testcase?

I'm not sure how to test this (originally I found this bug by stepping through with a debugger) -- is there a way to determine if an ASTUnit used a PCH for the preamble or not? I'd call the getMainBufferWithPrecompiledPreamble method manually but it's private.

You can probably find a way to test this by taking a look at unittests/Basic/VirtualFileSystemTest.cpp

rsmith added inline comments.May 25 2016, 1:46 PM

Rather than adding this IsVirtual flag, could you just set UFE->IsValid to true here? It looks like a simple oversight that this code fails to set the IsValid flag properly.

Thanks @bruno, I'll have a look at using a VFS for the test.


I could, but I didn't want to accidentally break something else that depends on virtual files not being valid. That's the type of change that can easily introduce a subtle bug not caught by the tests.

Semantically, is a virtual file always valid?

cameron314 updated this revision to Diff 59577.Jun 3 2016, 10:21 AM
cameron314 updated this object.
cameron314 removed rL LLVM as the repository for this revision.

It took some modifications to the ASTUnit to support a virtual file system with a PCH parse/reparse (preliminary VFS support had already been added in rL249410 but it did not support initial parses using PCHs, nor reparses), but I was finally able to write a test that checks that the reparse actually uses the PCH with my fix, and rejects the PCH (rereading everything and failing the test) without it.

This is a fairly important bug for anyone hosting clang as a library (e.g. IDEs).
Can someone have a look at this patch when they have a free moment?

rsmith added inline comments.Jun 9 2016, 1:48 PM

Yes. The IsValid flag is just supposed to mean that this file has actually been added to the UniqueRealFiles map rather than having been default-constructed by operator[].

cameron314 added inline comments.Jun 9 2016, 3:00 PM

Excellent then, I'll get rid of IsVirtual and use IsValid in place. This will condense things down to a one-line change plus a large diff for the test ^^

cameron314 updated this revision to Diff 60250.Jun 9 2016, 3:06 PM

Here's the final fix (it's the line in FileManager.cpp, plus a test).

Anyone have a few minutes to look at this?

Anyone have time to check this out this week?
It's a one-line fix, includes a test, and is for a fairly important bug :-)

cameron314 abandoned this revision.Sep 5 2017, 6:51 AM

This patch is obsolete. While waiting over a year for a review, somebody else came across the same fix (for a different manifestation of the same bug) in D27810 and managed to get it through. I think my test case and supporting changes are still useful, however, so I've submitted that separately in D37474.