Page MenuHomePhabricator

Allow to store precompiled preambles in memory.
ClosedPublic

Authored by ilya-biryukov on Nov 9 2017, 6:59 AM.

Details

Summary

These preambles are built by ASTUnit and clangd. Previously, preambles
were always stored on disk.

In-memory preambles are routed back to the compiler as virtual files in
a custom VFS.

Interface of ASTUnit does not allow to use in-memory preambles, as
ASTUnit::CodeComplete receives FileManager as a parameter, so we can't
change VFS used by the compiler inside the CodeComplete method.

A follow-up commit will update clangd in clang-tools-extra to use
in-memory preambles.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.Nov 9 2017, 6:59 AM
sammccall edited edge metadata.Nov 9 2017, 8:07 AM

I'm really happy you've made this work! I don't understand it enough to do a meaningful review (keen to learn if you have time for a walkthrough when back in the office).

ioeric added a subscriber: ioeric.Nov 9 2017, 8:11 AM
klimek added inline comments.Nov 10 2017, 7:29 AM
include/clang/Frontend/PrecompiledPreamble.h
99 ↗(On Diff #122244)

...ensure *the* preamble...

lib/Frontend/ASTUnit.cpp
1028 ↗(On Diff #122244)

Since when are we using the /*ref*/ annotation?

lib/Frontend/PrecompiledPreamble.cpp
206 ↗(On Diff #122244)

It looks like we should pass in the output stream, not the storage?

490 ↗(On Diff #122244)

This looks a bit like we should push it into the PCHStorage.

ilya-biryukov marked 2 inline comments as done.
  • Fixed comments.
  • Removed /*ref*/ annotations.
  • Removed unused "Storage" variable.
  • Extract a helper function that properly sets up VFS to access the PCHStorage.
ilya-biryukov added inline comments.Nov 14 2017, 7:06 AM
lib/Frontend/ASTUnit.cpp
1028 ↗(On Diff #122244)

Oh, sorry, that's my thing. Slipped into this change.
Removed those.

lib/Frontend/PrecompiledPreamble.cpp
206 ↗(On Diff #122244)

We're not actually using the Storage variable, it's a leftover from previous versions. Removed it.

Or did you mean that we should pass in the output stream directly to PrecompilePreambleAction's constructor?

490 ↗(On Diff #122244)

I've extracted a function here to make the code read simpler.
However, I placed it directly into the PrecompiledPreamble instead of PCHStorage to keep PCHStorage responsible for just one thing: managing the variant-like union.

klimek added inline comments.Nov 15 2017, 3:29 AM
lib/Frontend/PrecompiledPreamble.cpp
206 ↗(On Diff #122244)

Yes, I'm generally looking at things that might be better to decide at a higher abstraction level and pass in, rather than having switches for behavior (like InMemStorage) all over the place. Generally, I think we should have a storage (PCHStorage sounds like it was the right abstraction, but perhaps that one currently has a bad name) and the things dealing with that storage shouldn't care whether it's in memory or on the file system.

490 ↗(On Diff #122244)

It being called PCHStorage makes it sound like it handles the abstraction for how the preamble is stored. Given that the variant-like union is basically an interface with an implementation switch, I think all switching on it is also the responsibility of that class. Otherwise we'd need another abstraction on top of it?

ilya-biryukov added inline comments.Nov 15 2017, 7:57 AM
lib/Frontend/PrecompiledPreamble.cpp
206 ↗(On Diff #122244)

It's a bit easier to share code with GeneratePCHAction this way in a way that would obviously works (as we call the same functions at the same points in the compilation pipeline, that is in CreateASTConsumer).

PCHStorage is not a public interface and PrecompiledPreamble is exactly the interface that let's you use it without caring whether PCHs are stored in memory or on disk. It also feels ok for it to have the storage-dependent code as part of its own implementation.

490 ↗(On Diff #122244)

Abstraction on top of it is PrecompiledPreamble itself. And PCHStorage is just an implementation detail.
Does that sound reasonable?

klimek accepted this revision.Nov 16 2017, 2:00 AM

LG

lib/Frontend/PrecompiledPreamble.cpp
490 ↗(On Diff #122244)

That makes more sense now, thx for explaining. It still seems a bit off to me, but I can't come up with a better solution, so LG.

44 ↗(On Diff #122838)

erasedOnReboot seems weird for something that we don't want to have on the actual file system. Why do we even want a temp directory?

This revision is now accepted and ready to land.Nov 16 2017, 2:00 AM
ilya-biryukov added inline comments.Nov 16 2017, 4:28 AM
lib/Frontend/PrecompiledPreamble.cpp
44 ↗(On Diff #122838)

Yeah, erasedOnReboot seems weird here.
What I really wanted is a filepath that's valid on all platforms and is on an existing drive for Windows, so the value of erasedOnReboot doesn't really matter. system_temp_directory might be the wrong thing to use for that in the first place. Is there a better alternative I'm missing?

klimek added inline comments.Nov 16 2017, 4:49 AM
lib/Frontend/PrecompiledPreamble.cpp
44 ↗(On Diff #122838)

Why does it need to get an existing path / existing drive?

ilya-biryukov added inline comments.Nov 16 2017, 5:15 AM
lib/Frontend/PrecompiledPreamble.cpp
44 ↗(On Diff #122838)

Oh, maybe it doesn't have to be. But I expect that a probability of something breaking is lower if we use an existing path.
Maybe I'm wrong and it will all work out just fine, but that seems to be a rather good compromise adding an extra bit of confidence that things will be working in the long-term.

klimek added inline comments.Nov 16 2017, 5:40 AM
lib/Frontend/PrecompiledPreamble.cpp
44 ↗(On Diff #122838)

I'd on the contrary say that this is more likely to fail. If we don't have a real filesystem, or have a read-only view of a real file system, getting a temp dir might well fail, while we can easily overlay any path we want in memory.

ilya-biryukov added inline comments.Nov 16 2017, 6:06 AM
lib/Frontend/PrecompiledPreamble.cpp
44 ↗(On Diff #122838)

That's true in general, but LLVM's system_temp_directory will return the default (/var/tmp or /tmp on Linux, C:\Temp on Windows) if it fails to find the real temp dir.
So in the very worst case we'll be using hard-coded paths.

On the other hand, if that's one of the possible behaviors, we'd probably want to not break in that case anyway. I'll update the patch to follow your suggestions. Thanks!

  • Use a hard-coded virtual path for in-memory PCHs instead of system_temp_directory.
  • Removed redundant #include.
ilya-biryukov added inline comments.Nov 16 2017, 6:35 AM
lib/Frontend/PrecompiledPreamble.cpp
44 ↗(On Diff #122838)

This should do the trick.

This revision was automatically updated to reflect the committed changes.
mgorny added a subscriber: mgorny.Nov 16 2017, 9:11 AM

Please revert this commit. You've just broken all the stand-alone builds of clang.

cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp
27

This is private LLVM header which is not available when doing stand-alone builds.

Please revert this commit. You've just broken all the stand-alone builds of clang.

Sorry about that. Should be fixed in rL318514