This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Expose RequiresNullTerminator in FileManager.
ClosedPublic

Authored by Bigcheese on Apr 8 2020, 8:37 PM.

Details

Summary

This is needed to fix the reason
0a2be46cfdb698fe (Modules: Invalidate out-of-date PCMs as they're
discovered) and rG5b44a4b07fc1 ([modules] Do not cache invalid state for
modules that we attempted to load.) were reverted.

These patches changed Clang to use isVolatile when loading modules.
This had the side effect of not using mmap when loading modules, and
thus greatly increased memory usage.

The reason it wasn't using mmap is because MemoryBuffer plays some
games with file size when you request null termination, and it has to
disable these when isVolatile is set as the size may change by the
time it's mmapped. Clang by default passes
RequiresNullTerminator = true, and shouldUseMmap ignored if
RequiresNullTerminator was even requested.

This patch adds RequiresNullTerminator to the FileManager interface
so Clang can use it when loading modules, and changes shouldUseMmap to
only take volatility into account if RequiresNullTerminator is true.
This is fine as both mmap and a read loop are vulnerable to
modifying the file while reading, but are immune to the rename Clang
does when replacing a module file.

Diff Detail

Event Timeline

Bigcheese created this revision.Apr 8 2020, 8:37 PM

Code change LGTM.

Is there a way to add a MemoryBuffer unit test for the change to shouldUseMmap?

Not really. It's a static function in MemoryBuffer.cpp, and the MemoryBuffer class doesn't have a Kind member so we can't check for MemoryBufferMMapFile.

Not really. It's a static function in MemoryBuffer.cpp, and the MemoryBuffer class doesn't have a Kind member so we can't check for MemoryBufferMMapFile.

Ah, but there's a vtable :), and it looks like we already have a virtual function called MemoryBuffer::getBufferKind() to expose this.

Maybe use MemoryBuffer::getFile() API to test this?

Bigcheese updated this revision to Diff 256722.Apr 10 2020, 5:48 PM

Added a test.

dexonsmith accepted this revision.Apr 13 2020, 3:14 PM

LGTM.

llvm/unittests/Support/MemoryBufferTest.cpp
396 ↗(On Diff #256722)

Maybe add named arguments in comments for the last three? They're a bit inscrutable, and the whole point is to test IsVolatile.

This revision is now accepted and ready to land.Apr 13 2020, 3:14 PM
This revision was automatically updated to reflect the committed changes.