Page MenuHomePhabricator

[Frontend] Honor UserFilesAreVolatile flag getting file buffer in ASTUnit

Authored by yvvan on May 29 2018, 2:02 AM.

Diff Detail


Event Timeline

yvvan created this revision.May 29 2018, 2:02 AM
yvvan added a comment.May 29 2018, 2:06 AM

The precise path which leads here is yet unclear for me (it requires much more time to research) but changing this specific default value to true fixes main file from being memory mapped and therefore being blocked (on Windows)
I assume it comes from FileManager::getBufferForFile(StringRef Filename) but not sure about prior events causing it.

Usually the wrong behavior can be encountered via reparsing the file.

ilya-biryukov requested changes to this revision.May 30 2018, 6:12 AM

It's a very subtle change and it isn't clear if that's the right thing to do without understanding the problem that we're trying to solve.
Could you please elaborate on the problem?

accidental memory mapping for main file which sometimes happens through FileSystem::getBufferForFile

Why is memory mapping considered accidental and what problems does it cause?

This revision now requires changes to proceed.May 30 2018, 6:12 AM
yvvan added a comment.EditedMay 30 2018, 6:16 AM

Memory mapping locks the file on Windows. That means that in the addressed case it locks main file which we are editing. After that happens many tools struggle to do something with this file, for example git.
It's the quickest solution, probably not the best. Better one would introduce isVolatile parameter in FileSystem::getBufferForFile which should be set to true somewhere (I didn't have time to search for the specific location yet)

yvvan updated this revision to Diff 149249.May 31 2018, 1:53 AM

This is the proper fix. When we get a buffer for main file we should use the UserFilesAreVolatile flag to specify if memory mapping needs to occur or not.

yvvan retitled this revision from Treat files as volatile by default to [Frontend] Honor UserFilesAreVolatile flag getting file buffer in ASTUnit.May 31 2018, 1:56 AM
yvvan edited the summary of this revision. (Show Details)
ilya-biryukov accepted this revision.Jun 1 2018, 2:40 AM

LGTM. Is it plausible to add a unit-test for this?

This revision is now accepted and ready to land.Jun 1 2018, 2:40 AM
yvvan added a comment.Jun 1 2018, 4:12 AM

Is it plausible to add a unit-test for this?

i think I can add a unit-test for it since we have the 'getBufferKind' method in MemoryBuffer.

i think I can add a unit-test for it since we have the 'getBufferKind' method in MemoryBuffer.

That sounds good. Having a regression test that fails with descriptive messages in case anyone changes this would certainly be useful.
If that's not a lot of work, of course.

yvvan updated this revision to Diff 149683.Jun 4 2018, 1:37 AM

I could not properly test getMainBufferWithPrecompiledPreamble because it's private and requires some extra context to be called.

But there's another getBufferForFile call in ASTUnit which has similar symptoms and might cause wrong mmap behavior. I've also appended it with the same parameter 'UserFilesAreVolatile' and added unit-test for it.

yvvan updated this revision to Diff 149684.Jun 4 2018, 1:40 AM

fix indentation in unit-test

orgads added a subscriber: orgads.Jun 4 2018, 2:24 AM
orgads added inline comments.
21 ↗(On Diff #149684)

Macros are evil. Maybe use TEST_F, and make these members of the base class?

yvvan updated this revision to Diff 149697.Jun 4 2018, 2:49 AM
yvvan marked an inline comment as done.

Use fixture in unit-test

This revision was automatically updated to reflect the committed changes.