This is an archive of the discontinued LLVM Phabricator instance.

FileManager: Shrink FileEntryRef to the size of a pointer
ClosedPublic

Authored by dexonsmith on Oct 15 2020, 11:25 AM.

Details

Summary

Shrink FileEntryRef to the size of a pointer, by having it directly
reference the StringMapEntry the same way that DirectoryEntryRef
does. This makes FileEntryRef::FileEntryRef private as a side effect
(FileManager is a friend!).

There are two helper types added within FileEntryRef:

  • FileEntryRef::MapValue is the type stored in FileManager::SeenFileEntries. It's a replacement for SeenFileEntryOrRedirect, where the second pointer type has been changed from StringRef* to MapEntry* (see next bullet).
  • FileEntryRef::MapEntry is the instantiation of StringMapEntry<> where MapValue is stored. This is what FileEntryRef has a pointer to, in order to grab the name in addition to the value.

Diff Detail

Event Timeline

dexonsmith created this revision.Oct 15 2020, 11:25 AM
dexonsmith requested review of this revision.Oct 15 2020, 11:25 AM
dexonsmith planned changes to this revision.Oct 15 2020, 12:11 PM

I'd still appreciate feedback on the direction, but clang/test/ClangScanDeps/modules-full.cpp is failing (getting "<built-in>" listed twice under "file-deps" for each translation unit) so I'm marking this as needing changes.

I'd still appreciate feedback on the direction, but clang/test/ClangScanDeps/modules-full.cpp is failing (getting "<built-in>" listed twice under "file-deps" for each translation unit) so I'm marking this as needing changes.

Found the bug; it was pretty subtle. See https://reviews.llvm.org/D89508 for details.

Rebased on top of https://reviews.llvm.org/D89508. Still some things to split apart, but the bug is fixed and this is a fair bit smaller.

Removing some noise from the patch that I missed in the last update.

Peeled off the clangd change into https://reviews.llvm.org/D89514 and rebased.

dexonsmith retitled this revision from WIP: FileManager: Shrink FileEntryRef to the size of a pointer to FileManager: Shrink FileEntryRef to the size of a pointer.

Rebase on top of https://reviews.llvm.org/D89521 to further reduce noise. I think this is ready to go.

dexonsmith edited the summary of this revision. (Show Details)

Improved the naming of the helper types for FileEntryRef and the SeenFileEntries map and removed dead code for the old typedef. Found when working on a follow-up.

arphaman added inline comments.Oct 26 2020, 9:33 AM
clang/lib/Basic/FileManager.cpp
217–219

Can you use isa here instead of dyn_cast?

219

It looks like this accounts for one level of redirections, but the previous implementation could support multiple levels of redirections. Can multiple levels of redirections still be supported here too?

309

Can you rewrite the FIXME above, and also the assignment return makes it less readable. Maybe separate out the return onto the next line?

Adding a test for multi-level indirection.

dexonsmith planned changes to this revision.Oct 26 2020, 2:33 PM
dexonsmith added inline comments.
clang/lib/Basic/FileManager.cpp
219

I augmented the test for getFileRef to check for multi-level redirections. It asserts both before and after this patch. Here is a partial backtrace from before:

Assertion failed: (is<T>() && "Invalid accessor called"), function get, file llvm/include/llvm/ADT/PointerUnion.h, line 188.
[...]
8  BasicTests               0x0000000103fb5ce9 clang::FileEntry* llvm::PointerUnion<clang::FileEntry*, llvm::StringRef const*>::get<clang::FileEntry*>() const + 105
9  BasicTests               0x0000000103fb4f80 clang::FileManager::getFileRef(llvm::StringRef, bool, bool) + 2240
10 BasicTests               0x0000000103d27d57 (anonymous namespace)::FileManagerTest_getFileRefReturnsCorrectNameForDifferentStatPath_Test::TestBody() + 711

Can you confirm this is the right test to add, or did you mean something different?

clang/unittests/Basic/FileManagerTest.cpp
292

@arphaman, this is the line the test asserts on (with or without my patch).

Updated to address remaining review comments.

Note that we discussed double-redirection use case offline, and agreed it doesn't come up in practice (even with multiple RedirectingFileSystem layers) so we shouldn't support it. I added a targeted assertion and added an EXPECT_DEATH with a comment in the test.

dexonsmith marked 3 inline comments as done.Oct 26 2020, 3:46 PM
dexonsmith added inline comments.
clang/lib/Basic/FileManager.cpp
217–219

Yup, done (comment has strangely detached from the code though...)

219

We discussed offline and agreed this double-redirection doesn't happen practice.

309

Thanks, fixed.

arphaman accepted this revision.Oct 26 2020, 5:15 PM
This revision is now accepted and ready to land.Oct 26 2020, 5:15 PM
This revision was automatically updated to reflect the committed changes.
dexonsmith marked 2 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2020, 11:56 AM
thakis added a subscriber: thakis.Oct 27 2020, 4:32 PM
thakis added inline comments.
clang/include/clang/Basic/FileManager.h
118

It looks like this is too clever for gcc5.3: https://bugs.chromium.org/p/chromium/issues/detail?id=1143022

Ideas on how to work around that?

thakis added inline comments.Oct 27 2020, 4:57 PM
clang/include/clang/Basic/FileManager.h
118

Looks like the following might do it. I'll finish building locally and push this if it works:

$ git diff
diff --git a/clang/include/clang/Basic/FileManager.h b/clang/include/clang/Basic/FileManager.h
index d27b4260cca..12848f42770 100644
--- a/clang/include/clang/Basic/FileManager.h
+++ b/clang/include/clang/Basic/FileManager.h
@@ -107,7 +107,9 @@ public:
     /// VFSs that use external names. In that case, the \c FileEntryRef
     /// returned by the \c FileManager will have the external name, and not the
     /// name that was used to lookup the file.
-    llvm::PointerUnion<FileEntry *, const MapEntry *> V;
+    // The second type is really a `const MapEntry *`, but that confuses gcc5.3.
+    // Once that's no longer supported, change this back.
+    llvm::PointerUnion<FileEntry *, const void *> V;
 
     MapValue() = delete;
     MapValue(FileEntry &FE) : V(&FE) {}
diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index d26ead4a5b0..4d84c3102ec 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -215,7 +215,7 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
     FileEntryRef::MapValue Value = *SeenFileInsertResult.first->second;
     if (LLVM_LIKELY(Value.V.is<FileEntry *>()))
       return FileEntryRef(*SeenFileInsertResult.first);
-    return FileEntryRef(*Value.V.get<const FileEntryRef::MapEntry *>());
+    return FileEntryRef(*reinterpret_cast<const FileEntryRef::MapEntry *>(Value.V.get<const void *>()));
   }
 
   // We've not seen this before. Fill it in.
@@ -347,7 +347,7 @@ FileManager::getVirtualFile(StringRef Filename, off_t Size,
     FileEntry *FE;
     if (LLVM_LIKELY(FE = Value.V.dyn_cast<FileEntry *>()))
       return FE;
-    return &FileEntryRef(*Value.V.get<const FileEntryRef::MapEntry *>())
+    return &FileEntryRef(*reinterpret_cast<const FileEntryRef::MapEntry *>(Value.V.get<const void *>()))
                 .getFileEntry();
   }
dexonsmith added inline comments.Oct 27 2020, 5:38 PM
clang/include/clang/Basic/FileManager.h
118

Thanks Nico!