[PATCH] D151398: [clang] Make `FileEntryRef::getDir()` return the as-requested `DirectoryEntryRef`

[Richard Howell via Phabricator via cfe-commits]

rmaz accepted this revision.
rmaz added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang/include/clang/Basic/FileEntry.h:124
+/// Directory the file was found in.
 OptionalDirectoryEntryRef Dir;
 

If this is always set now, should it be a non optional `DirectoryEntryRef`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151398/new/

https://reviews.llvm.org/D151398

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147561: [clang] don't serialize MODULE_DIRECTORY with ModuleFileHomeIsCwd

[Richard Howell via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8ee5029b225b: [clang] dont serialize MODULE_DIRECTORY 
with ModuleFileHomeIsCwd (authored by rmaz).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147561/new/

https://reviews.llvm.org/D147561

Files:
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/module-file-home-is-cwd.m


Index: clang/test/Modules/module-file-home-is-cwd.m
===
--- clang/test/Modules/module-file-home-is-cwd.m
+++ clang/test/Modules/module-file-home-is-cwd.m
@@ -5,11 +5,12 @@
 // RUN: -fmodules-embed-all-files %S/Inputs/normal-module-map/module.map \
 // RUN: -o %t/mod.pcm
 // RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s 
--check-prefix=INPUT
 
-// CHECK:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}module.map'
-// CHECK:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}a2.h'
-// CHECK:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}a1.h'
 // CHECK-NOT: MODULE_DIRECTORY
+// INPUT:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}module.map'
+// INPUT:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}a2.h'
+// INPUT:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}a1.h'
 
 @import libA;
 
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1288,11 +1288,11 @@
 // If the home of the module is the current working directory, then we
 // want to pick up the cwd of the build process loading the module, not
 // our cwd, when we load this module.
-if (!(PP.getHeaderSearchInfo()
+if (!PP.getHeaderSearchInfo().getHeaderSearchOpts().ModuleFileHomeIsCwd &&
+(!PP.getHeaderSearchInfo()
   .getHeaderSearchOpts()
   .ModuleMapFileHomeIsCwd ||
-  PP.getHeaderSearchInfo().getHeaderSearchOpts().ModuleFileHomeIsCwd) 
||
-WritingModule->Directory->getName() != StringRef(".")) {
+ WritingModule->Directory->getName() != StringRef("."))) {
   // Module directory.
   auto Abbrev = std::make_shared();
   Abbrev->Add(BitCodeAbbrevOp(MODULE_DIRECTORY));


Index: clang/test/Modules/module-file-home-is-cwd.m
===
--- clang/test/Modules/module-file-home-is-cwd.m
+++ clang/test/Modules/module-file-home-is-cwd.m
@@ -5,11 +5,12 @@
 // RUN: -fmodules-embed-all-files %S/Inputs/normal-module-map/module.map \
 // RUN: -o %t/mod.pcm
 // RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s --check-prefix=INPUT
 
-// CHECK:  blob data = 'Inputs{{/|\\}}normal-module-map{{/|\\}}module.map'
-// CHECK:  blob data = 'Inputs{{/|\\}}normal-module-map{{/|\\}}a2.h'
-// CHECK:  blob data = 'Inputs{{/|\\}}normal-module-map{{/|\\}}a1.h'
 // CHECK-NOT: MODULE_DIRECTORY
+// INPUT:  blob data = 'Inputs{{/|\\}}normal-module-map{{/|\\}}module.map'
+// INPUT:  blob data = 'Inputs{{/|\\}}normal-module-map{{/|\\}}a2.h'
+// INPUT:  blob data = 'Inputs{{/|\\}}normal-module-map{{/|\\}}a1.h'
 
 @import libA;
 
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1288,11 +1288,11 @@
 // If the home of the module is the current working directory, then we
 // want to pick up the cwd of the build process loading the module, not
 // our cwd, when we load this module.
-if (!(PP.getHeaderSearchInfo()
+if (!PP.getHeaderSearchInfo().getHeaderSearchOpts().ModuleFileHomeIsCwd &&
+(!PP.getHeaderSearchInfo()
   .getHeaderSearchOpts()
   .ModuleMapFileHomeIsCwd ||
-  PP.getHeaderSearchInfo().getHeaderSearchOpts().ModuleFileHomeIsCwd) ||
-WritingModule->Directory->getName() != StringRef(".")) {
+ WritingModule->Directory->getName() != StringRef("."))) {
   // Module directory.
   auto Abbrev = std::make_shared();
   Abbrev->Add(BitCodeAbbrevOp(MODULE_DIRECTORY));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147561: [clang] don't serialize MODULE_DIRECTORY with ModuleFileHomeIsCwd

[Richard Howell via Phabricator via cfe-commits]

rmaz created this revision.
Herald added a project: All.
rmaz requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fix a bug in the MODULE_DIRECTORY serialization logic
that would cause MODULE_DIRECTORY to be serialized when
`-fmodule-file-home-is-cwd` is specified.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147561

Files:
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/module-file-home-is-cwd.m


Index: clang/test/Modules/module-file-home-is-cwd.m
===
--- clang/test/Modules/module-file-home-is-cwd.m
+++ clang/test/Modules/module-file-home-is-cwd.m
@@ -5,11 +5,12 @@
 // RUN: -fmodules-embed-all-files %S/Inputs/normal-module-map/module.map \
 // RUN: -o %t/mod.pcm
 // RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s 
--check-prefix=INPUT
 
-// CHECK:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}module.map'
-// CHECK:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}a2.h'
-// CHECK:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}a1.h'
 // CHECK-NOT: MODULE_DIRECTORY
+// INPUT:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}module.map'
+// INPUT:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}a2.h'
+// INPUT:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}a1.h'
 
 @import libA;
 
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1288,11 +1288,11 @@
 // If the home of the module is the current working directory, then we
 // want to pick up the cwd of the build process loading the module, not
 // our cwd, when we load this module.
-if (!(PP.getHeaderSearchInfo()
+if (!PP.getHeaderSearchInfo().getHeaderSearchOpts().ModuleFileHomeIsCwd &&
+(!PP.getHeaderSearchInfo()
   .getHeaderSearchOpts()
   .ModuleMapFileHomeIsCwd ||
-  PP.getHeaderSearchInfo().getHeaderSearchOpts().ModuleFileHomeIsCwd) 
||
-WritingModule->Directory->getName() != StringRef(".")) {
+ WritingModule->Directory->getName() != StringRef("."))) {
   // Module directory.
   auto Abbrev = std::make_shared();
   Abbrev->Add(BitCodeAbbrevOp(MODULE_DIRECTORY));


Index: clang/test/Modules/module-file-home-is-cwd.m
===
--- clang/test/Modules/module-file-home-is-cwd.m
+++ clang/test/Modules/module-file-home-is-cwd.m
@@ -5,11 +5,12 @@
 // RUN: -fmodules-embed-all-files %S/Inputs/normal-module-map/module.map \
 // RUN: -o %t/mod.pcm
 // RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s --check-prefix=INPUT
 
-// CHECK:  blob data = 'Inputs{{/|\\}}normal-module-map{{/|\\}}module.map'
-// CHECK:  blob data = 'Inputs{{/|\\}}normal-module-map{{/|\\}}a2.h'
-// CHECK:  blob data = 'Inputs{{/|\\}}normal-module-map{{/|\\}}a1.h'
 // CHECK-NOT: MODULE_DIRECTORY
+// INPUT:  blob data = 'Inputs{{/|\\}}normal-module-map{{/|\\}}module.map'
+// INPUT:  blob data = 'Inputs{{/|\\}}normal-module-map{{/|\\}}a2.h'
+// INPUT:  blob data = 'Inputs{{/|\\}}normal-module-map{{/|\\}}a1.h'
 
 @import libA;
 
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1288,11 +1288,11 @@
 // If the home of the module is the current working directory, then we
 // want to pick up the cwd of the build process loading the module, not
 // our cwd, when we load this module.
-if (!(PP.getHeaderSearchInfo()
+if (!PP.getHeaderSearchInfo().getHeaderSearchOpts().ModuleFileHomeIsCwd &&
+(!PP.getHeaderSearchInfo()
   .getHeaderSearchOpts()
   .ModuleMapFileHomeIsCwd ||
-  PP.getHeaderSearchInfo().getHeaderSearchOpts().ModuleFileHomeIsCwd) ||
-WritingModule->Directory->getName() != StringRef(".")) {
+ WritingModule->Directory->getName() != StringRef("."))) {
   // Module directory.
   auto Abbrev = std::make_shared();
   Abbrev->Add(BitCodeAbbrevOp(MODULE_DIRECTORY));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143414: [clang] refactor FileManager::GetUniqueIDMapping

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 497309.
rmaz added a comment.

remove `header_file_size()`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143414/new/

https://reviews.llvm.org/D143414

Files:
  clang/include/clang/Basic/FileManager.h
  clang/include/clang/Lex/HeaderSearch.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/Serialization/ASTWriter.cpp

Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -169,17 +169,10 @@
 
   const HeaderSearch  = PP.getHeaderSearchInfo();
 
-  SmallVector FilesByUID;
-  HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
-
-  if (FilesByUID.size() > HS.header_file_size())
-FilesByUID.resize(HS.header_file_size());
-
-  for (unsigned UID = 0, LastUID = FilesByUID.size(); UID != LastUID; ++UID) {
-const FileEntry *File = FilesByUID[UID];
-if (!File)
-  continue;
+  SmallVector FileEntries;
+  HS.getFileMgr().getSeenNonVirtualFileEntries(FileEntries);
 
+  for (auto File : FileEntries) {
 const HeaderFileInfo *HFI =
 HS.getExistingFileInfo(File, /*WantExternal*/ false);
 if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
@@ -1918,16 +1911,10 @@
 }
   }
 
-  SmallVector FilesByUID;
-  HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
+  SmallVector FileEntries;
+  HS.getFileMgr().getSeenNonVirtualFileEntries(FileEntries);
 
-  if (FilesByUID.size() > HS.header_file_size())
-FilesByUID.resize(HS.header_file_size());
-
-  for (unsigned UID = 0, LastUID = FilesByUID.size(); UID != LastUID; ++UID) {
-const FileEntry *File = FilesByUID[UID];
-if (!File)
-  continue;
+  for (auto File : FileEntries) {
 
 // Get the file info. This will load info from the external source if
 // necessary. Skip emitting this file if we have no information on it
@@ -1941,7 +1928,7 @@
   continue;
 
 // Massage the file path into an appropriate form.
-StringRef Filename = File->getName();
+StringRef Filename = File.getName();
 SmallString<128> FilenameTmp(Filename);
 if (PreparePathForOutput(FilenameTmp)) {
   // If we performed any translation on the file name at all, we need to
@@ -1950,9 +1937,8 @@
   SavedStrings.push_back(Filename.data());
 }
 
-HeaderFileInfoTrait::key_type Key = {
-  Filename, File->getSize(), getTimestampForOutput(File)
-};
+HeaderFileInfoTrait::key_type Key = {Filename, File.getSize(),
+ getTimestampForOutput(File)};
 HeaderFileInfoTrait::data_type Data = {
   *HFI, HS.getModuleMap().findResolvedModulesForHeader(File), {}
 };
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -611,25 +611,26 @@
   return std::error_code();
 }
 
-void FileManager::GetUniqueIDMapping(
-SmallVectorImpl ) const {
-  UIDToFiles.clear();
-  UIDToFiles.resize(NextFileUID);
-
-  // Map file entries
-  for (llvm::StringMap,
-   llvm::BumpPtrAllocator>::const_iterator
-   FE = SeenFileEntries.begin(),
-   FEEnd = SeenFileEntries.end();
-   FE != FEEnd; ++FE)
-if (llvm::ErrorOr Entry = FE->getValue()) {
-  if (const auto *FE = Entry->V.dyn_cast())
-UIDToFiles[FE->getUID()] = FE;
-}
+void FileManager::getSeenNonVirtualFileEntries(
+SmallVectorImpl ) const {
+  Entries.reserve(SeenFileEntries.size());
+
+  for (const auto  : SeenFileEntries) {
+if (auto Value = Entry.getValue())
+  // Ignore VFS-mapped entries, they will have a separate on-disk
+  // file entry.
+  if (Value->V.is())
+Entries.push_back(FileEntryRef(Entry));
+  }
+
+  // We want the file entries in deterministic order for
+  // module serialization.
+  llvm::sort(Entries, [](FileEntryRef , FileEntryRef ) {
+if (A.getUID() != B.getUID())
+  return A.getUID() < B.getUID();
 
-  // Map virtual file entries
-  for (const auto  : VirtualFileEntries)
-UIDToFiles[VFE->getUID()] = VFE;
+return A.getName() < B.getName();
+  });
 }
 
 StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) {
Index: clang/include/clang/Lex/HeaderSearch.h
===
--- clang/include/clang/Lex/HeaderSearch.h
+++ clang/include/clang/Lex/HeaderSearch.h
@@ -793,8 +793,6 @@
   /// Retrieve the module map.
   const ModuleMap () const { return ModMap; }
 
-  unsigned header_file_size() const { return FileInfo.size(); }
-
   /// Return the HeaderFileInfo structure for the specified FileEntry,
   /// in preparation for updating it in some way.
   HeaderFileInfo (const FileEntry *FE);
Index: clang/include/clang/Basic/FileManager.h
===
--- 

[PATCH] D143414: [clang] refactor FileManager::GetUniqueIDMapping

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D143414#4110461 , @benlangmuir 
wrote:

>> This should allow the path serialization of input files to use the paths 
>> used when looking up a file entry, instead of the last reference.
>
> Isn't this at odds with not having the VFS-mapped paths?
>
> It's not obvious to me why we want these specific semantics.  Elsewhere we 
> have tried to preserve the virtual paths as well as the vfsoverlay files 
> needed to interpret them.  Is there a reason the current approach is better? 
> I feel like there may be context here I'm lacking

My understanding for why we were dropping the virtual paths is that we would 
already have the on-disk path entries anyway, but @jansvoboda11 may know more.

The reason for the change from FileEntry to FileEntryRef is so we can produce 
deterministic pcm output regardless of if the files are regular files or hard 
links pointing to the same inode. Currently this can result in varying paths 
and numbers of inputs being serialized.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143414/new/

https://reviews.llvm.org/D143414

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143414: [clang] refactor FileManager::GetUniqueIDMapping

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 495512.
rmaz edited the summary of this revision.
rmaz added a comment.

Use FileEntryRef, rename method


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143414/new/

https://reviews.llvm.org/D143414

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/Serialization/ASTWriter.cpp

Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -169,17 +169,10 @@
 
   const HeaderSearch  = PP.getHeaderSearchInfo();
 
-  SmallVector FilesByUID;
-  HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
-
-  if (FilesByUID.size() > HS.header_file_size())
-FilesByUID.resize(HS.header_file_size());
-
-  for (unsigned UID = 0, LastUID = FilesByUID.size(); UID != LastUID; ++UID) {
-const FileEntry *File = FilesByUID[UID];
-if (!File)
-  continue;
+  SmallVector FileEntries;
+  HS.getFileMgr().getSeenNonVirtualFileEntries(FileEntries);
 
+  for (auto File : FileEntries) {
 const HeaderFileInfo *HFI =
 HS.getExistingFileInfo(File, /*WantExternal*/ false);
 if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
@@ -1918,16 +1911,10 @@
 }
   }
 
-  SmallVector FilesByUID;
-  HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
+  SmallVector FileEntries;
+  HS.getFileMgr().getSeenNonVirtualFileEntries(FileEntries);
 
-  if (FilesByUID.size() > HS.header_file_size())
-FilesByUID.resize(HS.header_file_size());
-
-  for (unsigned UID = 0, LastUID = FilesByUID.size(); UID != LastUID; ++UID) {
-const FileEntry *File = FilesByUID[UID];
-if (!File)
-  continue;
+  for (auto File : FileEntries) {
 
 // Get the file info. This will load info from the external source if
 // necessary. Skip emitting this file if we have no information on it
@@ -1941,7 +1928,7 @@
   continue;
 
 // Massage the file path into an appropriate form.
-StringRef Filename = File->getName();
+StringRef Filename = File.getName();
 SmallString<128> FilenameTmp(Filename);
 if (PreparePathForOutput(FilenameTmp)) {
   // If we performed any translation on the file name at all, we need to
@@ -1950,9 +1937,8 @@
   SavedStrings.push_back(Filename.data());
 }
 
-HeaderFileInfoTrait::key_type Key = {
-  Filename, File->getSize(), getTimestampForOutput(File)
-};
+HeaderFileInfoTrait::key_type Key = {Filename, File.getSize(),
+ getTimestampForOutput(File)};
 HeaderFileInfoTrait::data_type Data = {
   *HFI, HS.getModuleMap().findResolvedModulesForHeader(File), {}
 };
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -611,25 +611,26 @@
   return std::error_code();
 }
 
-void FileManager::GetUniqueIDMapping(
-SmallVectorImpl ) const {
-  UIDToFiles.clear();
-  UIDToFiles.resize(NextFileUID);
-
-  // Map file entries
-  for (llvm::StringMap,
-   llvm::BumpPtrAllocator>::const_iterator
-   FE = SeenFileEntries.begin(),
-   FEEnd = SeenFileEntries.end();
-   FE != FEEnd; ++FE)
-if (llvm::ErrorOr Entry = FE->getValue()) {
-  if (const auto *FE = Entry->V.dyn_cast())
-UIDToFiles[FE->getUID()] = FE;
-}
+void FileManager::getSeenNonVirtualFileEntries(
+SmallVectorImpl ) const {
+  Entries.reserve(SeenFileEntries.size());
+
+  for (const auto  : SeenFileEntries) {
+if (auto Value = Entry.getValue())
+  // Ignore VFS-mapped entries, they will have a separate on-disk
+  // file entry.
+  if (Value->V.is())
+Entries.push_back(FileEntryRef(Entry));
+  }
+
+  // We want the file entries in deterministic order for
+  // module serialization.
+  llvm::sort(Entries, [](FileEntryRef , FileEntryRef ) {
+if (A.getUID() != B.getUID())
+  return A.getUID() < B.getUID();
 
-  // Map virtual file entries
-  for (const auto  : VirtualFileEntries)
-UIDToFiles[VFE->getUID()] = VFE;
+return A.getName() < B.getName();
+  });
 }
 
 StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) {
Index: clang/include/clang/Basic/FileManager.h
===
--- clang/include/clang/Basic/FileManager.h
+++ clang/include/clang/Basic/FileManager.h
@@ -310,10 +310,10 @@
   /// \returns true if \c Path changed to absolute.
   bool makeAbsolutePath(SmallVectorImpl ) const;
 
-  /// Produce an array mapping from the unique IDs assigned to each
-  /// file to the corresponding FileEntry pointer.
-  void GetUniqueIDMapping(
-SmallVectorImpl ) const;
+  /// Produce an array of seen FileEntryRef. The entries will be sorted
+  /// by UID and Name. Virtual file entries will not be 

[PATCH] D143414: [clang] refactor FileManager::GetUniqueIDMapping

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 495259.
rmaz added a comment.

Don't return virtual file entries


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143414/new/

https://reviews.llvm.org/D143414

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/Serialization/ASTWriter.cpp

Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -36,6 +36,7 @@
 #include "clang/AST/TypeLocVisitor.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileEntry.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
 #include "clang/Basic/IdentifierTable.h"
@@ -169,17 +170,10 @@
 
   const HeaderSearch  = PP.getHeaderSearchInfo();
 
-  SmallVector FilesByUID;
-  HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
-
-  if (FilesByUID.size() > HS.header_file_size())
-FilesByUID.resize(HS.header_file_size());
-
-  for (unsigned UID = 0, LastUID = FilesByUID.size(); UID != LastUID; ++UID) {
-const FileEntry *File = FilesByUID[UID];
-if (!File)
-  continue;
+  SmallVector FileEntries;
+  HS.getFileMgr().getSeenFileEntries(FileEntries);
 
+  for (auto File : FileEntries) {
 const HeaderFileInfo *HFI =
 HS.getExistingFileInfo(File, /*WantExternal*/ false);
 if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
@@ -1918,16 +1912,10 @@
 }
   }
 
-  SmallVector FilesByUID;
-  HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
+  SmallVector FileEntries;
+  HS.getFileMgr().getSeenFileEntries(FileEntries);
 
-  if (FilesByUID.size() > HS.header_file_size())
-FilesByUID.resize(HS.header_file_size());
-
-  for (unsigned UID = 0, LastUID = FilesByUID.size(); UID != LastUID; ++UID) {
-const FileEntry *File = FilesByUID[UID];
-if (!File)
-  continue;
+  for (auto File : FileEntries) {
 
 // Get the file info. This will load info from the external source if
 // necessary. Skip emitting this file if we have no information on it
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -17,6 +17,7 @@
 //===--===//
 
 #include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileEntry.h"
 #include "clang/Basic/FileSystemStatCache.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
@@ -611,25 +612,28 @@
   return std::error_code();
 }
 
-void FileManager::GetUniqueIDMapping(
-SmallVectorImpl ) const {
-  UIDToFiles.clear();
-  UIDToFiles.resize(NextFileUID);
-
-  // Map file entries
-  for (llvm::StringMap,
-   llvm::BumpPtrAllocator>::const_iterator
-   FE = SeenFileEntries.begin(),
-   FEEnd = SeenFileEntries.end();
-   FE != FEEnd; ++FE)
-if (llvm::ErrorOr Entry = FE->getValue()) {
-  if (const auto *FE = Entry->V.dyn_cast())
-UIDToFiles[FE->getUID()] = FE;
-}
+void FileManager::getSeenFileEntries(
+SmallVectorImpl )
+const {
+  Entries.reserve(SeenFileEntries.size());
+
+  for (const auto  : SeenFileEntries) {
+if (auto Value = Entry.getValue())
+  // Ignore VFS-mapped entries, they will have a separate on-disk
+  // file entry.
+  if (Value->V.dyn_cast())
+Entries.push_back(FileEntryRef(Entry));
+  }
+
+  // We want the file entries in deterministic order for
+  // module serialization.
+  llvm::sort(Entries, [](OptionalFileEntryRefDegradesToFileEntryPtr ,
+ OptionalFileEntryRefDegradesToFileEntryPtr ) {
+if (A->getUID() != B->getUID())
+  return A->getUID() < B->getUID();
 
-  // Map virtual file entries
-  for (const auto  : VirtualFileEntries)
-UIDToFiles[VFE->getUID()] = VFE;
+return A->getName() < B->getName();
+  });
 }
 
 StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) {
Index: clang/include/clang/Basic/FileManager.h
===
--- clang/include/clang/Basic/FileManager.h
+++ clang/include/clang/Basic/FileManager.h
@@ -310,10 +310,11 @@
   /// \returns true if \c Path changed to absolute.
   bool makeAbsolutePath(SmallVectorImpl ) const;
 
-  /// Produce an array mapping from the unique IDs assigned to each
-  /// file to the corresponding FileEntry pointer.
-  void GetUniqueIDMapping(
-SmallVectorImpl ) const;
+  /// Produce an array of seen FileEntryRef. The entries will be sorted
+  /// by UID and Name.
+  void getSeenFileEntries(
+  SmallVectorImpl )
+  const;
 
   /// Retrieve the canonical name for a given directory.
   ///
___
cfe-commits mailing list

[PATCH] D143414: [clang] refactor FileManager::GetUniqueIDMapping

[Richard Howell via Phabricator via cfe-commits]

rmaz added inline comments.



Comment at: clang/include/clang/Basic/FileManager.h:316
+  void getSeenFileEntries(
+  SmallVectorImpl )
+  const;

jansvoboda11 wrote:
> Since we're already modifying the two only users of this function, maybe we 
> could use `Optional` instead of 
> `OptionalFileEntryRefDegradesToFileEntryPtr` (which we want to eventually 
> remove)?
We could do that with less churn once more or the called methods in the loop 
over the files are refactored in D142724, but I'm fine with either approach.



Comment at: clang/lib/Basic/FileManager.cpp:625-627
+Entries.push_back(
+FileEntryRef(*reinterpret_cast(
+Value->V.get(;

jansvoboda11 wrote:
> Why is this necessary? IIUC, `FileEntryRef` has this logic in 
> `getBaseMapEntry()`. I would expect `FileEntryRef(Entry)` to be correct 
> regardless of what's in `Value->V`.
I borrowed this from: 
https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/FileManager.cpp#L399-L408

I assumed this was necessary from @benlangmuir 's comment on D142780:

> Also, it will never give you a vfs mapped path since it's skipping those 
> (V.dyn_cast()).

Are you saying we should just do something like:
```
for (const auto  : SeenFileEntries) {
if (llvm::ErrorOr Value = Entry.getValue())
Entries.push_back(FileEntryRef(Entry));
}
```
?



Comment at: clang/lib/Serialization/ASTWriter.cpp:176
 
+  for (auto  : FileEntries) {
 const HeaderFileInfo *HFI =

jansvoboda11 wrote:
> I'm afraid that iterating over file entries in "non-deterministic" order can 
> cause non-determinism down the line. For example, calling 
> `HeaderSearch::findAllModulesForHeader()` in the loop can trigger 
> deserialization of `HeaderFileInfo` from loaded PCMs. That code fills the 
> `Module::Headers` vectors, which we serialize without sorting first.
Do you think we should move the sorting into the `getSeenFileEntries()` method?



Comment at: clang/lib/Serialization/ASTWriter.cpp:1939
 // Massage the file path into an appropriate form.
 StringRef Filename = File->getName();
 SmallString<128> FilenameTmp(Filename);

jansvoboda11 wrote:
> This will insert duplicate entries (with the same key) into the on-disk hash 
> table. I don't know if that's problematic, just thought I'd call it out.
Not sure I follow, why would we have `FileEntry`s with duplicate names?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143414/new/

https://reviews.llvm.org/D143414

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143414: [clang] refactor FileManager::GetUniqueIDMapping

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 495191.
rmaz added a comment.

Sort by UIDs, then Name


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143414/new/

https://reviews.llvm.org/D143414

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/Serialization/ASTWriter.cpp

Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -36,6 +36,7 @@
 #include "clang/AST/TypeLocVisitor.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileEntry.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
 #include "clang/Basic/IdentifierTable.h"
@@ -169,17 +170,10 @@
 
   const HeaderSearch  = PP.getHeaderSearchInfo();
 
-  SmallVector FilesByUID;
-  HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
-
-  if (FilesByUID.size() > HS.header_file_size())
-FilesByUID.resize(HS.header_file_size());
-
-  for (unsigned UID = 0, LastUID = FilesByUID.size(); UID != LastUID; ++UID) {
-const FileEntry *File = FilesByUID[UID];
-if (!File)
-  continue;
+  SmallVector FileEntries;
+  HS.getFileMgr().getSeenFileEntries(FileEntries);
 
+  for (auto  : FileEntries) {
 const HeaderFileInfo *HFI =
 HS.getExistingFileInfo(File, /*WantExternal*/ false);
 if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
@@ -1918,16 +1912,17 @@
 }
   }
 
-  SmallVector FilesByUID;
-  HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
+  SmallVector FileEntries;
+  HS.getFileMgr().getSeenFileEntries(FileEntries);
+  llvm::sort(FileEntries, [](OptionalFileEntryRefDegradesToFileEntryPtr ,
+ OptionalFileEntryRefDegradesToFileEntryPtr ) {
+if (A->getUID() == B->getUID())
+  return A->getName() < B->getName();
 
-  if (FilesByUID.size() > HS.header_file_size())
-FilesByUID.resize(HS.header_file_size());
+return A->getUID() < B->getUID();
+  });
 
-  for (unsigned UID = 0, LastUID = FilesByUID.size(); UID != LastUID; ++UID) {
-const FileEntry *File = FilesByUID[UID];
-if (!File)
-  continue;
+  for (auto  : FileEntries) {
 
 // Get the file info. This will load info from the external source if
 // necessary. Skip emitting this file if we have no information on it
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -17,6 +17,7 @@
 //===--===//
 
 #include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileEntry.h"
 #include "clang/Basic/FileSystemStatCache.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
@@ -611,25 +612,22 @@
   return std::error_code();
 }
 
-void FileManager::GetUniqueIDMapping(
-SmallVectorImpl ) const {
-  UIDToFiles.clear();
-  UIDToFiles.resize(NextFileUID);
-
-  // Map file entries
-  for (llvm::StringMap,
-   llvm::BumpPtrAllocator>::const_iterator
-   FE = SeenFileEntries.begin(),
-   FEEnd = SeenFileEntries.end();
-   FE != FEEnd; ++FE)
-if (llvm::ErrorOr Entry = FE->getValue()) {
-  if (const auto *FE = Entry->V.dyn_cast())
-UIDToFiles[FE->getUID()] = FE;
+void FileManager::getSeenFileEntries(
+SmallVectorImpl )
+const {
+  Entries.reserve(SeenFileEntries.size());
+
+  for (const auto  : SeenFileEntries) {
+if (llvm::ErrorOr Value = Entry.getValue()) {
+  if (const auto *FE = Value->V.dyn_cast()) {
+Entries.push_back(FileEntryRef(Entry));
+  } else {
+Entries.push_back(
+FileEntryRef(*reinterpret_cast(
+Value->V.get(;
+  }
 }
-
-  // Map virtual file entries
-  for (const auto  : VirtualFileEntries)
-UIDToFiles[VFE->getUID()] = VFE;
+  }
 }
 
 StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) {
Index: clang/include/clang/Basic/FileManager.h
===
--- clang/include/clang/Basic/FileManager.h
+++ clang/include/clang/Basic/FileManager.h
@@ -312,8 +312,9 @@
 
   /// Produce an array mapping from the unique IDs assigned to each
   /// file to the corresponding FileEntry pointer.
-  void GetUniqueIDMapping(
-SmallVectorImpl ) const;
+  void getSeenFileEntries(
+  SmallVectorImpl )
+  const;
 
   /// Retrieve the canonical name for a given directory.
   ///
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142780: [clang] NFCI: Use FileEntryRef in FileManager's UID mapping

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D142780#4087273 , @jansvoboda11 
wrote:

> Sounds like the easies way out is serializing all `FileEntryRef` objects we 
> know in deterministic order.

How about something like https://reviews.llvm.org/D143414 ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142780/new/

https://reviews.llvm.org/D142780

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143414: [clang] refactor FileManager::GetUniqueIDMapping

[Richard Howell via Phabricator via cfe-commits]

rmaz created this revision.
Herald added a subscriber: mgrang.
Herald added a project: All.
rmaz requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Refactor `FileManager::GetUniqueIDMapping` to populate an array of 
`OptionalFileEntryRefDegradesToFileEntryPtr` instead of UID to 
`FileEntry *`. This should allow the path serialization of input files
to use the paths used when looking up a file entry, instead of the 
last reference.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143414

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/Serialization/ASTWriter.cpp

Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -36,6 +36,7 @@
 #include "clang/AST/TypeLocVisitor.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileEntry.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
 #include "clang/Basic/IdentifierTable.h"
@@ -169,17 +170,10 @@
 
   const HeaderSearch  = PP.getHeaderSearchInfo();
 
-  SmallVector FilesByUID;
-  HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
-
-  if (FilesByUID.size() > HS.header_file_size())
-FilesByUID.resize(HS.header_file_size());
-
-  for (unsigned UID = 0, LastUID = FilesByUID.size(); UID != LastUID; ++UID) {
-const FileEntry *File = FilesByUID[UID];
-if (!File)
-  continue;
+  SmallVector FileEntries;
+  HS.getFileMgr().getSeenFileEntries(FileEntries);
 
+  for (auto File : FileEntries) {
 const HeaderFileInfo *HFI =
 HS.getExistingFileInfo(File, /*WantExternal*/ false);
 if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
@@ -1918,16 +1912,14 @@
 }
   }
 
-  SmallVector FilesByUID;
-  HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
-
-  if (FilesByUID.size() > HS.header_file_size())
-FilesByUID.resize(HS.header_file_size());
+  SmallVector FileEntries;
+  HS.getFileMgr().getSeenFileEntries(FileEntries);
+  llvm::sort(FileEntries, [](OptionalFileEntryRefDegradesToFileEntryPtr ,
+ OptionalFileEntryRefDegradesToFileEntryPtr ) {
+return A->getName() < B->getName();
+  });
 
-  for (unsigned UID = 0, LastUID = FilesByUID.size(); UID != LastUID; ++UID) {
-const FileEntry *File = FilesByUID[UID];
-if (!File)
-  continue;
+  for (auto File : FileEntries) {
 
 // Get the file info. This will load info from the external source if
 // necessary. Skip emitting this file if we have no information on it
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -17,6 +17,7 @@
 //===--===//
 
 #include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileEntry.h"
 #include "clang/Basic/FileSystemStatCache.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
@@ -611,25 +612,22 @@
   return std::error_code();
 }
 
-void FileManager::GetUniqueIDMapping(
-SmallVectorImpl ) const {
-  UIDToFiles.clear();
-  UIDToFiles.resize(NextFileUID);
-
-  // Map file entries
-  for (llvm::StringMap,
-   llvm::BumpPtrAllocator>::const_iterator
-   FE = SeenFileEntries.begin(),
-   FEEnd = SeenFileEntries.end();
-   FE != FEEnd; ++FE)
-if (llvm::ErrorOr Entry = FE->getValue()) {
-  if (const auto *FE = Entry->V.dyn_cast())
-UIDToFiles[FE->getUID()] = FE;
+void FileManager::getSeenFileEntries(
+SmallVectorImpl )
+const {
+  Entries.reserve(SeenFileEntries.size());
+
+  for (const auto  : SeenFileEntries) {
+if (llvm::ErrorOr Value = Entry.getValue()) {
+  if (const auto *FE = Value->V.dyn_cast()) {
+Entries.push_back(FileEntryRef(Entry));
+  } else {
+Entries.push_back(
+FileEntryRef(*reinterpret_cast(
+Value->V.get(;
+  }
 }
-
-  // Map virtual file entries
-  for (const auto  : VirtualFileEntries)
-UIDToFiles[VFE->getUID()] = VFE;
+  }
 }
 
 StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) {
Index: clang/include/clang/Basic/FileManager.h
===
--- clang/include/clang/Basic/FileManager.h
+++ clang/include/clang/Basic/FileManager.h
@@ -312,8 +312,9 @@
 
   /// Produce an array mapping from the unique IDs assigned to each
   /// file to the corresponding FileEntry pointer.
-  void GetUniqueIDMapping(
-SmallVectorImpl ) const;
+  void getSeenFileEntries(
+  SmallVectorImpl )
+  const;
 
   /// Retrieve the canonical name for a given directory.
   ///
___

[PATCH] D142780: [clang] NFCI: Use FileEntryRef in FileManager's UID mapping

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D142780#4087270 , @benlangmuir 
wrote:

> If compiling a single pcm accesses multiple hard links with the same UID, 
> then it would not be possible to use the set of UIDs to get the "right path". 
>  At best we could make it get a deterministic path -- e.g. if we tracked the 
> order of access.

With you so far. Is there a reason we need to use the UIDs in this case? Would 
it be possible to refactor `GetUniqueIDMapping` to instead populate an array 
with all the `FileEntryRef`s that had been seen and serialize that instead?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142780/new/

https://reviews.llvm.org/D142780

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142780: [clang] NFCI: Use FileEntryRef in FileManager's UID mapping

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D142780#4087236 , @benlangmuir 
wrote:

> I think if we want to change this to FileEntryRef it needs to be 
> deterministic which ref you get.

I think this might be the root of the problem we are seeing: depending on build 
configuration sometimes our build inputs are hard links that in the case of 
identical inputs point to the same inode. In that case we are seeing 
non-deterministic header paths serialized in pcm files. IIUC the header files 
are serialized based in their unique ID, so it wouldn't be possible to handle 
this case, is this right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142780/new/

https://reviews.llvm.org/D142780

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142724: [clang] use FileEntryRef for SUBMODULE_TOPHEADER

[Richard Howell via Phabricator via cfe-commits]

rmaz added inline comments.



Comment at: clang/lib/Lex/ModuleMap.cpp:663
   Result->IsInferred = true;
-  Result->addTopHeader(File);
+  Result->addTopHeader(File->getLastRef());
 

jansvoboda11 wrote:
> rmaz wrote:
> > jansvoboda11 wrote:
> > > How much work would be to refactor `File` to be `FileEntryRef`? Would be 
> > > good if we didn't need to resort to `getLastRef()`.
> > It is unfortunately a fair bit. I spent some time on it but hit a roadblock 
> > with the FileManagers VirtualFileEntries:
> > 
> > https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/FileManager.h#L70-L71
> > 
> > We would require this to be refactored to FileEntry too, as it is used here 
> > to provide a UID mapping to FileEntry pointers which needs to be changed to 
> > refs:
> > 
> > https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/FileManager.cpp#L614-L633
> > 
> > I can put up a follow on diff once this one is shipped if preferred?
> I was able to refactor the UID mapping to use `FileEntryRef` in D142780. 
> Given that, I think it would be preferable to propagate these into 
> `findOrCreateModuleForHeaderInUmbrellaDir()` and remove `getLastRef()`.
Nice, i'll rebase this and add the remaining changes once its shipped.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142724/new/

https://reviews.llvm.org/D142724

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142724: [clang] use FileEntryRef for SUBMODULE_TOPHEADER

[Richard Howell via Phabricator via cfe-commits]

rmaz marked an inline comment as done.
rmaz added inline comments.



Comment at: clang/lib/Lex/ModuleMap.cpp:663
   Result->IsInferred = true;
-  Result->addTopHeader(File);
+  Result->addTopHeader(File->getLastRef());
 

jansvoboda11 wrote:
> How much work would be to refactor `File` to be `FileEntryRef`? Would be good 
> if we didn't need to resort to `getLastRef()`.
It is unfortunately a fair bit. I spent some time on it but hit a roadblock 
with the FileManagers VirtualFileEntries:

https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/FileManager.h#L70-L71

We would require this to be refactored to FileEntry too, as it is used here to 
provide a UID mapping to FileEntry pointers which needs to be changed to refs:

https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/FileManager.cpp#L614-L633

I can put up a follow on diff once this one is shipped if preferred?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142724/new/

https://reviews.llvm.org/D142724

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142724: [clang] use FileEntryRef for SUBMODULE_TOPHEADER

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 492829.
rmaz added a comment.

Use FileEntryRef for AddTopHeader()


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142724/new/

https://reviews.llvm.org/D142724

Files:
  clang/include/clang/Basic/Module.h
  clang/lib/Basic/Module.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/tools/libclang/CIndex.cpp

Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -8788,7 +8788,7 @@
 return 0;
   Module *Mod = static_cast(CXMod);
   FileManager  = cxtu::getASTUnit(TU)->getFileManager();
-  ArrayRef TopHeaders = Mod->getTopHeaders(FileMgr);
+  auto TopHeaders = Mod->getTopHeaders(FileMgr);
   return TopHeaders.size();
 }
 
@@ -8803,9 +8803,9 @@
   Module *Mod = static_cast(CXMod);
   FileManager  = cxtu::getASTUnit(TU)->getFileManager();
 
-  ArrayRef TopHeaders = Mod->getTopHeaders(FileMgr);
+  auto TopHeaders = Mod->getTopHeaders(FileMgr);
   if (Index < TopHeaders.size())
-return const_cast(TopHeaders[Index]);
+return const_cast([Index].getFileEntry());
 
   return nullptr;
 }
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2878,8 +2878,8 @@
 {
   auto TopHeaders = Mod->getTopHeaders(PP->getFileManager());
   RecordData::value_type Record[] = {SUBMODULE_TOPHEADER};
-  for (auto *H : TopHeaders) {
-SmallString<128> HeaderName(H->getName());
+  for (auto H : TopHeaders) {
+SmallString<128> HeaderName(H.getName());
 PreparePathForOutput(HeaderName);
 Stream.EmitRecordWithBlob(TopHeaderAbbrev, Record, HeaderName);
   }
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -660,7 +660,7 @@
   Explicit).first;
   InferredModuleAllowedBy[Result] = UmbrellaModuleMap;
   Result->IsInferred = true;
-  Result->addTopHeader(File);
+  Result->addTopHeader(File->getLastRef());
 
   // If inferred submodules export everything they import, add a
   // wildcard to the set of exports.
Index: clang/lib/Frontend/FrontendAction.cpp
===
--- clang/lib/Frontend/FrontendAction.cpp
+++ clang/lib/Frontend/FrontendAction.cpp
@@ -353,7 +353,7 @@
   // Add includes for each of these headers.
   for (auto HK : {Module::HK_Normal, Module::HK_Private}) {
 for (Module::Header  : Module->Headers[HK]) {
-  Module->addTopHeader(H.Entry);
+  Module->addTopHeader(*H.Entry);
   // Use the path as specified in the module map file. We'll look for this
   // file relative to the module build directory (the directory containing
   // the module map file) so this will find the same file that we found
@@ -365,7 +365,7 @@
   // Note that Module->PrivateHeaders will not be a TopHeader.
 
   if (Module::Header UmbrellaHeader = Module->getUmbrellaHeader()) {
-Module->addTopHeader(UmbrellaHeader.Entry);
+Module->addTopHeader(*UmbrellaHeader.Entry);
 if (Module->Parent)
   // Include the umbrella header for submodules.
   addHeaderInclude(UmbrellaHeader.PathRelativeToRootModuleDirectory,
@@ -423,7 +423,7 @@
 llvm::sort(Headers, llvm::less_first());
 for (auto  : Headers) {
   // Include this header as part of the umbrella directory.
-  Module->addTopHeader(H.second);
+  Module->addTopHeader(*H.second);
   addHeaderInclude(H.first, Includes, LangOpts, Module->IsExternC);
 }
   }
Index: clang/lib/Basic/Module.cpp
===
--- clang/lib/Basic/Module.cpp
+++ clang/lib/Basic/Module.cpp
@@ -269,16 +269,13 @@
   Umbrella.dyn_cast()};
 }
 
-void Module::addTopHeader(const FileEntry *File) {
-  assert(File);
-  TopHeaders.insert(File);
-}
+void Module::addTopHeader(FileEntryRef File) { TopHeaders.insert(File); }
 
-ArrayRef Module::getTopHeaders(FileManager ) {
+ArrayRef Module::getTopHeaders(FileManager ) {
   if (!TopHeaderNames.empty()) {
 for (std::vector::iterator
I = TopHeaderNames.begin(), E = TopHeaderNames.end(); I != E; ++I) {
-  if (auto FE = FileMgr.getFile(*I))
+  if (auto FE = FileMgr.getFileRef(*I))
 TopHeaders.insert(*FE);
 }
 TopHeaderNames.clear();
Index: clang/include/clang/Basic/Module.h
===
--- clang/include/clang/Basic/Module.h
+++ clang/include/clang/Basic/Module.h
@@ -189,7 +189,7 @@
   OptionalFileEntryRef ASTFile;
 
   /// The top-level headers associated with this module.
-  

[PATCH] D142724: [clang] use FileEntryRef for SUBMODULE_TOPHEADER

[Richard Howell via Phabricator via cfe-commits]

rmaz created this revision.
Herald added a subscriber: arphaman.
Herald added a project: All.
rmaz requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Refactor a Module's TopHeaders to use FileEntryRef. This will keep
the paths serialized in a module the same as the ones used to look
up the header initially.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142724

Files:
  clang/include/clang/Basic/Module.h
  clang/lib/Basic/Module.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/tools/libclang/CIndex.cpp

Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -8788,7 +8788,7 @@
 return 0;
   Module *Mod = static_cast(CXMod);
   FileManager  = cxtu::getASTUnit(TU)->getFileManager();
-  ArrayRef TopHeaders = Mod->getTopHeaders(FileMgr);
+  auto TopHeaders = Mod->getTopHeaders(FileMgr);
   return TopHeaders.size();
 }
 
@@ -8803,9 +8803,9 @@
   Module *Mod = static_cast(CXMod);
   FileManager  = cxtu::getASTUnit(TU)->getFileManager();
 
-  ArrayRef TopHeaders = Mod->getTopHeaders(FileMgr);
+  auto TopHeaders = Mod->getTopHeaders(FileMgr);
   if (Index < TopHeaders.size())
-return const_cast(TopHeaders[Index]);
+return const_cast([Index].getFileEntry());
 
   return nullptr;
 }
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2878,8 +2878,8 @@
 {
   auto TopHeaders = Mod->getTopHeaders(PP->getFileManager());
   RecordData::value_type Record[] = {SUBMODULE_TOPHEADER};
-  for (auto *H : TopHeaders) {
-SmallString<128> HeaderName(H->getName());
+  for (auto H : TopHeaders) {
+SmallString<128> HeaderName(H.getName());
 PreparePathForOutput(HeaderName);
 Stream.EmitRecordWithBlob(TopHeaderAbbrev, Record, HeaderName);
   }
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -660,7 +660,7 @@
   Explicit).first;
   InferredModuleAllowedBy[Result] = UmbrellaModuleMap;
   Result->IsInferred = true;
-  Result->addTopHeader(File);
+  Result->addTopHeader(File->getLastRef());
 
   // If inferred submodules export everything they import, add a
   // wildcard to the set of exports.
Index: clang/lib/Basic/Module.cpp
===
--- clang/lib/Basic/Module.cpp
+++ clang/lib/Basic/Module.cpp
@@ -269,16 +269,16 @@
   Umbrella.dyn_cast()};
 }
 
-void Module::addTopHeader(const FileEntry *File) {
+void Module::addTopHeader(OptionalFileEntryRef File) {
   assert(File);
-  TopHeaders.insert(File);
+  TopHeaders.insert(*File);
 }
 
-ArrayRef Module::getTopHeaders(FileManager ) {
+ArrayRef Module::getTopHeaders(FileManager ) {
   if (!TopHeaderNames.empty()) {
 for (std::vector::iterator
I = TopHeaderNames.begin(), E = TopHeaderNames.end(); I != E; ++I) {
-  if (auto FE = FileMgr.getFile(*I))
+  if (auto FE = FileMgr.getFileRef(*I))
 TopHeaders.insert(*FE);
 }
 TopHeaderNames.clear();
Index: clang/include/clang/Basic/Module.h
===
--- clang/include/clang/Basic/Module.h
+++ clang/include/clang/Basic/Module.h
@@ -189,7 +189,7 @@
   OptionalFileEntryRef ASTFile;
 
   /// The top-level headers associated with this module.
-  llvm::SmallSetVector TopHeaders;
+  llvm::SmallSetVector TopHeaders;
 
   /// top-level header filenames that aren't resolved to FileEntries yet.
   std::vector TopHeaderNames;
@@ -639,7 +639,7 @@
   }
 
   /// Add a top-level header associated with this module.
-  void addTopHeader(const FileEntry *File);
+  void addTopHeader(OptionalFileEntryRef File);
 
   /// Add a top-level header filename associated with this module.
   void addTopHeaderFilename(StringRef Filename) {
@@ -647,7 +647,7 @@
   }
 
   /// The top-level headers associated with this module.
-  ArrayRef getTopHeaders(FileManager );
+  ArrayRef getTopHeaders(FileManager );
 
   /// Determine whether this module has declared its intention to
   /// directly use another module.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142113: [clang][nfc] refactor Module::Header to use OptionalFileEntryRef

[Richard Howell via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG75fbb5d2238f: [clang][nfc] refactor Module::Header to use 
OptionalFileEntryRef (authored by rmaz).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142113/new/

https://reviews.llvm.org/D142113

Files:
  clang/include/clang/Basic/Module.h
  clang/include/clang/Lex/ModuleMap.h
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Sema/SemaModule.cpp
  clang/lib/Serialization/ASTReader.cpp

Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -1947,7 +1947,7 @@
   Reader.ResolveImportedPath(M, Filename);
 // FIXME: NameAsWritten
 Module::Header H = {std::string(key.Filename), "",
-*FileMgr.getFile(Filename)};
+FileMgr.getOptionalFileRef(Filename)};
 ModMap.addHeader(Mod, H, HeaderRole, /*Imported*/true);
 HFI.isModuleHeader |= ModuleMap::isModular(HeaderRole);
   }
@@ -5661,7 +5661,7 @@
   //`Headers/`, so this path will never exist.
   std::string Filename = std::string(Blob);
   ResolveImportedPath(F, Filename);
-  if (auto Umbrella = PP.getFileManager().getFile(Filename)) {
+  if (auto Umbrella = PP.getFileManager().getOptionalFileRef(Filename)) {
 if (!CurrentModule->getUmbrellaHeader()) {
   // FIXME: NameAsWritten
   ModMap.setUmbrellaHeader(CurrentModule, *Umbrella, Blob, "");
Index: clang/lib/Sema/SemaModule.cpp
===
--- clang/lib/Sema/SemaModule.cpp
+++ clang/lib/Sema/SemaModule.cpp
@@ -120,12 +120,12 @@
   // TODO: Make the C++20 header lookup independent.
   // When the input is pre-processed source, we need a file ref to the original
   // file for the header map.
-  auto F = SourceMgr.getFileManager().getFile(HUName);
+  auto F = SourceMgr.getFileManager().getOptionalFileRef(HUName);
   // For the sake of error recovery (if someone has moved the original header
   // after creating the pre-processed output) fall back to obtaining the file
   // ref for the input file, which must be present.
   if (!F)
-F = SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
+F = SourceMgr.getFileEntryRefForID(SourceMgr.getMainFileID());
   assert(F && "failed to find the header unit source?");
   Module::Header H{HUName.str(), HUName.str(), *F};
   auto  = PP.getHeaderSearchInfo().getModuleMap();
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -304,7 +304,7 @@
   // supplied by Clang. Find that builtin header.
   SmallString<128> Path;
   llvm::sys::path::append(Path, BuiltinIncludeDir->getName(), Header.FileName);
-  auto File = SourceMgr.getFileManager().getFile(Path);
+  auto File = SourceMgr.getFileManager().getOptionalFileRef(Path);
   if (!File)
 return false;
 
@@ -1024,7 +1024,7 @@
   // Look for an umbrella header.
   SmallString<128> UmbrellaName = StringRef(FrameworkDir->getName());
   llvm::sys::path::append(UmbrellaName, "Headers", ModuleName + ".h");
-  auto UmbrellaHeader = FileMgr.getFile(UmbrellaName);
+  auto UmbrellaHeader = FileMgr.getOptionalFileRef(UmbrellaName);
 
   // FIXME: If there's no umbrella header, we could probably scan the
   // framework to load *everything*. But, it's not clear that this is a good
@@ -1136,14 +1136,14 @@
 }
 
 void ModuleMap::setUmbrellaHeader(
-Module *Mod, const FileEntry *UmbrellaHeader, const Twine ,
+Module *Mod, FileEntryRef UmbrellaHeader, const Twine ,
 const Twine ) {
   Headers[UmbrellaHeader].push_back(KnownHeader(Mod, NormalHeader));
-  Mod->Umbrella = UmbrellaHeader;
+  Mod->Umbrella = ();
   Mod->UmbrellaAsWritten = NameAsWritten.str();
   Mod->UmbrellaRelativeToRootModuleDirectory =
   PathRelativeToRootModuleDirectory.str();
-  UmbrellaDirs[UmbrellaHeader->getDir()] = Mod;
+  UmbrellaDirs[UmbrellaHeader.getDir()] = Mod;
 
   // Notify callbacks that we just added a new header.
   for (const auto  : Callbacks)
@@ -2510,8 +2510,8 @@
 SourceMgr.getFileManager().getVirtualFileSystem();
 for (llvm::vfs::recursive_directory_iterator I(FS, Dir->getName(), EC), E;
  I != E && !EC; I.increment(EC)) {
-  if (auto FE = SourceMgr.getFileManager().getFile(I->path())) {
-Module::Header Header = {"", std::string(I->path()), *FE};
+  if (auto FE = SourceMgr.getFileManager().getOptionalFileRef(I->path())) {
+Module::Header Header = {"", std::string(I->path()), FE};
 Headers.push_back(std::move(Header));
   }
 }
Index: clang/lib/Frontend/FrontendAction.cpp

[PATCH] D142113: [clang][nfc] refactor Module::Header to use OptionalFileEntryRef

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 490624.
rmaz added a comment.

rebase on last successful build: 
https://buildkite.com/llvm-project/llvm-main/builds/6356


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142113/new/

https://reviews.llvm.org/D142113

Files:
  clang/include/clang/Basic/Module.h
  clang/include/clang/Lex/ModuleMap.h
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Sema/SemaModule.cpp
  clang/lib/Serialization/ASTReader.cpp

Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -1947,7 +1947,7 @@
   Reader.ResolveImportedPath(M, Filename);
 // FIXME: NameAsWritten
 Module::Header H = {std::string(key.Filename), "",
-*FileMgr.getFile(Filename)};
+FileMgr.getOptionalFileRef(Filename)};
 ModMap.addHeader(Mod, H, HeaderRole, /*Imported*/true);
 HFI.isModuleHeader |= ModuleMap::isModular(HeaderRole);
   }
@@ -5661,7 +5661,7 @@
   //`Headers/`, so this path will never exist.
   std::string Filename = std::string(Blob);
   ResolveImportedPath(F, Filename);
-  if (auto Umbrella = PP.getFileManager().getFile(Filename)) {
+  if (auto Umbrella = PP.getFileManager().getOptionalFileRef(Filename)) {
 if (!CurrentModule->getUmbrellaHeader()) {
   // FIXME: NameAsWritten
   ModMap.setUmbrellaHeader(CurrentModule, *Umbrella, Blob, "");
Index: clang/lib/Sema/SemaModule.cpp
===
--- clang/lib/Sema/SemaModule.cpp
+++ clang/lib/Sema/SemaModule.cpp
@@ -120,12 +120,12 @@
   // TODO: Make the C++20 header lookup independent.
   // When the input is pre-processed source, we need a file ref to the original
   // file for the header map.
-  auto F = SourceMgr.getFileManager().getFile(HUName);
+  auto F = SourceMgr.getFileManager().getOptionalFileRef(HUName);
   // For the sake of error recovery (if someone has moved the original header
   // after creating the pre-processed output) fall back to obtaining the file
   // ref for the input file, which must be present.
   if (!F)
-F = SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
+F = SourceMgr.getFileEntryRefForID(SourceMgr.getMainFileID());
   assert(F && "failed to find the header unit source?");
   Module::Header H{HUName.str(), HUName.str(), *F};
   auto  = PP.getHeaderSearchInfo().getModuleMap();
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -304,7 +304,7 @@
   // supplied by Clang. Find that builtin header.
   SmallString<128> Path;
   llvm::sys::path::append(Path, BuiltinIncludeDir->getName(), Header.FileName);
-  auto File = SourceMgr.getFileManager().getFile(Path);
+  auto File = SourceMgr.getFileManager().getOptionalFileRef(Path);
   if (!File)
 return false;
 
@@ -1024,7 +1024,7 @@
   // Look for an umbrella header.
   SmallString<128> UmbrellaName = StringRef(FrameworkDir->getName());
   llvm::sys::path::append(UmbrellaName, "Headers", ModuleName + ".h");
-  auto UmbrellaHeader = FileMgr.getFile(UmbrellaName);
+  auto UmbrellaHeader = FileMgr.getOptionalFileRef(UmbrellaName);
 
   // FIXME: If there's no umbrella header, we could probably scan the
   // framework to load *everything*. But, it's not clear that this is a good
@@ -1136,14 +1136,14 @@
 }
 
 void ModuleMap::setUmbrellaHeader(
-Module *Mod, const FileEntry *UmbrellaHeader, const Twine ,
+Module *Mod, FileEntryRef UmbrellaHeader, const Twine ,
 const Twine ) {
   Headers[UmbrellaHeader].push_back(KnownHeader(Mod, NormalHeader));
-  Mod->Umbrella = UmbrellaHeader;
+  Mod->Umbrella = ();
   Mod->UmbrellaAsWritten = NameAsWritten.str();
   Mod->UmbrellaRelativeToRootModuleDirectory =
   PathRelativeToRootModuleDirectory.str();
-  UmbrellaDirs[UmbrellaHeader->getDir()] = Mod;
+  UmbrellaDirs[UmbrellaHeader.getDir()] = Mod;
 
   // Notify callbacks that we just added a new header.
   for (const auto  : Callbacks)
@@ -2510,8 +2510,8 @@
 SourceMgr.getFileManager().getVirtualFileSystem();
 for (llvm::vfs::recursive_directory_iterator I(FS, Dir->getName(), EC), E;
  I != E && !EC; I.increment(EC)) {
-  if (auto FE = SourceMgr.getFileManager().getFile(I->path())) {
-Module::Header Header = {"", std::string(I->path()), *FE};
+  if (auto FE = SourceMgr.getFileManager().getOptionalFileRef(I->path())) {
+Module::Header Header = {"", std::string(I->path()), FE};
 Headers.push_back(std::move(Header));
   }
 }
Index: clang/lib/Frontend/FrontendAction.cpp
===
--- clang/lib/Frontend/FrontendAction.cpp
+++ 

[PATCH] D142113: [clang][nfc] refactor Module::Header to use OptionalFileEntryRef

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 490568.
rmaz added a comment.

address comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142113/new/

https://reviews.llvm.org/D142113

Files:
  clang/include/clang/Basic/Module.h
  clang/include/clang/Lex/ModuleMap.h
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Sema/SemaModule.cpp
  clang/lib/Serialization/ASTReader.cpp

Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -1947,7 +1947,7 @@
   Reader.ResolveImportedPath(M, Filename);
 // FIXME: NameAsWritten
 Module::Header H = {std::string(key.Filename), "",
-*FileMgr.getFile(Filename)};
+FileMgr.getOptionalFileRef(Filename)};
 ModMap.addHeader(Mod, H, HeaderRole, /*Imported*/true);
 HFI.isModuleHeader |= ModuleMap::isModular(HeaderRole);
   }
@@ -5661,7 +5661,7 @@
   //`Headers/`, so this path will never exist.
   std::string Filename = std::string(Blob);
   ResolveImportedPath(F, Filename);
-  if (auto Umbrella = PP.getFileManager().getFile(Filename)) {
+  if (auto Umbrella = PP.getFileManager().getOptionalFileRef(Filename)) {
 if (!CurrentModule->getUmbrellaHeader()) {
   // FIXME: NameAsWritten
   ModMap.setUmbrellaHeader(CurrentModule, *Umbrella, Blob, "");
Index: clang/lib/Sema/SemaModule.cpp
===
--- clang/lib/Sema/SemaModule.cpp
+++ clang/lib/Sema/SemaModule.cpp
@@ -120,12 +120,12 @@
   // TODO: Make the C++20 header lookup independent.
   // When the input is pre-processed source, we need a file ref to the original
   // file for the header map.
-  auto F = SourceMgr.getFileManager().getFile(HUName);
+  auto F = SourceMgr.getFileManager().getOptionalFileRef(HUName);
   // For the sake of error recovery (if someone has moved the original header
   // after creating the pre-processed output) fall back to obtaining the file
   // ref for the input file, which must be present.
   if (!F)
-F = SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
+F = SourceMgr.getFileEntryRefForID(SourceMgr.getMainFileID());
   assert(F && "failed to find the header unit source?");
   Module::Header H{HUName.str(), HUName.str(), *F};
   auto  = PP.getHeaderSearchInfo().getModuleMap();
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -304,7 +304,7 @@
   // supplied by Clang. Find that builtin header.
   SmallString<128> Path;
   llvm::sys::path::append(Path, BuiltinIncludeDir->getName(), Header.FileName);
-  auto File = SourceMgr.getFileManager().getFile(Path);
+  auto File = SourceMgr.getFileManager().getOptionalFileRef(Path);
   if (!File)
 return false;
 
@@ -1024,7 +1024,7 @@
   // Look for an umbrella header.
   SmallString<128> UmbrellaName = StringRef(FrameworkDir->getName());
   llvm::sys::path::append(UmbrellaName, "Headers", ModuleName + ".h");
-  auto UmbrellaHeader = FileMgr.getFile(UmbrellaName);
+  auto UmbrellaHeader = FileMgr.getOptionalFileRef(UmbrellaName);
 
   // FIXME: If there's no umbrella header, we could probably scan the
   // framework to load *everything*. But, it's not clear that this is a good
@@ -1136,14 +1136,14 @@
 }
 
 void ModuleMap::setUmbrellaHeader(
-Module *Mod, const FileEntry *UmbrellaHeader, const Twine ,
+Module *Mod, FileEntryRef UmbrellaHeader, const Twine ,
 const Twine ) {
   Headers[UmbrellaHeader].push_back(KnownHeader(Mod, NormalHeader));
-  Mod->Umbrella = UmbrellaHeader;
+  Mod->Umbrella = ();
   Mod->UmbrellaAsWritten = NameAsWritten.str();
   Mod->UmbrellaRelativeToRootModuleDirectory =
   PathRelativeToRootModuleDirectory.str();
-  UmbrellaDirs[UmbrellaHeader->getDir()] = Mod;
+  UmbrellaDirs[UmbrellaHeader.getDir()] = Mod;
 
   // Notify callbacks that we just added a new header.
   for (const auto  : Callbacks)
@@ -2510,8 +2510,8 @@
 SourceMgr.getFileManager().getVirtualFileSystem();
 for (llvm::vfs::recursive_directory_iterator I(FS, Dir->getName(), EC), E;
  I != E && !EC; I.increment(EC)) {
-  if (auto FE = SourceMgr.getFileManager().getFile(I->path())) {
-Module::Header Header = {"", std::string(I->path()), *FE};
+  if (auto FE = SourceMgr.getFileManager().getOptionalFileRef(I->path())) {
+Module::Header Header = {"", std::string(I->path()), FE};
 Headers.push_back(std::move(Header));
   }
 }
Index: clang/lib/Frontend/FrontendAction.cpp
===
--- clang/lib/Frontend/FrontendAction.cpp
+++ clang/lib/Frontend/FrontendAction.cpp
@@ -12,6 +12,7 @@
 #include "clang/AST/DeclGroup.h"
 #include 

[PATCH] D142028: [clang] remove SUBMODULE_TOPHEADER

[Richard Howell via Phabricator via cfe-commits]

rmaz abandoned this revision.
rmaz added a comment.

Its going to be less work to refactor TopheaderNames to use FileEntryRef, so 
went that way instead. First step is here: https://reviews.llvm.org/D142113


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142028/new/

https://reviews.llvm.org/D142028

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142113: [clang][nfc] refactor Module::Header to use OptionalFileEntryRef

[Richard Howell via Phabricator via cfe-commits]

rmaz created this revision.
Herald added a subscriber: ChuanqiXu.
Herald added a project: All.
rmaz requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Refactor the `Module::Header` class to use an `OptionalFileEntryRef` 
instead of a `FileEntry*`.

This is mostly based on https://reviews.llvm.org/D90497


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142113

Files:
  clang/include/clang/Basic/Module.h
  clang/include/clang/Lex/ModuleMap.h
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Sema/SemaModule.cpp
  clang/lib/Serialization/ASTReader.cpp

Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -1947,7 +1947,7 @@
   Reader.ResolveImportedPath(M, Filename);
 // FIXME: NameAsWritten
 Module::Header H = {std::string(key.Filename), "",
-*FileMgr.getFile(Filename)};
+*FileMgr.getOptionalFileRef(Filename)};
 ModMap.addHeader(Mod, H, HeaderRole, /*Imported*/true);
 HFI.isModuleHeader |= ModuleMap::isModular(HeaderRole);
   }
@@ -5661,7 +5661,7 @@
   //`Headers/`, so this path will never exist.
   std::string Filename = std::string(Blob);
   ResolveImportedPath(F, Filename);
-  if (auto Umbrella = PP.getFileManager().getFile(Filename)) {
+  if (auto Umbrella = PP.getFileManager().getOptionalFileRef(Filename)) {
 if (!CurrentModule->getUmbrellaHeader()) {
   // FIXME: NameAsWritten
   ModMap.setUmbrellaHeader(CurrentModule, *Umbrella, Blob, "");
Index: clang/lib/Sema/SemaModule.cpp
===
--- clang/lib/Sema/SemaModule.cpp
+++ clang/lib/Sema/SemaModule.cpp
@@ -120,12 +120,12 @@
   // TODO: Make the C++20 header lookup independent.
   // When the input is pre-processed source, we need a file ref to the original
   // file for the header map.
-  auto F = SourceMgr.getFileManager().getFile(HUName);
+  auto F = SourceMgr.getFileManager().getOptionalFileRef(HUName);
   // For the sake of error recovery (if someone has moved the original header
   // after creating the pre-processed output) fall back to obtaining the file
   // ref for the input file, which must be present.
   if (!F)
-F = SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
+F = SourceMgr.getFileEntryRefForID(SourceMgr.getMainFileID());
   assert(F && "failed to find the header unit source?");
   Module::Header H{HUName.str(), HUName.str(), *F};
   auto  = PP.getHeaderSearchInfo().getModuleMap();
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -304,7 +304,7 @@
   // supplied by Clang. Find that builtin header.
   SmallString<128> Path;
   llvm::sys::path::append(Path, BuiltinIncludeDir->getName(), Header.FileName);
-  auto File = SourceMgr.getFileManager().getFile(Path);
+  auto File = SourceMgr.getFileManager().getOptionalFileRef(Path);
   if (!File)
 return false;
 
@@ -1024,7 +1024,7 @@
   // Look for an umbrella header.
   SmallString<128> UmbrellaName = StringRef(FrameworkDir->getName());
   llvm::sys::path::append(UmbrellaName, "Headers", ModuleName + ".h");
-  auto UmbrellaHeader = FileMgr.getFile(UmbrellaName);
+  auto UmbrellaHeader = FileMgr.getOptionalFileRef(UmbrellaName);
 
   // FIXME: If there's no umbrella header, we could probably scan the
   // framework to load *everything*. But, it's not clear that this is a good
@@ -1136,14 +1136,14 @@
 }
 
 void ModuleMap::setUmbrellaHeader(
-Module *Mod, const FileEntry *UmbrellaHeader, const Twine ,
+Module *Mod, FileEntryRef UmbrellaHeader, const Twine ,
 const Twine ) {
   Headers[UmbrellaHeader].push_back(KnownHeader(Mod, NormalHeader));
-  Mod->Umbrella = UmbrellaHeader;
+  Mod->Umbrella = ();
   Mod->UmbrellaAsWritten = NameAsWritten.str();
   Mod->UmbrellaRelativeToRootModuleDirectory =
   PathRelativeToRootModuleDirectory.str();
-  UmbrellaDirs[UmbrellaHeader->getDir()] = Mod;
+  UmbrellaDirs[UmbrellaHeader.getDir()] = Mod;
 
   // Notify callbacks that we just added a new header.
   for (const auto  : Callbacks)
@@ -2510,7 +2510,7 @@
 SourceMgr.getFileManager().getVirtualFileSystem();
 for (llvm::vfs::recursive_directory_iterator I(FS, Dir->getName(), EC), E;
  I != E && !EC; I.increment(EC)) {
-  if (auto FE = SourceMgr.getFileManager().getFile(I->path())) {
+  if (auto FE = SourceMgr.getFileManager().getOptionalFileRef(I->path())) {
 Module::Header Header = {"", std::string(I->path()), *FE};
 Headers.push_back(std::move(Header));
   }
Index: clang/lib/Frontend/FrontendAction.cpp
===
--- 

[PATCH] D142028: [clang] remove SUBMODULE_TOPHEADER

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D142028#4062798 , @akyrtzi wrote:

> Is it reasonable to at least have an alternative implementation for it (e.g. 
> from the module headers find the headers that were not included from other 
> headers), not sure if it'd be straightforward or not.

Its feasible to refactor by moving the `collectModuleHeaderIncludes` function 
to be a method on `Module` with a callback. Then we could avoid serializing the 
top header list and call the method in CIndex as well as FrontendAction. I'll 
take a look at how much work this is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142028/new/

https://reviews.llvm.org/D142028

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142028: [clang] remove SUBMODULE_TOPHEADER

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

@akyrtzi it looks like you added this field a while back. Do you know if there 
are still consumers for the the libclang API, do we need to keep this field 
around?

The alternative is to refactor the `TopHeaders` set to use `FileEntryRef` so 
that we can keep track of the correct import names for the headers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142028/new/

https://reviews.llvm.org/D142028

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142028: [clang] remove SUBMODULE_TOPHEADER

[Richard Howell via Phabricator via cfe-commits]

rmaz created this revision.
Herald added a subscriber: arphaman.
Herald added a project: All.
rmaz requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

We are seeing issues with path serialization of the
`SUBMODULE_TOPHEADER` field when the inputs are links. While working
on a fix I noticed this field does not appear to be used anywhere
outside of libclang. This diff removes the field entirely and 
updates the libclang API to always return empty top headers.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142028

Files:
  clang/include/clang/Basic/Module.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/Basic/Module.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Index/annotate-module.m
  clang/test/Modules/relative-submodule-topheader.m
  clang/tools/libclang/CIndex.cpp

Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -8777,31 +8777,15 @@
 CXModule CXMod) {
   if (isNotUsableTU(TU)) {
 LOG_BAD_TU(TU);
-return 0;
   }
-  if (!CXMod)
-return 0;
-  Module *Mod = static_cast(CXMod);
-  FileManager  = cxtu::getASTUnit(TU)->getFileManager();
-  ArrayRef TopHeaders = Mod->getTopHeaders(FileMgr);
-  return TopHeaders.size();
+  return 0;
 }
 
 CXFile clang_Module_getTopLevelHeader(CXTranslationUnit TU, CXModule CXMod,
   unsigned Index) {
   if (isNotUsableTU(TU)) {
 LOG_BAD_TU(TU);
-return nullptr;
   }
-  if (!CXMod)
-return nullptr;
-  Module *Mod = static_cast(CXMod);
-  FileManager  = cxtu::getASTUnit(TU)->getFileManager();
-
-  ArrayRef TopHeaders = Mod->getTopHeaders(FileMgr);
-  if (Index < TopHeaders.size())
-return const_cast(TopHeaders[Index]);
-
   return nullptr;
 }
 
Index: clang/test/Modules/relative-submodule-topheader.m
===
--- clang/test/Modules/relative-submodule-topheader.m
+++ clang/test/Modules/relative-submodule-topheader.m
@@ -3,8 +3,5 @@
 // RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s
 
 // CHECK:  blob data = 'vector.h'
-// CHECK:  blob data = 'vector.h'
 // CHECK:  blob data = 'type_traits.h'
-// CHECK:  blob data = 'type_traits.h'
 // CHECK:  blob data = 'hash_map.h'
-// CHECK:  blob data = 'hash_map.h'
Index: clang/test/Index/annotate-module.m
===
--- clang/test/Index/annotate-module.m
+++ clang/test/Index/annotate-module.m
@@ -46,6 +46,4 @@
 // RUN: c-index-test -cursor-at=%s:3:11 %s -fmodules-cache-path=%t.cache -fmodules -F %S/../Modules/Inputs \
 // RUN: | FileCheck %s -check-prefix=CHECK-CURSOR
 
-// CHECK-CURSOR:  3:1 ModuleImport=DependsOnModule:3:1 (Definition) Extent=[3:1 - 3:24] Spelling=DependsOnModule ([3:9 - 3:24]) ModuleName=DependsOnModule ({{.*}}DependsOnModule-{{[^.]*}}.pcm) system=0 Headers(2):
-// CHECK-CURSOR-NEXT: {{.*}}other.h
-// CHECK-CURSOR-NEXT: {{.*}}DependsOnModule.h
+// CHECK-CURSOR:  3:1 ModuleImport=DependsOnModule:3:1 (Definition) Extent=[3:1 - 3:24] Spelling=DependsOnModule ([3:9 - 3:24]) ModuleName=DependsOnModule ({{.*}}DependsOnModule-{{[^.]*}}.pcm) system=0 Headers(0):
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -890,7 +890,6 @@
   RECORD(SUBMODULE_DEFINITION);
   RECORD(SUBMODULE_UMBRELLA_HEADER);
   RECORD(SUBMODULE_HEADER);
-  RECORD(SUBMODULE_TOPHEADER);
   RECORD(SUBMODULE_UMBRELLA_DIR);
   RECORD(SUBMODULE_IMPORTS);
   RECORD(SUBMODULE_AFFECTING_MODULES);
@@ -2741,11 +2740,6 @@
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // Name
   unsigned HeaderAbbrev = Stream.EmitAbbrev(std::move(Abbrev));
 
-  Abbrev = std::make_shared();
-  Abbrev->Add(BitCodeAbbrevOp(SUBMODULE_TOPHEADER));
-  Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // Name
-  unsigned TopHeaderAbbrev = Stream.EmitAbbrev(std::move(Abbrev));
-
   Abbrev = std::make_shared();
   Abbrev->Add(BitCodeAbbrevOp(SUBMODULE_UMBRELLA_DIR));
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // Name
@@ -2873,17 +2867,6 @@
 Stream.EmitRecordWithBlob(HL.Abbrev, Record, H.NameAsWritten);
 }
 
-// Emit the top headers.
-{
-  auto TopHeaders = Mod->getTopHeaders(PP->getFileManager());
-  RecordData::value_type Record[] = {SUBMODULE_TOPHEADER};
-  for (auto *H : TopHeaders) {
-SmallString<128> HeaderName(H->getName());
-PreparePathForOutput(HeaderName);
-Stream.EmitRecordWithBlob(TopHeaderAbbrev, Record, HeaderName);
-  }
-}
-
 // Emit the imports.
 

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D133586#3831618 , @vsapsai wrote:

> How correct is it to access `isConst`, `isVolatile`, `isRestrict` for 
> `FunctionNoProtoType`? Yes, we can provide some default value but I'm curious 
> if accessing that default value is correct.
>
> For the record, I've tried to fix the same problem in 
> https://reviews.llvm.org/D104963 in a different way.

That was my initial solution as well, but it seemed safer to ensure these 
methods always returned a consistent value without auditing all the possible 
call sites. I agree that it doesn't seem right that these methods are on the 
base class at all if they are only valid in one of the subclasses.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134911: [clang][DebugInfo] Respect fmodule-file-home-is-cwd in skeleton CUs for clang modules

[Richard Howell via Phabricator via cfe-commits]

rmaz accepted this revision.
rmaz added a comment.
This revision is now accepted and ready to land.

Nice, LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134911/new/

https://reviews.llvm.org/D134911

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

[Richard Howell via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG1f451a8bd6f3: [clang] initialize type qualifiers for 
FunctionNoProtoType (authored by rmaz).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

Files:
  clang/include/clang/AST/Type.h
  clang/unittests/AST/DeclTest.cpp


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -354,3 +354,25 @@
 
   EXPECT_TRUE(getFooValue->isInlined());
 }
+
+TEST(Decl, NoProtoFunctionDeclAttributes) {
+  llvm::Annotations Code(R"(
+void f();
+)");
+
+  auto AST = tooling::buildASTFromCodeWithArgs(
+  Code.code(),
+  /*Args=*/{"-target", "i386-apple-darwin", "-x", "objective-c",
+"-std=c89"});
+  ASTContext  = AST->getASTContext();
+
+  auto *f = selectFirst(
+  "f", match(functionDecl(hasName("f")).bind("f"), Ctx));
+
+  const auto *FPT = f->getType()->getAs();
+
+  // Functions without prototypes always have 0 initialized qualifiers
+  EXPECT_FALSE(FPT->isConst());
+  EXPECT_FALSE(FPT->isVolatile());
+  EXPECT_FALSE(FPT->isRestrict());
+}
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -3892,7 +3892,10 @@
   }
 
   Qualifiers getFastTypeQuals() const {
-return Qualifiers::fromFastMask(FunctionTypeBits.FastTypeQuals);
+if (isFunctionProtoType())
+  return Qualifiers::fromFastMask(FunctionTypeBits.FastTypeQuals);
+
+return Qualifiers();
   }
 
 public:


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -354,3 +354,25 @@
 
   EXPECT_TRUE(getFooValue->isInlined());
 }
+
+TEST(Decl, NoProtoFunctionDeclAttributes) {
+  llvm::Annotations Code(R"(
+void f();
+)");
+
+  auto AST = tooling::buildASTFromCodeWithArgs(
+  Code.code(),
+  /*Args=*/{"-target", "i386-apple-darwin", "-x", "objective-c",
+"-std=c89"});
+  ASTContext  = AST->getASTContext();
+
+  auto *f = selectFirst(
+  "f", match(functionDecl(hasName("f")).bind("f"), Ctx));
+
+  const auto *FPT = f->getType()->getAs();
+
+  // Functions without prototypes always have 0 initialized qualifiers
+  EXPECT_FALSE(FPT->isConst());
+  EXPECT_FALSE(FPT->isVolatile());
+  EXPECT_FALSE(FPT->isRestrict());
+}
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -3892,7 +3892,10 @@
   }
 
   Qualifiers getFastTypeQuals() const {
-return Qualifiers::fromFastMask(FunctionTypeBits.FastTypeQuals);
+if (isFunctionProtoType())
+  return Qualifiers::fromFastMask(FunctionTypeBits.FastTypeQuals);
+
+return Qualifiers();
   }
 
 public:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 462898.
rmaz added a comment.

remove else case


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

Files:
  clang/include/clang/AST/Type.h
  clang/unittests/AST/DeclTest.cpp


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -354,3 +354,25 @@
 
   EXPECT_TRUE(getFooValue->isInlined());
 }
+
+TEST(Decl, NoProtoFunctionDeclAttributes) {
+  llvm::Annotations Code(R"(
+void f();
+)");
+
+  auto AST = tooling::buildASTFromCodeWithArgs(
+  Code.code(),
+  /*Args=*/{"-target", "i386-apple-darwin", "-x", "objective-c",
+"-std=c89"});
+  ASTContext  = AST->getASTContext();
+
+  auto *f = selectFirst(
+  "f", match(functionDecl(hasName("f")).bind("f"), Ctx));
+
+  const auto *FPT = f->getType()->getAs();
+
+  // Functions without prototypes always have 0 initialized qualifiers
+  EXPECT_FALSE(FPT->isConst());
+  EXPECT_FALSE(FPT->isVolatile());
+  EXPECT_FALSE(FPT->isRestrict());
+}
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -3892,7 +3892,10 @@
   }
 
   Qualifiers getFastTypeQuals() const {
-return Qualifiers::fromFastMask(FunctionTypeBits.FastTypeQuals);
+if (isFunctionProtoType())
+  return Qualifiers::fromFastMask(FunctionTypeBits.FastTypeQuals);
+
+return Qualifiers();
   }
 
 public:


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -354,3 +354,25 @@
 
   EXPECT_TRUE(getFooValue->isInlined());
 }
+
+TEST(Decl, NoProtoFunctionDeclAttributes) {
+  llvm::Annotations Code(R"(
+void f();
+)");
+
+  auto AST = tooling::buildASTFromCodeWithArgs(
+  Code.code(),
+  /*Args=*/{"-target", "i386-apple-darwin", "-x", "objective-c",
+"-std=c89"});
+  ASTContext  = AST->getASTContext();
+
+  auto *f = selectFirst(
+  "f", match(functionDecl(hasName("f")).bind("f"), Ctx));
+
+  const auto *FPT = f->getType()->getAs();
+
+  // Functions without prototypes always have 0 initialized qualifiers
+  EXPECT_FALSE(FPT->isConst());
+  EXPECT_FALSE(FPT->isVolatile());
+  EXPECT_FALSE(FPT->isRestrict());
+}
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -3892,7 +3892,10 @@
   }
 
   Qualifiers getFastTypeQuals() const {
-return Qualifiers::fromFastMask(FunctionTypeBits.FastTypeQuals);
+if (isFunctionProtoType())
+  return Qualifiers::fromFastMask(FunctionTypeBits.FastTypeQuals);
+
+return Qualifiers();
   }
 
 public:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 462518.
rmaz added a comment.

Remove unnecessary friend class


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

Files:
  clang/include/clang/AST/Type.h
  clang/unittests/AST/DeclTest.cpp


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -354,3 +354,25 @@
 
   EXPECT_TRUE(getFooValue->isInlined());
 }
+
+TEST(Decl, NoProtoFunctionDeclAttributes) {
+  llvm::Annotations Code(R"(
+void f();
+)");
+
+  auto AST = tooling::buildASTFromCodeWithArgs(
+  Code.code(),
+  /*Args=*/{"-target", "i386-apple-darwin", "-x", "objective-c",
+"-std=c89"});
+  ASTContext  = AST->getASTContext();
+
+  auto *f = selectFirst(
+  "f", match(functionDecl(hasName("f")).bind("f"), Ctx));
+
+  const auto *FPT = f->getType()->getAs();
+
+  // Functions without prototypes always have 0 initialized qualifiers
+  EXPECT_FALSE(FPT->isConst());
+  EXPECT_FALSE(FPT->isVolatile());
+  EXPECT_FALSE(FPT->isRestrict());
+}
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -3892,7 +3892,10 @@
   }
 
   Qualifiers getFastTypeQuals() const {
-return Qualifiers::fromFastMask(FunctionTypeBits.FastTypeQuals);
+if (isFunctionProtoType())
+  return Qualifiers::fromFastMask(FunctionTypeBits.FastTypeQuals);
+else
+  return Qualifiers();
   }
 
 public:


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -354,3 +354,25 @@
 
   EXPECT_TRUE(getFooValue->isInlined());
 }
+
+TEST(Decl, NoProtoFunctionDeclAttributes) {
+  llvm::Annotations Code(R"(
+void f();
+)");
+
+  auto AST = tooling::buildASTFromCodeWithArgs(
+  Code.code(),
+  /*Args=*/{"-target", "i386-apple-darwin", "-x", "objective-c",
+"-std=c89"});
+  ASTContext  = AST->getASTContext();
+
+  auto *f = selectFirst(
+  "f", match(functionDecl(hasName("f")).bind("f"), Ctx));
+
+  const auto *FPT = f->getType()->getAs();
+
+  // Functions without prototypes always have 0 initialized qualifiers
+  EXPECT_FALSE(FPT->isConst());
+  EXPECT_FALSE(FPT->isVolatile());
+  EXPECT_FALSE(FPT->isRestrict());
+}
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -3892,7 +3892,10 @@
   }
 
   Qualifiers getFastTypeQuals() const {
-return Qualifiers::fromFastMask(FunctionTypeBits.FastTypeQuals);
+if (isFunctionProtoType())
+  return Qualifiers::fromFastMask(FunctionTypeBits.FastTypeQuals);
+else
+  return Qualifiers();
   }
 
 public:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 462483.
rmaz added a comment.

Return default Qualifiers for FunctionNoProtoType


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

Files:
  clang/include/clang/AST/Type.h
  clang/unittests/AST/DeclTest.cpp


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -354,3 +354,25 @@
 
   EXPECT_TRUE(getFooValue->isInlined());
 }
+
+TEST(Decl, NoProtoFunctionDeclAttributes) {
+  llvm::Annotations Code(R"(
+void f();
+)");
+
+  auto AST = tooling::buildASTFromCodeWithArgs(
+  Code.code(),
+  /*Args=*/{"-target", "i386-apple-darwin", "-x", "objective-c",
+"-std=c89"});
+  ASTContext  = AST->getASTContext();
+
+  auto *f = selectFirst(
+  "f", match(functionDecl(hasName("f")).bind("f"), Ctx));
+
+  const auto *FPT = f->getType()->getAs();
+
+  // Functions without prototypes always have 0 initialized qualifiers
+  EXPECT_FALSE(FPT->isConst());
+  EXPECT_FALSE(FPT->isVolatile());
+  EXPECT_FALSE(FPT->isRestrict());
+}
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -1647,6 +1647,7 @@
   /// Only common bits are stored here. Additional uncommon bits are stored
   /// in a trailing object after FunctionProtoType.
   class FunctionTypeBitfields {
+friend class FunctionNoProtoType;
 friend class FunctionProtoType;
 friend class FunctionType;
 
@@ -3892,7 +3893,10 @@
   }
 
   Qualifiers getFastTypeQuals() const {
-return Qualifiers::fromFastMask(FunctionTypeBits.FastTypeQuals);
+if (isFunctionProtoType())
+  return Qualifiers::fromFastMask(FunctionTypeBits.FastTypeQuals);
+else
+  return Qualifiers();
   }
 
 public:


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -354,3 +354,25 @@
 
   EXPECT_TRUE(getFooValue->isInlined());
 }
+
+TEST(Decl, NoProtoFunctionDeclAttributes) {
+  llvm::Annotations Code(R"(
+void f();
+)");
+
+  auto AST = tooling::buildASTFromCodeWithArgs(
+  Code.code(),
+  /*Args=*/{"-target", "i386-apple-darwin", "-x", "objective-c",
+"-std=c89"});
+  ASTContext  = AST->getASTContext();
+
+  auto *f = selectFirst(
+  "f", match(functionDecl(hasName("f")).bind("f"), Ctx));
+
+  const auto *FPT = f->getType()->getAs();
+
+  // Functions without prototypes always have 0 initialized qualifiers
+  EXPECT_FALSE(FPT->isConst());
+  EXPECT_FALSE(FPT->isVolatile());
+  EXPECT_FALSE(FPT->isRestrict());
+}
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -1647,6 +1647,7 @@
   /// Only common bits are stored here. Additional uncommon bits are stored
   /// in a trailing object after FunctionProtoType.
   class FunctionTypeBitfields {
+friend class FunctionNoProtoType;
 friend class FunctionProtoType;
 friend class FunctionType;
 
@@ -3892,7 +3893,10 @@
   }
 
   Qualifiers getFastTypeQuals() const {
-return Qualifiers::fromFastMask(FunctionTypeBits.FastTypeQuals);
+if (isFunctionProtoType())
+  return Qualifiers::fromFastMask(FunctionTypeBits.FastTypeQuals);
+else
+  return Qualifiers();
   }
 
 public:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 462275.
rmaz added a comment.

add -std=c89 to test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

Files:
  clang/include/clang/AST/Type.h
  clang/unittests/AST/DeclTest.cpp


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -354,3 +354,25 @@
 
   EXPECT_TRUE(getFooValue->isInlined());
 }
+
+TEST(Decl, NoProtoFunctionDeclAttributes) {
+  llvm::Annotations Code(R"(
+void f();
+)");
+
+  auto AST = tooling::buildASTFromCodeWithArgs(
+  Code.code(),
+  /*Args=*/{"-target", "i386-apple-darwin", "-x", "objective-c",
+"-std=c89"});
+  ASTContext  = AST->getASTContext();
+
+  auto *f = selectFirst(
+  "f", match(functionDecl(hasName("f")).bind("f"), Ctx));
+
+  const auto *FPT = f->getType()->getAs();
+
+  // Functions without prototypes always have 0 initialized qualifiers
+  EXPECT_FALSE(FPT->isConst());
+  EXPECT_FALSE(FPT->isVolatile());
+  EXPECT_FALSE(FPT->isRestrict());
+}
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -1647,6 +1647,7 @@
   /// Only common bits are stored here. Additional uncommon bits are stored
   /// in a trailing object after FunctionProtoType.
   class FunctionTypeBitfields {
+friend class FunctionNoProtoType;
 friend class FunctionProtoType;
 friend class FunctionType;
 
@@ -3942,7 +3943,9 @@
  Result->getDependence() &
  ~(TypeDependence::DependentInstantiation |
TypeDependence::UnexpandedPack),
- Info) {}
+ Info) {
+FunctionTypeBits.FastTypeQuals = 0;
+  }
 
 public:
   // No additional state past what FunctionType provides.


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -354,3 +354,25 @@
 
   EXPECT_TRUE(getFooValue->isInlined());
 }
+
+TEST(Decl, NoProtoFunctionDeclAttributes) {
+  llvm::Annotations Code(R"(
+void f();
+)");
+
+  auto AST = tooling::buildASTFromCodeWithArgs(
+  Code.code(),
+  /*Args=*/{"-target", "i386-apple-darwin", "-x", "objective-c",
+"-std=c89"});
+  ASTContext  = AST->getASTContext();
+
+  auto *f = selectFirst(
+  "f", match(functionDecl(hasName("f")).bind("f"), Ctx));
+
+  const auto *FPT = f->getType()->getAs();
+
+  // Functions without prototypes always have 0 initialized qualifiers
+  EXPECT_FALSE(FPT->isConst());
+  EXPECT_FALSE(FPT->isVolatile());
+  EXPECT_FALSE(FPT->isRestrict());
+}
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -1647,6 +1647,7 @@
   /// Only common bits are stored here. Additional uncommon bits are stored
   /// in a trailing object after FunctionProtoType.
   class FunctionTypeBitfields {
+friend class FunctionNoProtoType;
 friend class FunctionProtoType;
 friend class FunctionType;
 
@@ -3942,7 +3943,9 @@
  Result->getDependence() &
  ~(TypeDependence::DependentInstantiation |
TypeDependence::UnexpandedPack),
- Info) {}
+ Info) {
+FunctionTypeBits.FastTypeQuals = 0;
+  }
 
 public:
   // No additional state past what FunctionType provides.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

[Richard Howell via Phabricator via cfe-commits]

rmaz added inline comments.



Comment at: clang/include/clang/AST/Type.h:3947
+ Info) {
+FunctionTypeBits.FastTypeQuals = 0;
+  }

aaron.ballman wrote:
> It seems a bit odd to me that we only want to initialize one member of the 
> bits and none of the rest.
The reason I only set the `FastTypeQuals` is because the rest of the 
`FunctionTypeBits` are not accessed in the base class or this class, except for 
`ExtInfo` which is initialized in the base class.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D133586#3809475 , @aaron.ballman 
wrote:

> This is why I'm wondering how we're hitting this problem in the first place. 
> C++ shouldn't be able to create a function without a prototype so why does 
> the ODR hash matter (do we use that in C and I just wasn't aware of it)?

The ODR hash matters because it is serialized in PCM output, regardless of 
which function type it is:

https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ODRHash.cpp#L889-L908

Leaving the `FastTypeQuals` uninitialized can lead to non-deterministic values 
of the ODR hash, and so the serialized module output. With enough leading 0s 
this can lead to a size change in the PCM file, which will start to fail builds 
due to a size mismatch during PCM validation.

Some alternative approaches could be:

- don't call these methods in `VisitFunctionNoProtoType` (and audit other 
callsites)
- remove these methods from `FunctionNoProtoType`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 462248.
rmaz added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

Files:
  clang/include/clang/AST/Type.h
  clang/unittests/AST/DeclTest.cpp


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -354,3 +354,24 @@
 
   EXPECT_TRUE(getFooValue->isInlined());
 }
+
+TEST(Decl, NoProtoFunctionDeclAttributes) {
+  llvm::Annotations Code(R"(
+void f();
+)");
+
+  auto AST = tooling::buildASTFromCodeWithArgs(
+  Code.code(),
+  /*Args=*/{"-target", "i386-apple-darwin", "-x", "objective-c"});
+  ASTContext  = AST->getASTContext();
+
+  auto *f = selectFirst(
+  "f", match(functionDecl(hasName("f")).bind("f"), Ctx));
+
+  const auto *FPT = f->getType()->getAs();
+
+  // Functions without prototypes always have 0 initialized qualifiers
+  EXPECT_FALSE(FPT->isConst());
+  EXPECT_FALSE(FPT->isVolatile());
+  EXPECT_FALSE(FPT->isRestrict());
+}
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -1647,6 +1647,7 @@
   /// Only common bits are stored here. Additional uncommon bits are stored
   /// in a trailing object after FunctionProtoType.
   class FunctionTypeBitfields {
+friend class FunctionNoProtoType;
 friend class FunctionProtoType;
 friend class FunctionType;
 
@@ -3942,7 +3943,9 @@
  Result->getDependence() &
  ~(TypeDependence::DependentInstantiation |
TypeDependence::UnexpandedPack),
- Info) {}
+ Info) {
+FunctionTypeBits.FastTypeQuals = 0;
+  }
 
 public:
   // No additional state past what FunctionType provides.


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -354,3 +354,24 @@
 
   EXPECT_TRUE(getFooValue->isInlined());
 }
+
+TEST(Decl, NoProtoFunctionDeclAttributes) {
+  llvm::Annotations Code(R"(
+void f();
+)");
+
+  auto AST = tooling::buildASTFromCodeWithArgs(
+  Code.code(),
+  /*Args=*/{"-target", "i386-apple-darwin", "-x", "objective-c"});
+  ASTContext  = AST->getASTContext();
+
+  auto *f = selectFirst(
+  "f", match(functionDecl(hasName("f")).bind("f"), Ctx));
+
+  const auto *FPT = f->getType()->getAs();
+
+  // Functions without prototypes always have 0 initialized qualifiers
+  EXPECT_FALSE(FPT->isConst());
+  EXPECT_FALSE(FPT->isVolatile());
+  EXPECT_FALSE(FPT->isRestrict());
+}
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -1647,6 +1647,7 @@
   /// Only common bits are stored here. Additional uncommon bits are stored
   /// in a trailing object after FunctionProtoType.
   class FunctionTypeBitfields {
+friend class FunctionNoProtoType;
 friend class FunctionProtoType;
 friend class FunctionType;
 
@@ -3942,7 +3943,9 @@
  Result->getDependence() &
  ~(TypeDependence::DependentInstantiation |
TypeDependence::UnexpandedPack),
- Info) {}
+ Info) {
+FunctionTypeBits.FastTypeQuals = 0;
+  }
 
 public:
   // No additional state past what FunctionType provides.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 461912.
rmaz added a comment.

Specify target and language for test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

Files:
  clang/include/clang/AST/Type.h
  clang/unittests/AST/DeclTest.cpp


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -311,3 +311,23 @@
   EXPECT_TRUE(bar->isInlined());
 }
 
+TEST(Decl, NoProtoFunctionDeclAttributes) {
+  llvm::Annotations Code(R"(
+void f();
+)");
+
+  auto AST = tooling::buildASTFromCodeWithArgs(
+  Code.code(),
+  /*Args=*/{"-target", "i386-apple-darwin", "-x", "objective-c"});
+  ASTContext  = AST->getASTContext();
+
+  auto *f = selectFirst(
+  "f", match(functionDecl(hasName("f")).bind("f"), Ctx));
+
+  const auto *FPT = f->getType()->getAs();
+
+  // Functions without prototypes always have 0 initialized qualifiers
+  EXPECT_FALSE(FPT->isConst());
+  EXPECT_FALSE(FPT->isVolatile());
+  EXPECT_FALSE(FPT->isRestrict());
+}
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -1647,6 +1647,7 @@
   /// Only common bits are stored here. Additional uncommon bits are stored
   /// in a trailing object after FunctionProtoType.
   class FunctionTypeBitfields {
+friend class FunctionNoProtoType;
 friend class FunctionProtoType;
 friend class FunctionType;
 
@@ -3926,7 +3927,9 @@
  Result->getDependence() &
  ~(TypeDependence::DependentInstantiation |
TypeDependence::UnexpandedPack),
- Info) {}
+ Info) {
+FunctionTypeBits.FastTypeQuals = 0;
+  }
 
 public:
   // No additional state past what FunctionType provides.


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -311,3 +311,23 @@
   EXPECT_TRUE(bar->isInlined());
 }
 
+TEST(Decl, NoProtoFunctionDeclAttributes) {
+  llvm::Annotations Code(R"(
+void f();
+)");
+
+  auto AST = tooling::buildASTFromCodeWithArgs(
+  Code.code(),
+  /*Args=*/{"-target", "i386-apple-darwin", "-x", "objective-c"});
+  ASTContext  = AST->getASTContext();
+
+  auto *f = selectFirst(
+  "f", match(functionDecl(hasName("f")).bind("f"), Ctx));
+
+  const auto *FPT = f->getType()->getAs();
+
+  // Functions without prototypes always have 0 initialized qualifiers
+  EXPECT_FALSE(FPT->isConst());
+  EXPECT_FALSE(FPT->isVolatile());
+  EXPECT_FALSE(FPT->isRestrict());
+}
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -1647,6 +1647,7 @@
   /// Only common bits are stored here. Additional uncommon bits are stored
   /// in a trailing object after FunctionProtoType.
   class FunctionTypeBitfields {
+friend class FunctionNoProtoType;
 friend class FunctionProtoType;
 friend class FunctionType;
 
@@ -3926,7 +3927,9 @@
  Result->getDependence() &
  ~(TypeDependence::DependentInstantiation |
TypeDependence::UnexpandedPack),
- Info) {}
+ Info) {
+FunctionTypeBits.FastTypeQuals = 0;
+  }
 
 public:
   // No additional state past what FunctionType provides.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 461689.
rmaz edited the summary of this revision.
rmaz added a comment.

Add unit test for type qualifiers


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

Files:
  clang/include/clang/AST/Type.h
  clang/unittests/AST/DeclTest.cpp


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -311,3 +311,21 @@
   EXPECT_TRUE(bar->isInlined());
 }
 
+TEST(Decl, NoProtoFunctionDeclAttributes) {
+  llvm::Annotations Code(R"(
+void f();
+)");
+
+  auto AST = tooling::buildASTFromCodeWithArgs(Code.code(), /*Args=*/{});
+  ASTContext  = AST->getASTContext();
+
+  auto *f = selectFirst(
+  "f", match(functionDecl(hasName("f")).bind("f"), Ctx));
+
+  const auto *FPT = f->getType()->getAs();
+
+  // Functions without prototypes always have 0 initialized qualifiers
+  EXPECT_FALSE(FPT->isConst());
+  EXPECT_FALSE(FPT->isVolatile());
+  EXPECT_FALSE(FPT->isRestrict());
+}
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -1647,6 +1647,7 @@
   /// Only common bits are stored here. Additional uncommon bits are stored
   /// in a trailing object after FunctionProtoType.
   class FunctionTypeBitfields {
+friend class FunctionNoProtoType;
 friend class FunctionProtoType;
 friend class FunctionType;
 
@@ -3926,7 +3927,9 @@
  Result->getDependence() &
  ~(TypeDependence::DependentInstantiation |
TypeDependence::UnexpandedPack),
- Info) {}
+ Info) {
+FunctionTypeBits.FastTypeQuals = 0;
+  }
 
 public:
   // No additional state past what FunctionType provides.


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -311,3 +311,21 @@
   EXPECT_TRUE(bar->isInlined());
 }
 
+TEST(Decl, NoProtoFunctionDeclAttributes) {
+  llvm::Annotations Code(R"(
+void f();
+)");
+
+  auto AST = tooling::buildASTFromCodeWithArgs(Code.code(), /*Args=*/{});
+  ASTContext  = AST->getASTContext();
+
+  auto *f = selectFirst(
+  "f", match(functionDecl(hasName("f")).bind("f"), Ctx));
+
+  const auto *FPT = f->getType()->getAs();
+
+  // Functions without prototypes always have 0 initialized qualifiers
+  EXPECT_FALSE(FPT->isConst());
+  EXPECT_FALSE(FPT->isVolatile());
+  EXPECT_FALSE(FPT->isRestrict());
+}
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -1647,6 +1647,7 @@
   /// Only common bits are stored here. Additional uncommon bits are stored
   /// in a trailing object after FunctionProtoType.
   class FunctionTypeBitfields {
+friend class FunctionNoProtoType;
 friend class FunctionProtoType;
 friend class FunctionType;
 
@@ -3926,7 +3927,9 @@
  Result->getDependence() &
  ~(TypeDependence::DependentInstantiation |
TypeDependence::UnexpandedPack),
- Info) {}
+ Info) {
+FunctionTypeBits.FastTypeQuals = 0;
+  }
 
 public:
   // No additional state past what FunctionType provides.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] initialize type qualifiers for FunctionNoProtoType

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D133586#3781536 , @rtrieu wrote:

> In D133586#3781468 , @rmaz wrote:
>
>> In D133586#3781381 , @rtrieu wrote:
>>
>>> Do you have a test case for this?
>>
>> Was struggling to think of a good one. How about a test that repeatedly 
>> generates a pcm for a func decl with no params and checks if the 
>> DECL_FUNCTION record is the same?
>
> Have you looked at clang/test/Modules/odr_hash.cpp?  It's where most of the 
> ODR hash testing takes place by testing that Decls can be merged properly 
> instead of checking the contents of pcm files..  Using `#if define`, it 
> creates multiple modules from the same file.  I would suggest creating two 
> functions in each of the modules, then in the main file, using the function 
> to force it to be loaded from the modules and merged together.  The test 
> should fail with the current Clang, but pass with your patch.  You may need 
> to create your test file if you need different compiler options.

I took a look at writing a test to cover this, but hit the following problem: 
function qualifiers are only valid on c++ members, and there we cannot create 
FunctionNoProtoTypes. I couldn't think of a way of testing this by comparing 
module output that would fail deterministically, as it relies on reusing 
uninitialized memory that is non-zero.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] do not hash undefined qualifiers for FunctionNoProtoType

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 459884.
rmaz added a comment.

zero out qual types in constructor


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

Files:
  clang/include/clang/AST/Type.h


Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -1647,6 +1647,7 @@
   /// Only common bits are stored here. Additional uncommon bits are stored
   /// in a trailing object after FunctionProtoType.
   class FunctionTypeBitfields {
+friend class FunctionNoProtoType;
 friend class FunctionProtoType;
 friend class FunctionType;
 
@@ -3926,7 +3927,9 @@
  Result->getDependence() &
  ~(TypeDependence::DependentInstantiation |
TypeDependence::UnexpandedPack),
- Info) {}
+ Info) {
+FunctionTypeBits.FastTypeQuals = 0;
+  }
 
 public:
   // No additional state past what FunctionType provides.


Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -1647,6 +1647,7 @@
   /// Only common bits are stored here. Additional uncommon bits are stored
   /// in a trailing object after FunctionProtoType.
   class FunctionTypeBitfields {
+friend class FunctionNoProtoType;
 friend class FunctionProtoType;
 friend class FunctionType;
 
@@ -3926,7 +3927,9 @@
  Result->getDependence() &
  ~(TypeDependence::DependentInstantiation |
TypeDependence::UnexpandedPack),
- Info) {}
+ Info) {
+FunctionTypeBits.FastTypeQuals = 0;
+  }
 
 public:
   // No additional state past what FunctionType provides.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] do not hash undefined qualifiers for FunctionNoProtoType

[Richard Howell via Phabricator via cfe-commits]

rmaz planned changes to this revision.
rmaz added a comment.

I think it is probably safer to zero out the FastTypeQuals instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133611: [clang] sort additional module maps when serializing

[Richard Howell via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3c1b42347b3a: [clang] sort additional module maps when 
serializing (authored by rmaz).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133611/new/

https://reviews.llvm.org/D133611

Files:
  clang/lib/Serialization/ASTWriter.cpp


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1282,7 +1282,12 @@
 if (auto *AdditionalModMaps =
 Map.getAdditionalModuleMapFiles(WritingModule)) {
   Record.push_back(AdditionalModMaps->size());
-  for (const FileEntry *F : *AdditionalModMaps)
+  SmallVector ModMaps(AdditionalModMaps->begin(),
+AdditionalModMaps->end());
+  llvm::sort(ModMaps, [](const FileEntry *A, const FileEntry *B) {
+return A->getName() < B->getName();
+  });
+  for (const FileEntry *F : ModMaps)
 AddPath(F->getName(), Record);
 } else {
   Record.push_back(0);


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1282,7 +1282,12 @@
 if (auto *AdditionalModMaps =
 Map.getAdditionalModuleMapFiles(WritingModule)) {
   Record.push_back(AdditionalModMaps->size());
-  for (const FileEntry *F : *AdditionalModMaps)
+  SmallVector ModMaps(AdditionalModMaps->begin(),
+AdditionalModMaps->end());
+  llvm::sort(ModMaps, [](const FileEntry *A, const FileEntry *B) {
+return A->getName() < B->getName();
+  });
+  for (const FileEntry *F : ModMaps)
 AddPath(F->getName(), Record);
 } else {
   Record.push_back(0);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133611: [clang] sort additional module maps when serializing

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 459492.
rmaz added a comment.

clang-format


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133611/new/

https://reviews.llvm.org/D133611

Files:
  clang/lib/Serialization/ASTWriter.cpp


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1282,7 +1282,12 @@
 if (auto *AdditionalModMaps =
 Map.getAdditionalModuleMapFiles(WritingModule)) {
   Record.push_back(AdditionalModMaps->size());
-  for (const FileEntry *F : *AdditionalModMaps)
+  SmallVector ModMaps(AdditionalModMaps->begin(),
+AdditionalModMaps->end());
+  llvm::sort(ModMaps, [](const FileEntry *A, const FileEntry *B) {
+return A->getName() < B->getName();
+  });
+  for (const FileEntry *F : ModMaps)
 AddPath(F->getName(), Record);
 } else {
   Record.push_back(0);


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1282,7 +1282,12 @@
 if (auto *AdditionalModMaps =
 Map.getAdditionalModuleMapFiles(WritingModule)) {
   Record.push_back(AdditionalModMaps->size());
-  for (const FileEntry *F : *AdditionalModMaps)
+  SmallVector ModMaps(AdditionalModMaps->begin(),
+AdditionalModMaps->end());
+  llvm::sort(ModMaps, [](const FileEntry *A, const FileEntry *B) {
+return A->getName() < B->getName();
+  });
+  for (const FileEntry *F : ModMaps)
 AddPath(F->getName(), Record);
 } else {
   Record.push_back(0);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133611: [clang] sort additional module maps when serializing

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 459467.
rmaz added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133611/new/

https://reviews.llvm.org/D133611

Files:
  clang/lib/Serialization/ASTWriter.cpp


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1282,7 +1282,11 @@
 if (auto *AdditionalModMaps =
 Map.getAdditionalModuleMapFiles(WritingModule)) {
   Record.push_back(AdditionalModMaps->size());
-  for (const FileEntry *F : *AdditionalModMaps)
+  SmallVector ModMaps(AdditionalModMaps->begin(), 
AdditionalModMaps->end());
+  llvm::sort(ModMaps, [](const FileEntry *A, const FileEntry *B) {
+return A->getName() < B->getName();
+  });
+  for (const FileEntry *F : ModMaps)
 AddPath(F->getName(), Record);
 } else {
   Record.push_back(0);


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1282,7 +1282,11 @@
 if (auto *AdditionalModMaps =
 Map.getAdditionalModuleMapFiles(WritingModule)) {
   Record.push_back(AdditionalModMaps->size());
-  for (const FileEntry *F : *AdditionalModMaps)
+  SmallVector ModMaps(AdditionalModMaps->begin(), AdditionalModMaps->end());
+  llvm::sort(ModMaps, [](const FileEntry *A, const FileEntry *B) {
+return A->getName() < B->getName();
+  });
+  for (const FileEntry *F : ModMaps)
 AddPath(F->getName(), Record);
 } else {
   Record.push_back(0);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] do not hash undefined qualifiers for FunctionNoProtoType

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D133586#3781381 , @rtrieu wrote:

> Do you have a test case for this?

Was struggling to think of a good one. How about a test that repeatedly 
generates a pcm for a func decl with no params and checks if the DECL_FUNCTION 
record is the same?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133611: [clang] sort additional module maps when serializing

[Richard Howell via Phabricator via cfe-commits]

rmaz created this revision.
Herald added a subscriber: mgrang.
Herald added a project: All.
rmaz requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Sort additional module maps when serializing pcm files. This ensures
the `MODULE_MAP_FILE` record is deterministic across repeated builds.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133611

Files:
  clang/lib/Serialization/ASTWriter.cpp


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1282,7 +1282,11 @@
 if (auto *AdditionalModMaps =
 Map.getAdditionalModuleMapFiles(WritingModule)) {
   Record.push_back(AdditionalModMaps->size());
-  for (const FileEntry *F : *AdditionalModMaps)
+  SmallVector ModMaps(AdditionalModMaps->begin(), 
AdditionalModMaps->end());
+  llvm::sort(ModMaps, [](const FileEntry *A, const FileEntry *B) {
+return A->getName() < B->getName();
+  });
+  for (const FileEntry *F : ModMaps)
 AddPath(F->getName(), Record);
 } else {
   Record.push_back(0);


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1282,7 +1282,11 @@
 if (auto *AdditionalModMaps =
 Map.getAdditionalModuleMapFiles(WritingModule)) {
   Record.push_back(AdditionalModMaps->size());
-  for (const FileEntry *F : *AdditionalModMaps)
+  SmallVector ModMaps(AdditionalModMaps->begin(), AdditionalModMaps->end());
+  llvm::sort(ModMaps, [](const FileEntry *A, const FileEntry *B) {
+return A->getName() < B->getName();
+  });
+  for (const FileEntry *F : ModMaps)
 AddPath(F->getName(), Record);
 } else {
   Record.push_back(0);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] do not hash undefined qualifiers for FunctionNoProtoType

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 459108.
rmaz added a comment.

Move code to VisitFunctionProtoType instead of branch


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133586/new/

https://reviews.llvm.org/D133586

Files:
  clang/lib/AST/ODRHash.cpp


Index: clang/lib/AST/ODRHash.cpp
===
--- clang/lib/AST/ODRHash.cpp
+++ clang/lib/AST/ODRHash.cpp
@@ -889,9 +889,6 @@
   void VisitFunctionType(const FunctionType *T) {
 AddQualType(T->getReturnType());
 T->getExtInfo().Profile(ID);
-Hash.AddBoolean(T->isConst());
-Hash.AddBoolean(T->isVolatile());
-Hash.AddBoolean(T->isRestrict());
 VisitType(T);
   }
 
@@ -904,6 +901,9 @@
 for (auto ParamType : T->getParamTypes())
   AddQualType(ParamType);
 
+Hash.AddBoolean(T->isConst());
+Hash.AddBoolean(T->isVolatile());
+Hash.AddBoolean(T->isRestrict());
 VisitFunctionType(T);
   }
 


Index: clang/lib/AST/ODRHash.cpp
===
--- clang/lib/AST/ODRHash.cpp
+++ clang/lib/AST/ODRHash.cpp
@@ -889,9 +889,6 @@
   void VisitFunctionType(const FunctionType *T) {
 AddQualType(T->getReturnType());
 T->getExtInfo().Profile(ID);
-Hash.AddBoolean(T->isConst());
-Hash.AddBoolean(T->isVolatile());
-Hash.AddBoolean(T->isRestrict());
 VisitType(T);
   }
 
@@ -904,6 +901,9 @@
 for (auto ParamType : T->getParamTypes())
   AddQualType(ParamType);
 
+Hash.AddBoolean(T->isConst());
+Hash.AddBoolean(T->isVolatile());
+Hash.AddBoolean(T->isRestrict());
 VisitFunctionType(T);
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133586: [clang] do not hash undefined qualifiers for FunctionNoProtoType

[Richard Howell via Phabricator via cfe-commits]

rmaz created this revision.
Herald added a project: All.
rmaz requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

When calculating the ODR hash of a `FunctionNoProtoType` do not
include qualifiers derived from `FastTypeQuals`. These are only
defined in the constructor for `FunctionProtoType`.

This change ensures the ODR hash and PCM output is stable for
and modules containing `FunctionNoProtoType`s.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133586

Files:
  clang/lib/AST/ODRHash.cpp


Index: clang/lib/AST/ODRHash.cpp
===
--- clang/lib/AST/ODRHash.cpp
+++ clang/lib/AST/ODRHash.cpp
@@ -889,9 +889,12 @@
   void VisitFunctionType(const FunctionType *T) {
 AddQualType(T->getReturnType());
 T->getExtInfo().Profile(ID);
-Hash.AddBoolean(T->isConst());
-Hash.AddBoolean(T->isVolatile());
-Hash.AddBoolean(T->isRestrict());
+if (T->isFunctionProtoType()) {
+  // These values are undefined for FunctionNoProtoType
+  Hash.AddBoolean(T->isConst());
+  Hash.AddBoolean(T->isVolatile());
+  Hash.AddBoolean(T->isRestrict());
+}
 VisitType(T);
   }
 


Index: clang/lib/AST/ODRHash.cpp
===
--- clang/lib/AST/ODRHash.cpp
+++ clang/lib/AST/ODRHash.cpp
@@ -889,9 +889,12 @@
   void VisitFunctionType(const FunctionType *T) {
 AddQualType(T->getReturnType());
 T->getExtInfo().Profile(ID);
-Hash.AddBoolean(T->isConst());
-Hash.AddBoolean(T->isVolatile());
-Hash.AddBoolean(T->isRestrict());
+if (T->isFunctionProtoType()) {
+  // These values are undefined for FunctionNoProtoType
+  Hash.AddBoolean(T->isConst());
+  Hash.AddBoolean(T->isVolatile());
+  Hash.AddBoolean(T->isRestrict());
+}
 VisitType(T);
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130710: [ASTWriter] Provide capability to output a PCM/PCH file that does not write out information about its output path

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D130710#3688338 , @akyrtzi wrote:

> In D130710#3685436 , @benlangmuir 
> wrote:
>
>> Is the functionality provided by `ORIGINAL_PCH_DIR` still useful for making 
>> it possible to move a PCH and its referenced headers?  It's not completely 
>> clear to me when this feature is used in practice.  It would be nice to 
>> remove it or change the default behaviour if possible, rather than require a 
>> new option, but I'm open to this approach if we think we can't change the 
>> default.
>
> Given that there was a recent change  
> related to `ORIGINAL_PCH_DIR`, I'm reluctant to change the default at this 
> point, I added a `FIXME` for following up to see if we can remove it or 
> change the default later on.

We don't use this functionality, no. My change was made for the same reason: to 
be able to relocate and cache pcm files when used in conjunction with 
`-fmodule-file-home-is-cwd`, removing the field entirely would be preferable 
from our perspective.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130710/new/

https://reviews.llvm.org/D130710

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124946: [clang] serialize ORIGINAL_PCH_DIR relative to BaseDirectory

[Richard Howell via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGee51e9795a31: [clang] serialize ORIGINAL_PCH_DIR relative to 
BaseDirectory (authored by rmaz).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124946/new/

https://reviews.llvm.org/D124946

Files:
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/relative-original-dir.m


Index: clang/test/Modules/relative-original-dir.m
===
--- /dev/null
+++ clang/test/Modules/relative-original-dir.m
@@ -0,0 +1,7 @@
+// RUN: rm -rf %t/normal-module-map
+// RUN: mkdir -p %t
+// RUN: cp -r %S/Inputs/normal-module-map %t
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -fmodule-name=libA 
-emit-module %t/normal-module-map/module.map -o 
%t/normal-module-map/outdir/mod.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram 
%t/normal-module-map/outdir/mod.pcm | FileCheck %s
+
+// CHECK:  blob data = 'outdir'
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1470,8 +1470,7 @@
 unsigned AbbrevCode = Stream.EmitAbbrev(std::move(Abbrev));
 
 SmallString<128> OutputPath(OutputFile);
-
-SM.getFileManager().makeAbsolutePath(OutputPath);
+PreparePathForOutput(OutputPath);
 StringRef origDir = llvm::sys::path::parent_path(OutputPath);
 
 RecordData::value_type Record[] = {ORIGINAL_PCH_DIR};
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -2898,6 +2898,7 @@
 
 case ORIGINAL_PCH_DIR:
   F.OriginalDir = std::string(Blob);
+  ResolveImportedPath(F, F.OriginalDir);
   break;
 
 case MODULE_NAME:


Index: clang/test/Modules/relative-original-dir.m
===
--- /dev/null
+++ clang/test/Modules/relative-original-dir.m
@@ -0,0 +1,7 @@
+// RUN: rm -rf %t/normal-module-map
+// RUN: mkdir -p %t
+// RUN: cp -r %S/Inputs/normal-module-map %t
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -fmodule-name=libA -emit-module %t/normal-module-map/module.map -o %t/normal-module-map/outdir/mod.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/normal-module-map/outdir/mod.pcm | FileCheck %s
+
+// CHECK:  blob data = 'outdir'
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1470,8 +1470,7 @@
 unsigned AbbrevCode = Stream.EmitAbbrev(std::move(Abbrev));
 
 SmallString<128> OutputPath(OutputFile);
-
-SM.getFileManager().makeAbsolutePath(OutputPath);
+PreparePathForOutput(OutputPath);
 StringRef origDir = llvm::sys::path::parent_path(OutputPath);
 
 RecordData::value_type Record[] = {ORIGINAL_PCH_DIR};
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -2898,6 +2898,7 @@
 
 case ORIGINAL_PCH_DIR:
   F.OriginalDir = std::string(Blob);
+  ResolveImportedPath(F, F.OriginalDir);
   break;
 
 case MODULE_NAME:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124938: [clang] serialize SUBMODULE_TOPHEADER relative to BaseDirectory

[Richard Howell via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf11056943e56: [clang] serialize SUBMODULE_TOPHEADER relative 
to BaseDirectory (authored by rmaz).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124938/new/

https://reviews.llvm.org/D124938

Files:
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/relative-submodule-topheader.m


Index: clang/test/Modules/relative-submodule-topheader.m
===
--- /dev/null
+++ clang/test/Modules/relative-submodule-topheader.m
@@ -0,0 +1,10 @@
+// RUN: cd %S
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -x objective-c++ 
-fmodule-name=std -emit-module Inputs/submodules/module.map -o %t/mod.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s
+
+// CHECK:  blob data = 'vector.h'
+// CHECK:  blob data = 'vector.h'
+// CHECK:  blob data = 'type_traits.h'
+// CHECK:  blob data = 'type_traits.h'
+// CHECK:  blob data = 'hash_map.h'
+// CHECK:  blob data = 'hash_map.h'
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2857,8 +2857,11 @@
 {
   auto TopHeaders = Mod->getTopHeaders(PP->getFileManager());
   RecordData::value_type Record[] = {SUBMODULE_TOPHEADER};
-  for (auto *H : TopHeaders)
-Stream.EmitRecordWithBlob(TopHeaderAbbrev, Record, H->getName());
+  for (auto *H : TopHeaders) {
+SmallString<128> HeaderName(H->getName());
+PreparePathForOutput(HeaderName);
+Stream.EmitRecordWithBlob(TopHeaderAbbrev, Record, HeaderName);
+  }
 }
 
 // Emit the imports.
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -5637,9 +5637,12 @@
   // them here.
   break;
 
-case SUBMODULE_TOPHEADER:
-  CurrentModule->addTopHeaderFilename(Blob);
+case SUBMODULE_TOPHEADER: {
+  std::string HeaderName(Blob);
+  ResolveImportedPath(F, HeaderName);
+  CurrentModule->addTopHeaderFilename(HeaderName);
   break;
+}
 
 case SUBMODULE_UMBRELLA_DIR: {
   // See comments in SUBMODULE_UMBRELLA_HEADER


Index: clang/test/Modules/relative-submodule-topheader.m
===
--- /dev/null
+++ clang/test/Modules/relative-submodule-topheader.m
@@ -0,0 +1,10 @@
+// RUN: cd %S
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -x objective-c++ -fmodule-name=std -emit-module Inputs/submodules/module.map -o %t/mod.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s
+
+// CHECK:  blob data = 'vector.h'
+// CHECK:  blob data = 'vector.h'
+// CHECK:  blob data = 'type_traits.h'
+// CHECK:  blob data = 'type_traits.h'
+// CHECK:  blob data = 'hash_map.h'
+// CHECK:  blob data = 'hash_map.h'
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2857,8 +2857,11 @@
 {
   auto TopHeaders = Mod->getTopHeaders(PP->getFileManager());
   RecordData::value_type Record[] = {SUBMODULE_TOPHEADER};
-  for (auto *H : TopHeaders)
-Stream.EmitRecordWithBlob(TopHeaderAbbrev, Record, H->getName());
+  for (auto *H : TopHeaders) {
+SmallString<128> HeaderName(H->getName());
+PreparePathForOutput(HeaderName);
+Stream.EmitRecordWithBlob(TopHeaderAbbrev, Record, HeaderName);
+  }
 }
 
 // Emit the imports.
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -5637,9 +5637,12 @@
   // them here.
   break;
 
-case SUBMODULE_TOPHEADER:
-  CurrentModule->addTopHeaderFilename(Blob);
+case SUBMODULE_TOPHEADER: {
+  std::string HeaderName(Blob);
+  ResolveImportedPath(F, HeaderName);
+  CurrentModule->addTopHeaderFilename(HeaderName);
   break;
+}
 
 case SUBMODULE_UMBRELLA_DIR: {
   // See comments in SUBMODULE_UMBRELLA_HEADER
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124874: [clang] add -fmodule-file-home-is-cwd

[Richard Howell via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG646e502de0d8: [clang] add -fmodule-file-home-is-cwd 
(authored by rmaz).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124874/new/

https://reviews.llvm.org/D124874

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/module-file-home-is-cwd.m


Index: clang/test/Modules/module-file-home-is-cwd.m
===
--- /dev/null
+++ clang/test/Modules/module-file-home-is-cwd.m
@@ -0,0 +1,8 @@
+// RUN: cd %S
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -fmodule-file-home-is-cwd 
-fmodule-name=libA -emit-module Inputs/normal-module-map/module.map -o 
%t/mod.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s
+
+// CHECK:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}a1.h'
+// CHECK:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}a2.h'
+// CHECK:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}module.map'
+// CHECK-NOT: MODULE_DIRECTORY
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1224,15 +1224,24 @@
   }
 
   if (WritingModule && WritingModule->Directory) {
-SmallString<128> BaseDir(WritingModule->Directory->getName());
+SmallString<128> BaseDir;
+if (PP.getHeaderSearchInfo().getHeaderSearchOpts().ModuleFileHomeIsCwd) {
+  // Use the current working directory as the base path for all inputs.
+  auto *CWD =
+  Context.getSourceManager().getFileManager().getDirectory(".").get();
+  BaseDir.assign(CWD->getName());
+} else {
+  BaseDir.assign(WritingModule->Directory->getName());
+}
 cleanPathForOutput(Context.getSourceManager().getFileManager(), BaseDir);
 
 // If the home of the module is the current working directory, then we
 // want to pick up the cwd of the build process loading the module, not
 // our cwd, when we load this module.
-if (!PP.getHeaderSearchInfo()
- .getHeaderSearchOpts()
- .ModuleMapFileHomeIsCwd ||
+if (!(PP.getHeaderSearchInfo()
+  .getHeaderSearchOpts()
+  .ModuleMapFileHomeIsCwd ||
+  PP.getHeaderSearchInfo().getHeaderSearchOpts().ModuleFileHomeIsCwd) 
||
 WritingModule->Directory->getName() != StringRef(".")) {
   // Module directory.
   auto Abbrev = std::make_shared();
Index: clang/include/clang/Lex/HeaderSearchOptions.h
===
--- clang/include/clang/Lex/HeaderSearchOptions.h
+++ clang/include/clang/Lex/HeaderSearchOptions.h
@@ -143,6 +143,12 @@
   /// file.
   unsigned ModuleMapFileHomeIsCwd : 1;
 
+  /// Set the base path of a built module file to be the current working
+  /// directory. This is useful for sharing module files across machines
+  /// that build with different paths without having to rewrite all
+  /// modulemap files to have working directory relative paths.
+  unsigned ModuleFileHomeIsCwd : 1;
+
   /// Also search for prebuilt implicit modules in the prebuilt module cache
   /// path.
   unsigned EnablePrebuiltImplicitModules : 1;
@@ -222,9 +228,9 @@
   HeaderSearchOptions(StringRef _Sysroot = "/")
   : Sysroot(_Sysroot), ModuleFormat("raw"), DisableModuleHash(false),
 ImplicitModuleMaps(false), ModuleMapFileHomeIsCwd(false),
-EnablePrebuiltImplicitModules(false), UseBuiltinIncludes(true),
-UseStandardSystemIncludes(true), UseStandardCXXIncludes(true),
-UseLibcxx(false), Verbose(false),
+ModuleFileHomeIsCwd(false), EnablePrebuiltImplicitModules(false),
+UseBuiltinIncludes(true), UseStandardSystemIncludes(true),
+UseStandardCXXIncludes(true), UseLibcxx(false), Verbose(false),
 ModulesValidateOncePerBuildSession(false),
 ModulesValidateSystemHeaders(false),
 ValidateASTInputFilesContent(false), UseDebugInfo(false),
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -5619,6 +5619,10 @@
   HelpText<"Use the current working directory as the home directory of "
"module maps specified by -fmodule-map-file=">,
   MarshallingInfoFlag>;
+def fmodule_file_home_is_cwd : Flag<["-"], "fmodule-file-home-is-cwd">,
+  HelpText<"Use the current working directory as the base directory of "
+   "compiled module files.">,
+  MarshallingInfoFlag>;
 def fmodule_feature : Separate<["-"], "fmodule-feature">,
   MetaVarName<"">,
   HelpText<"Enable  in module map requires declarations">,


Index: clang/test/Modules/module-file-home-is-cwd.m

[PATCH] D124946: [clang] serialize ORIGINAL_PCH_DIR relative to BaseDirectory

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D124946#3500454 , @urnathan wrote:

> Looks good to me, but perhaps leave it a few days for others to comment (my 
> familiarity with this code is low).  I do know people want relocatable 
> outputs though.

Are we good to go with this one too now?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124946/new/

https://reviews.llvm.org/D124946

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124874: [clang] add -fmodule-file-home-is-cwd

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 428483.
rmaz added a comment.

Refactor branch into existing case


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124874/new/

https://reviews.llvm.org/D124874

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/module-file-home-is-cwd.m


Index: clang/test/Modules/module-file-home-is-cwd.m
===
--- /dev/null
+++ clang/test/Modules/module-file-home-is-cwd.m
@@ -0,0 +1,8 @@
+// RUN: cd %S
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -fmodule-file-home-is-cwd 
-fmodule-name=libA -emit-module Inputs/normal-module-map/module.map -o 
%t/mod.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s
+
+// CHECK:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}a1.h'
+// CHECK:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}a2.h'
+// CHECK:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}module.map'
+// CHECK-NOT: MODULE_DIRECTORY
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1224,15 +1224,24 @@
   }
 
   if (WritingModule && WritingModule->Directory) {
-SmallString<128> BaseDir(WritingModule->Directory->getName());
+SmallString<128> BaseDir;
+if (PP.getHeaderSearchInfo().getHeaderSearchOpts().ModuleFileHomeIsCwd) {
+  // Use the current working directory as the base path for all inputs.
+  auto *CWD =
+  Context.getSourceManager().getFileManager().getDirectory(".").get();
+  BaseDir.assign(CWD->getName());
+} else {
+  BaseDir.assign(WritingModule->Directory->getName());
+}
 cleanPathForOutput(Context.getSourceManager().getFileManager(), BaseDir);
 
 // If the home of the module is the current working directory, then we
 // want to pick up the cwd of the build process loading the module, not
 // our cwd, when we load this module.
-if (!PP.getHeaderSearchInfo()
- .getHeaderSearchOpts()
- .ModuleMapFileHomeIsCwd ||
+if (!(PP.getHeaderSearchInfo()
+  .getHeaderSearchOpts()
+  .ModuleMapFileHomeIsCwd ||
+  PP.getHeaderSearchInfo().getHeaderSearchOpts().ModuleFileHomeIsCwd) 
||
 WritingModule->Directory->getName() != StringRef(".")) {
   // Module directory.
   auto Abbrev = std::make_shared();
Index: clang/include/clang/Lex/HeaderSearchOptions.h
===
--- clang/include/clang/Lex/HeaderSearchOptions.h
+++ clang/include/clang/Lex/HeaderSearchOptions.h
@@ -143,6 +143,12 @@
   /// file.
   unsigned ModuleMapFileHomeIsCwd : 1;
 
+  /// Set the base path of a built module file to be the current working
+  /// directory. This is useful for sharing module files across machines
+  /// that build with different paths without having to rewrite all
+  /// modulemap files to have working directory relative paths.
+  unsigned ModuleFileHomeIsCwd : 1;
+
   /// Also search for prebuilt implicit modules in the prebuilt module cache
   /// path.
   unsigned EnablePrebuiltImplicitModules : 1;
@@ -222,9 +228,9 @@
   HeaderSearchOptions(StringRef _Sysroot = "/")
   : Sysroot(_Sysroot), ModuleFormat("raw"), DisableModuleHash(false),
 ImplicitModuleMaps(false), ModuleMapFileHomeIsCwd(false),
-EnablePrebuiltImplicitModules(false), UseBuiltinIncludes(true),
-UseStandardSystemIncludes(true), UseStandardCXXIncludes(true),
-UseLibcxx(false), Verbose(false),
+ModuleFileHomeIsCwd(false), EnablePrebuiltImplicitModules(false),
+UseBuiltinIncludes(true), UseStandardSystemIncludes(true),
+UseStandardCXXIncludes(true), UseLibcxx(false), Verbose(false),
 ModulesValidateOncePerBuildSession(false),
 ModulesValidateSystemHeaders(false),
 ValidateASTInputFilesContent(false), UseDebugInfo(false),
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -5609,6 +5609,10 @@
   HelpText<"Use the current working directory as the home directory of "
"module maps specified by -fmodule-map-file=">,
   MarshallingInfoFlag>;
+def fmodule_file_home_is_cwd : Flag<["-"], "fmodule-file-home-is-cwd">,
+  HelpText<"Use the current working directory as the base directory of "
+   "compiled module files.">,
+  MarshallingInfoFlag>;
 def fmodule_feature : Separate<["-"], "fmodule-feature">,
   MetaVarName<"">,
   HelpText<"Enable  in module map requires declarations">,


Index: clang/test/Modules/module-file-home-is-cwd.m
===

[PATCH] D124874: [clang] add -fmodule-file-home-is-cwd

[Richard Howell via Phabricator via cfe-commits]

rmaz added inline comments.



Comment at: clang/lib/Serialization/ASTWriter.cpp:1231-1236
+SmallString<128> BaseDir(CWD->getName());
+cleanPathForOutput(Context.getSourceManager().getFileManager(), BaseDir);
+BaseDirectory.assign(BaseDir.begin(), BaseDir.end());
+  } else if (WritingModule && WritingModule->Directory) {
 SmallString<128> BaseDir(WritingModule->Directory->getName());
 cleanPathForOutput(Context.getSourceManager().getFileManager(), BaseDir);

urnathan wrote:
> This looks unfortunately duplicatey.  Any less-worse way of avoiding that?
It could be moved into the `else` clause at the cost of complicating the `if` 
statement with another case and adding another nested `if`. I thought it was 
simpler like this, but can change if you prefer.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124874/new/

https://reviews.llvm.org/D124874

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124946: [clang] serialize ORIGINAL_PCH_DIR relative to BaseDirectory

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 427731.
rmaz added a comment.

fix for missing temp dir in test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124946/new/

https://reviews.llvm.org/D124946

Files:
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/relative-original-dir.m


Index: clang/test/Modules/relative-original-dir.m
===
--- /dev/null
+++ clang/test/Modules/relative-original-dir.m
@@ -0,0 +1,7 @@
+// RUN: rm -rf %t/normal-module-map
+// RUN: mkdir -p %t
+// RUN: cp -r %S/Inputs/normal-module-map %t
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -fmodule-name=libA 
-emit-module %t/normal-module-map/module.map -o 
%t/normal-module-map/outdir/mod.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram 
%t/normal-module-map/outdir/mod.pcm | FileCheck %s
+
+// CHECK:  blob data = 'outdir'
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1461,8 +1461,7 @@
 unsigned AbbrevCode = Stream.EmitAbbrev(std::move(Abbrev));
 
 SmallString<128> OutputPath(OutputFile);
-
-SM.getFileManager().makeAbsolutePath(OutputPath);
+PreparePathForOutput(OutputPath);
 StringRef origDir = llvm::sys::path::parent_path(OutputPath);
 
 RecordData::value_type Record[] = {ORIGINAL_PCH_DIR};
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -2898,6 +2898,7 @@
 
 case ORIGINAL_PCH_DIR:
   F.OriginalDir = std::string(Blob);
+  ResolveImportedPath(F, F.OriginalDir);
   break;
 
 case MODULE_NAME:


Index: clang/test/Modules/relative-original-dir.m
===
--- /dev/null
+++ clang/test/Modules/relative-original-dir.m
@@ -0,0 +1,7 @@
+// RUN: rm -rf %t/normal-module-map
+// RUN: mkdir -p %t
+// RUN: cp -r %S/Inputs/normal-module-map %t
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -fmodule-name=libA -emit-module %t/normal-module-map/module.map -o %t/normal-module-map/outdir/mod.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/normal-module-map/outdir/mod.pcm | FileCheck %s
+
+// CHECK:  blob data = 'outdir'
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1461,8 +1461,7 @@
 unsigned AbbrevCode = Stream.EmitAbbrev(std::move(Abbrev));
 
 SmallString<128> OutputPath(OutputFile);
-
-SM.getFileManager().makeAbsolutePath(OutputPath);
+PreparePathForOutput(OutputPath);
 StringRef origDir = llvm::sys::path::parent_path(OutputPath);
 
 RecordData::value_type Record[] = {ORIGINAL_PCH_DIR};
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -2898,6 +2898,7 @@
 
 case ORIGINAL_PCH_DIR:
   F.OriginalDir = std::string(Blob);
+  ResolveImportedPath(F, F.OriginalDir);
   break;
 
 case MODULE_NAME:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124874: [clang] add -fmodule-file-home-is-cwd

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 427065.
rmaz added a comment.

fix windows paths attempt #2


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124874/new/

https://reviews.llvm.org/D124874

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/module-file-home-is-cwd.m


Index: clang/test/Modules/module-file-home-is-cwd.m
===
--- /dev/null
+++ clang/test/Modules/module-file-home-is-cwd.m
@@ -0,0 +1,8 @@
+// RUN: cd %S
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -fmodule-file-home-is-cwd 
-fmodule-name=libA -emit-module Inputs/normal-module-map/module.map -o 
%t/mod.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s
+
+// CHECK:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}a1.h'
+// CHECK:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}a2.h'
+// CHECK:  blob data = 
'Inputs{{/|\\}}normal-module-map{{/|\\}}module.map'
+// CHECK-NOT: MODULE_DIRECTORY
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1223,7 +1223,15 @@
 Stream.EmitRecordWithBlob(AbbrevCode, Record, WritingModule->Name);
   }
 
-  if (WritingModule && WritingModule->Directory) {
+  if (WritingModule &&
+  PP.getHeaderSearchInfo().getHeaderSearchOpts().ModuleFileHomeIsCwd) {
+// Use the current working directory as the base path for all inputs.
+auto *CWD =
+Context.getSourceManager().getFileManager().getDirectory(".").get();
+SmallString<128> BaseDir(CWD->getName());
+cleanPathForOutput(Context.getSourceManager().getFileManager(), BaseDir);
+BaseDirectory.assign(BaseDir.begin(), BaseDir.end());
+  } else if (WritingModule && WritingModule->Directory) {
 SmallString<128> BaseDir(WritingModule->Directory->getName());
 cleanPathForOutput(Context.getSourceManager().getFileManager(), BaseDir);
 
Index: clang/include/clang/Lex/HeaderSearchOptions.h
===
--- clang/include/clang/Lex/HeaderSearchOptions.h
+++ clang/include/clang/Lex/HeaderSearchOptions.h
@@ -143,6 +143,12 @@
   /// file.
   unsigned ModuleMapFileHomeIsCwd : 1;
 
+  /// Set the base path of a built module file to be the current working
+  /// directory. This is useful for sharing module files across machines
+  /// that build with different paths without having to rewrite all
+  /// modulemap files to have working directory relative paths.
+  unsigned ModuleFileHomeIsCwd : 1;
+
   /// Also search for prebuilt implicit modules in the prebuilt module cache
   /// path.
   unsigned EnablePrebuiltImplicitModules : 1;
@@ -222,9 +228,9 @@
   HeaderSearchOptions(StringRef _Sysroot = "/")
   : Sysroot(_Sysroot), ModuleFormat("raw"), DisableModuleHash(false),
 ImplicitModuleMaps(false), ModuleMapFileHomeIsCwd(false),
-EnablePrebuiltImplicitModules(false), UseBuiltinIncludes(true),
-UseStandardSystemIncludes(true), UseStandardCXXIncludes(true),
-UseLibcxx(false), Verbose(false),
+ModuleFileHomeIsCwd(false), EnablePrebuiltImplicitModules(false),
+UseBuiltinIncludes(true), UseStandardSystemIncludes(true),
+UseStandardCXXIncludes(true), UseLibcxx(false), Verbose(false),
 ModulesValidateOncePerBuildSession(false),
 ModulesValidateSystemHeaders(false),
 ValidateASTInputFilesContent(false), UseDebugInfo(false),
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -5609,6 +5609,10 @@
   HelpText<"Use the current working directory as the home directory of "
"module maps specified by -fmodule-map-file=">,
   MarshallingInfoFlag>;
+def fmodule_file_home_is_cwd : Flag<["-"], "fmodule-file-home-is-cwd">,
+  HelpText<"Use the current working directory as the base directory of "
+   "compiled module files.">,
+  MarshallingInfoFlag>;
 def fmodule_feature : Separate<["-"], "fmodule-feature">,
   MetaVarName<"">,
   HelpText<"Enable  in module map requires declarations">,


Index: clang/test/Modules/module-file-home-is-cwd.m
===
--- /dev/null
+++ clang/test/Modules/module-file-home-is-cwd.m
@@ -0,0 +1,8 @@
+// RUN: cd %S
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -fmodule-file-home-is-cwd -fmodule-name=libA -emit-module Inputs/normal-module-map/module.map -o %t/mod.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s
+
+// CHECK:  blob data = 'Inputs{{/|\\}}normal-module-map{{/|\\}}a1.h'
+// CHECK:  blob data = 

[PATCH] D124946: [clang] serialize ORIGINAL_PCH_DIR relative to BaseDirectory

[Richard Howell via Phabricator via cfe-commits]

rmaz created this revision.
Herald added a project: All.
rmaz requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This diff changes the serialization of the `ORIGINAL_PCH_DIR`
entry in module files to be serialized relative to the module's
`BaseDirectory`. This will allow for the module to be relocatable
across machines.

The path is restored relative to the module's BaseDirectory on
deserialization.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124946

Files:
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/relative-original-dir.m


Index: clang/test/Modules/relative-original-dir.m
===
--- /dev/null
+++ clang/test/Modules/relative-original-dir.m
@@ -0,0 +1,6 @@
+// RUN: rm -rf %t/normal-module-map
+// RUN: cp -R %S/Inputs/normal-module-map %t
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -fmodule-name=libA 
-emit-module %t/normal-module-map/module.map -o 
%t/normal-module-map/outdir/mod.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram 
%t/normal-module-map/outdir/mod.pcm | FileCheck %s
+
+// CHECK:  blob data = 'outdir'
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1461,8 +1461,7 @@
 unsigned AbbrevCode = Stream.EmitAbbrev(std::move(Abbrev));
 
 SmallString<128> OutputPath(OutputFile);
-
-SM.getFileManager().makeAbsolutePath(OutputPath);
+PreparePathForOutput(OutputPath);
 StringRef origDir = llvm::sys::path::parent_path(OutputPath);
 
 RecordData::value_type Record[] = {ORIGINAL_PCH_DIR};
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -2898,6 +2898,7 @@
 
 case ORIGINAL_PCH_DIR:
   F.OriginalDir = std::string(Blob);
+  ResolveImportedPath(F, F.OriginalDir);
   break;
 
 case MODULE_NAME:


Index: clang/test/Modules/relative-original-dir.m
===
--- /dev/null
+++ clang/test/Modules/relative-original-dir.m
@@ -0,0 +1,6 @@
+// RUN: rm -rf %t/normal-module-map
+// RUN: cp -R %S/Inputs/normal-module-map %t
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -fmodule-name=libA -emit-module %t/normal-module-map/module.map -o %t/normal-module-map/outdir/mod.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/normal-module-map/outdir/mod.pcm | FileCheck %s
+
+// CHECK:  blob data = 'outdir'
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1461,8 +1461,7 @@
 unsigned AbbrevCode = Stream.EmitAbbrev(std::move(Abbrev));
 
 SmallString<128> OutputPath(OutputFile);
-
-SM.getFileManager().makeAbsolutePath(OutputPath);
+PreparePathForOutput(OutputPath);
 StringRef origDir = llvm::sys::path::parent_path(OutputPath);
 
 RecordData::value_type Record[] = {ORIGINAL_PCH_DIR};
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -2898,6 +2898,7 @@
 
 case ORIGINAL_PCH_DIR:
   F.OriginalDir = std::string(Blob);
+  ResolveImportedPath(F, F.OriginalDir);
   break;
 
 case MODULE_NAME:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124938: [clang] serialize SUBMODULE_TOPHEADER relative to BaseDirectory

[Richard Howell via Phabricator via cfe-commits]

rmaz created this revision.
Herald added a project: All.
rmaz requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This diff changes the serialization of the `SUBMODULE_TOPHEADER`
entry in module files to be serialized relative to the module's
`BaseDirectory`. This matches the behavior of the
`SUBMODULE_HEADER` entry and will allow for the module to be
relocatable across machines.

The path is restored relative to the module's `BaseDirectory` on
deserialization.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124938

Files:
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/relative-submodule-topheader.m


Index: clang/test/Modules/relative-submodule-topheader.m
===
--- /dev/null
+++ clang/test/Modules/relative-submodule-topheader.m
@@ -0,0 +1,10 @@
+// RUN: cd %S
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -x objective-c++ 
-fmodule-name=std -emit-module Inputs/submodules/module.map -o %t/mod.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s
+
+// CHECK:  blob data = 'vector.h'
+// CHECK:  blob data = 'vector.h'
+// CHECK:  blob data = 'type_traits.h'
+// CHECK:  blob data = 'type_traits.h'
+// CHECK:  blob data = 'hash_map.h'
+// CHECK:  blob data = 'hash_map.h'
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2848,8 +2848,11 @@
 {
   auto TopHeaders = Mod->getTopHeaders(PP->getFileManager());
   RecordData::value_type Record[] = {SUBMODULE_TOPHEADER};
-  for (auto *H : TopHeaders)
-Stream.EmitRecordWithBlob(TopHeaderAbbrev, Record, H->getName());
+  for (auto *H : TopHeaders) {
+SmallString<128> HeaderName(H->getName());
+PreparePathForOutput(HeaderName);
+Stream.EmitRecordWithBlob(TopHeaderAbbrev, Record, HeaderName);
+  }
 }
 
 // Emit the imports.
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -5637,9 +5637,12 @@
   // them here.
   break;
 
-case SUBMODULE_TOPHEADER:
-  CurrentModule->addTopHeaderFilename(Blob);
+case SUBMODULE_TOPHEADER: {
+  std::string HeaderName(Blob);
+  ResolveImportedPath(F, HeaderName);
+  CurrentModule->addTopHeaderFilename(HeaderName);
   break;
+}
 
 case SUBMODULE_UMBRELLA_DIR: {
   // See comments in SUBMODULE_UMBRELLA_HEADER


Index: clang/test/Modules/relative-submodule-topheader.m
===
--- /dev/null
+++ clang/test/Modules/relative-submodule-topheader.m
@@ -0,0 +1,10 @@
+// RUN: cd %S
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -x objective-c++ -fmodule-name=std -emit-module Inputs/submodules/module.map -o %t/mod.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s
+
+// CHECK:  blob data = 'vector.h'
+// CHECK:  blob data = 'vector.h'
+// CHECK:  blob data = 'type_traits.h'
+// CHECK:  blob data = 'type_traits.h'
+// CHECK:  blob data = 'hash_map.h'
+// CHECK:  blob data = 'hash_map.h'
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2848,8 +2848,11 @@
 {
   auto TopHeaders = Mod->getTopHeaders(PP->getFileManager());
   RecordData::value_type Record[] = {SUBMODULE_TOPHEADER};
-  for (auto *H : TopHeaders)
-Stream.EmitRecordWithBlob(TopHeaderAbbrev, Record, H->getName());
+  for (auto *H : TopHeaders) {
+SmallString<128> HeaderName(H->getName());
+PreparePathForOutput(HeaderName);
+Stream.EmitRecordWithBlob(TopHeaderAbbrev, Record, HeaderName);
+  }
 }
 
 // Emit the imports.
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -5637,9 +5637,12 @@
   // them here.
   break;
 
-case SUBMODULE_TOPHEADER:
-  CurrentModule->addTopHeaderFilename(Blob);
+case SUBMODULE_TOPHEADER: {
+  std::string HeaderName(Blob);
+  ResolveImportedPath(F, HeaderName);
+  CurrentModule->addTopHeaderFilename(HeaderName);
   break;
+}
 
 case SUBMODULE_UMBRELLA_DIR: {
   // See comments in SUBMODULE_UMBRELLA_HEADER
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124874: [clang] add -fmodule-file-home-is-cwd

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 427034.
rmaz added a comment.

Use regex for path separators in test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124874/new/

https://reviews.llvm.org/D124874

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/module-file-home-is-cwd.m


Index: clang/test/Modules/module-file-home-is-cwd.m
===
--- /dev/null
+++ clang/test/Modules/module-file-home-is-cwd.m
@@ -0,0 +1,8 @@
+// RUN: cd %S
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -fmodule-file-home-is-cwd 
-fmodule-name=libA -emit-module Inputs/normal-module-map/module.map -o 
%t/mod.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s
+
+// CHECK:  blob data = 
'Inputs{{/|}}normal-module-map{{/|}}a1.h'
+// CHECK:  blob data = 
'Inputs{{/|}}normal-module-map{{/|}}a2.h'
+// CHECK:  blob data = 
'Inputs{{/|}}normal-module-map{{/|}}module.map'
+// CHECK-NOT: MODULE_DIRECTORY
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1223,7 +1223,15 @@
 Stream.EmitRecordWithBlob(AbbrevCode, Record, WritingModule->Name);
   }
 
-  if (WritingModule && WritingModule->Directory) {
+  if (WritingModule &&
+  PP.getHeaderSearchInfo().getHeaderSearchOpts().ModuleFileHomeIsCwd) {
+// Use the current working directory as the base path for all inputs.
+auto *CWD =
+Context.getSourceManager().getFileManager().getDirectory(".").get();
+SmallString<128> BaseDir(CWD->getName());
+cleanPathForOutput(Context.getSourceManager().getFileManager(), BaseDir);
+BaseDirectory.assign(BaseDir.begin(), BaseDir.end());
+  } else if (WritingModule && WritingModule->Directory) {
 SmallString<128> BaseDir(WritingModule->Directory->getName());
 cleanPathForOutput(Context.getSourceManager().getFileManager(), BaseDir);
 
Index: clang/include/clang/Lex/HeaderSearchOptions.h
===
--- clang/include/clang/Lex/HeaderSearchOptions.h
+++ clang/include/clang/Lex/HeaderSearchOptions.h
@@ -143,6 +143,12 @@
   /// file.
   unsigned ModuleMapFileHomeIsCwd : 1;
 
+  /// Set the base path of a built module file to be the current working
+  /// directory. This is useful for sharing module files across machines
+  /// that build with different paths without having to rewrite all
+  /// modulemap files to have working directory relative paths.
+  unsigned ModuleFileHomeIsCwd : 1;
+
   /// Also search for prebuilt implicit modules in the prebuilt module cache
   /// path.
   unsigned EnablePrebuiltImplicitModules : 1;
@@ -222,9 +228,9 @@
   HeaderSearchOptions(StringRef _Sysroot = "/")
   : Sysroot(_Sysroot), ModuleFormat("raw"), DisableModuleHash(false),
 ImplicitModuleMaps(false), ModuleMapFileHomeIsCwd(false),
-EnablePrebuiltImplicitModules(false), UseBuiltinIncludes(true),
-UseStandardSystemIncludes(true), UseStandardCXXIncludes(true),
-UseLibcxx(false), Verbose(false),
+ModuleFileHomeIsCwd(false), EnablePrebuiltImplicitModules(false),
+UseBuiltinIncludes(true), UseStandardSystemIncludes(true),
+UseStandardCXXIncludes(true), UseLibcxx(false), Verbose(false),
 ModulesValidateOncePerBuildSession(false),
 ModulesValidateSystemHeaders(false),
 ValidateASTInputFilesContent(false), UseDebugInfo(false),
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -5609,6 +5609,10 @@
   HelpText<"Use the current working directory as the home directory of "
"module maps specified by -fmodule-map-file=">,
   MarshallingInfoFlag>;
+def fmodule_file_home_is_cwd : Flag<["-"], "fmodule-file-home-is-cwd">,
+  HelpText<"Use the current working directory as the base directory of "
+   "compiled module files.">,
+  MarshallingInfoFlag>;
 def fmodule_feature : Separate<["-"], "fmodule-feature">,
   MetaVarName<"">,
   HelpText<"Enable  in module map requires declarations">,


Index: clang/test/Modules/module-file-home-is-cwd.m
===
--- /dev/null
+++ clang/test/Modules/module-file-home-is-cwd.m
@@ -0,0 +1,8 @@
+// RUN: cd %S
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -fmodule-file-home-is-cwd -fmodule-name=libA -emit-module Inputs/normal-module-map/module.map -o %t/mod.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s
+
+// CHECK:  blob data = 'Inputs{{/|}}normal-module-map{{/|}}a1.h'
+// CHECK:  blob data = 

[PATCH] D124874: [clang] add -fmodule-file-home-is-cwd

[Richard Howell via Phabricator via cfe-commits]

rmaz created this revision.
Herald added a project: All.
rmaz requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This diff adds a new frontend flag `-fmodule-file-home-is-cwd`.
The behavior of this flag is similar to
`-fmodule-map-file-home-is-cwd` but does not require the module
map files to be modified to have inputs relative to the cwd.
Instead the output modules will have their `BaseDirectory` set
to the cwd and will try and resolve paths relative to that.

The motiviation for this change is to support relocatable pcm
files that are built on different machines with different paths
without having to alter module map files, which is sometimes not
possible as they are provided by 3rd parties.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124874

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/module-file-home-is-cwd.m


Index: clang/test/Modules/module-file-home-is-cwd.m
===
--- /dev/null
+++ clang/test/Modules/module-file-home-is-cwd.m
@@ -0,0 +1,8 @@
+// RUN: cd %S
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -fmodule-file-home-is-cwd 
-fmodule-name=libA -emit-module Inputs/normal-module-map/module.map -o 
%t/mod.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t/mod.pcm | FileCheck %s
+
+// CHECK:  blob data = 'Inputs/normal-module-map/a1.h'
+// CHECK:  blob data = 'Inputs/normal-module-map/a2.h'
+// CHECK:  blob data = 
'Inputs/normal-module-map/module.map'
+// CHECK-NOT: MODULE_DIRECTORY
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1223,7 +1223,15 @@
 Stream.EmitRecordWithBlob(AbbrevCode, Record, WritingModule->Name);
   }
 
-  if (WritingModule && WritingModule->Directory) {
+  if (WritingModule &&
+  PP.getHeaderSearchInfo().getHeaderSearchOpts().ModuleFileHomeIsCwd) {
+// Use the current working directory as the base path for all inputs.
+auto *CWD =
+Context.getSourceManager().getFileManager().getDirectory(".").get();
+SmallString<128> BaseDir(CWD->getName());
+cleanPathForOutput(Context.getSourceManager().getFileManager(), BaseDir);
+BaseDirectory.assign(BaseDir.begin(), BaseDir.end());
+  } else if (WritingModule && WritingModule->Directory) {
 SmallString<128> BaseDir(WritingModule->Directory->getName());
 cleanPathForOutput(Context.getSourceManager().getFileManager(), BaseDir);
 
Index: clang/include/clang/Lex/HeaderSearchOptions.h
===
--- clang/include/clang/Lex/HeaderSearchOptions.h
+++ clang/include/clang/Lex/HeaderSearchOptions.h
@@ -143,6 +143,12 @@
   /// file.
   unsigned ModuleMapFileHomeIsCwd : 1;
 
+  /// Set the base path of a built module file to be the current working
+  /// directory. This is useful for sharing module files across machines
+  /// that build with different paths without having to rewrite all
+  /// modulemap files to have working directory relative paths.
+  unsigned ModuleFileHomeIsCwd : 1;
+
   /// Also search for prebuilt implicit modules in the prebuilt module cache
   /// path.
   unsigned EnablePrebuiltImplicitModules : 1;
@@ -222,9 +228,9 @@
   HeaderSearchOptions(StringRef _Sysroot = "/")
   : Sysroot(_Sysroot), ModuleFormat("raw"), DisableModuleHash(false),
 ImplicitModuleMaps(false), ModuleMapFileHomeIsCwd(false),
-EnablePrebuiltImplicitModules(false), UseBuiltinIncludes(true),
-UseStandardSystemIncludes(true), UseStandardCXXIncludes(true),
-UseLibcxx(false), Verbose(false),
+ModuleFileHomeIsCwd(false), EnablePrebuiltImplicitModules(false),
+UseBuiltinIncludes(true), UseStandardSystemIncludes(true),
+UseStandardCXXIncludes(true), UseLibcxx(false), Verbose(false),
 ModulesValidateOncePerBuildSession(false),
 ModulesValidateSystemHeaders(false),
 ValidateASTInputFilesContent(false), UseDebugInfo(false),
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -5609,6 +5609,10 @@
   HelpText<"Use the current working directory as the home directory of "
"module maps specified by -fmodule-map-file=">,
   MarshallingInfoFlag>;
+def fmodule_file_home_is_cwd : Flag<["-"], "fmodule-file-home-is-cwd">,
+  HelpText<"Use the current working directory as the base directory of "
+   "compiled module files.">,
+  MarshallingInfoFlag>;
 def fmodule_feature : Separate<["-"], "fmodule-feature">,
   MetaVarName<"">,
   HelpText<"Enable  in module map requires declarations">,


Index: 

[PATCH] D116174: [clang] support relative roots to vfs overlays

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 400983.
rmaz added a comment.

Rebase on stable commit


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116174/new/

https://reviews.llvm.org/D116174

Files:
  clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
  clang/test/VFS/vfsoverlay-relative-root.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -2164,6 +2164,11 @@
 
 TEST_F(VFSFromYAMLTest, RelativePaths) {
   IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  std::error_code EC;
+  SmallString<128> CWD;
+  EC = llvm::sys::fs::current_path(CWD);
+  ASSERT_FALSE(EC);
+
   // Filename at root level without a parent directory.
   IntrusiveRefCntPtr FS = getFromYAMLString(
   "{ 'roots': [\n"
@@ -2172,16 +2177,26 @@
   "  }\n"
   "] }",
   Lower);
-  EXPECT_EQ(nullptr, FS.get());
+  ASSERT_TRUE(FS.get() != nullptr);
+  SmallString<128> ExpectedPathNotInDir("file-not-in-directory.h");
+  llvm::sys::fs::make_absolute(ExpectedPathNotInDir);
+  checkContents(FS->dir_begin(CWD, EC), {ExpectedPathNotInDir});
 
   // Relative file path.
   FS = getFromYAMLString("{ 'roots': [\n"
- "  { 'type': 'file', 'name': 'relative/file/path.h',\n"
+ "  { 'type': 'file', 'name': 'relative/path.h',\n"
  "'external-contents': '//root/external/file'\n"
  "  }\n"
  "] }",
  Lower);
-  EXPECT_EQ(nullptr, FS.get());
+  ASSERT_TRUE(FS.get() != nullptr);
+  SmallString<128> Parent("relative");
+  llvm::sys::fs::make_absolute(Parent);
+  auto I = FS->dir_begin(Parent, EC);
+  ASSERT_FALSE(EC);
+  // Convert to POSIX path for comparison of windows paths
+  ASSERT_EQ("relative/path.h",
+getPosixPath(std::string(I->path().substr(CWD.size() + 1;
 
   // Relative directory path.
   FS = getFromYAMLString(
@@ -2191,9 +2206,14 @@
   "  }\n"
   "] }",
   Lower);
-  EXPECT_EQ(nullptr, FS.get());
+  ASSERT_TRUE(FS.get() != nullptr);
+  SmallString<128> Root("relative/directory");
+  llvm::sys::fs::make_absolute(Root);
+  I = FS->dir_begin(Root, EC);
+  ASSERT_FALSE(EC);
+  ASSERT_EQ("path.h", std::string(I->path().substr(Root.size() + 1)));
 
-  EXPECT_EQ(3, NumDiagnostics);
+  EXPECT_EQ(0, NumDiagnostics);
 }
 
 TEST_F(VFSFromYAMLTest, NonFallthroughDirectoryIteration) {
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1649,10 +1649,19 @@
 sys::path::Style::windows_backslash)) {
 path_style = sys::path::Style::windows_backslash;
   } else {
-assert(NameValueNode && "Name presence should be checked earlier");
-error(NameValueNode,
+// Relative VFS root entries are made absolute to the current working
+// directory, then we can determine the path style from that.
+auto EC = sys::fs::make_absolute(Name);
+if (EC) {
+  assert(NameValueNode && "Name presence should be checked earlier");
+  error(
+  NameValueNode,
   "entry with relative path at the root level is not discoverable");
-return nullptr;
+  return nullptr;
+}
+path_style = sys::path::is_absolute(Name, sys::path::Style::posix)
+ ? sys::path::Style::posix
+ : sys::path::Style::windows_backslash;
   }
 }
 
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -547,6 +547,9 @@
 /// }
 /// \endverbatim
 ///
+/// The roots may be absolute or relative. If relative they will be made
+/// absolute against the current working directory.
+///
 /// All configuration options are optional.
 ///   'case-sensitive': 
 ///   'use-external-names': 
Index: clang/test/VFS/vfsoverlay-relative-root.c
===
--- /dev/null
+++ clang/test/VFS/vfsoverlay-relative-root.c
@@ -0,0 +1,3 @@
+// RUN: %clang_cc1 -Werror -ivfsoverlay %S/Inputs/vfsoverlay-root-relative.yaml -I virtual -fsyntax-only %s
+
+#include "virtual_header.h"
Index: clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
===
--- /dev/null
+++ clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
@@ -0,0 +1,17 @@
+{
+  'version': 0,
+  

[PATCH] D116174: [clang] support relative roots to vfs overlays

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 400894.
rmaz added a comment.

Windows test fixes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116174/new/

https://reviews.llvm.org/D116174

Files:
  clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
  clang/test/VFS/vfsoverlay-relative-root.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -2164,6 +2164,11 @@
 
 TEST_F(VFSFromYAMLTest, RelativePaths) {
   IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  std::error_code EC;
+  SmallString<128> CWD;
+  EC = llvm::sys::fs::current_path(CWD);
+  ASSERT_FALSE(EC);
+
   // Filename at root level without a parent directory.
   IntrusiveRefCntPtr FS = getFromYAMLString(
   "{ 'roots': [\n"
@@ -2172,16 +2177,26 @@
   "  }\n"
   "] }",
   Lower);
-  EXPECT_EQ(nullptr, FS.get());
+  ASSERT_TRUE(FS.get() != nullptr);
+  SmallString<128> ExpectedPathNotInDir("file-not-in-directory.h");
+  llvm::sys::fs::make_absolute(ExpectedPathNotInDir);
+  checkContents(FS->dir_begin(CWD, EC), {ExpectedPathNotInDir});
 
   // Relative file path.
   FS = getFromYAMLString("{ 'roots': [\n"
- "  { 'type': 'file', 'name': 'relative/file/path.h',\n"
+ "  { 'type': 'file', 'name': 'relative/path.h',\n"
  "'external-contents': '//root/external/file'\n"
  "  }\n"
  "] }",
  Lower);
-  EXPECT_EQ(nullptr, FS.get());
+  ASSERT_TRUE(FS.get() != nullptr);
+  SmallString<128> Parent("relative");
+  llvm::sys::fs::make_absolute(Parent);
+  auto I = FS->dir_begin(Parent, EC);
+  ASSERT_FALSE(EC);
+  // Convert to POSIX path for comparison of windows paths
+  ASSERT_EQ("relative/path.h",
+getPosixPath(std::string(I->path().substr(CWD.size() + 1;
 
   // Relative directory path.
   FS = getFromYAMLString(
@@ -2191,9 +2206,14 @@
   "  }\n"
   "] }",
   Lower);
-  EXPECT_EQ(nullptr, FS.get());
+  ASSERT_TRUE(FS.get() != nullptr);
+  SmallString<128> Root("relative/directory");
+  llvm::sys::fs::make_absolute(Root);
+  I = FS->dir_begin(Root, EC);
+  ASSERT_FALSE(EC);
+  ASSERT_EQ("path.h", std::string(I->path().substr(Root.size() + 1)));
 
-  EXPECT_EQ(3, NumDiagnostics);
+  EXPECT_EQ(0, NumDiagnostics);
 }
 
 TEST_F(VFSFromYAMLTest, NonFallthroughDirectoryIteration) {
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1649,10 +1649,19 @@
 sys::path::Style::windows_backslash)) {
 path_style = sys::path::Style::windows_backslash;
   } else {
-assert(NameValueNode && "Name presence should be checked earlier");
-error(NameValueNode,
+// Relative VFS root entries are made absolute to the current working
+// directory, then we can determine the path style from that.
+auto EC = sys::fs::make_absolute(Name);
+if (EC) {
+  assert(NameValueNode && "Name presence should be checked earlier");
+  error(
+  NameValueNode,
   "entry with relative path at the root level is not discoverable");
-return nullptr;
+  return nullptr;
+}
+path_style = sys::path::is_absolute(Name, sys::path::Style::posix)
+ ? sys::path::Style::posix
+ : sys::path::Style::windows_backslash;
   }
 }
 
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -547,6 +547,9 @@
 /// }
 /// \endverbatim
 ///
+/// The roots may be absolute or relative. If relative they will be made
+/// absolute against the current working directory.
+///
 /// All configuration options are optional.
 ///   'case-sensitive': 
 ///   'use-external-names': 
Index: clang/test/VFS/vfsoverlay-relative-root.c
===
--- /dev/null
+++ clang/test/VFS/vfsoverlay-relative-root.c
@@ -0,0 +1,3 @@
+// RUN: %clang_cc1 -Werror -ivfsoverlay %S/Inputs/vfsoverlay-root-relative.yaml -I virtual -fsyntax-only %s
+
+#include "virtual_header.h"
Index: clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
===
--- /dev/null
+++ clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
@@ -0,0 +1,17 @@
+{
+  'version': 0,
+  'fallthrough': 

[PATCH] D116174: [clang] support relative roots to vfs overlays

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 400102.
rmaz added a comment.

Update VirtualFileSystemTest to validate relative root paths.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116174/new/

https://reviews.llvm.org/D116174

Files:
  clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
  clang/test/VFS/vfsoverlay-relative-root.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -2164,6 +2164,11 @@
 
 TEST_F(VFSFromYAMLTest, RelativePaths) {
   IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  std::error_code EC;
+  SmallString<128> current_dir;
+  EC = llvm::sys::fs::current_path(current_dir);
+  ASSERT_FALSE(EC);
+
   // Filename at root level without a parent directory.
   IntrusiveRefCntPtr FS = getFromYAMLString(
   "{ 'roots': [\n"
@@ -2172,7 +2177,12 @@
   "  }\n"
   "] }",
   Lower);
-  EXPECT_EQ(nullptr, FS.get());
+  ASSERT_TRUE(FS.get() != nullptr);
+  SmallString<128> expected_path_not_in_dir("file-not-in-directory.h");
+  llvm::sys::fs::make_absolute(expected_path_not_in_dir);
+  checkContents(FS->dir_begin(current_dir, EC),
+{expected_path_not_in_dir});
+
 
   // Relative file path.
   FS = getFromYAMLString("{ 'roots': [\n"
@@ -2181,7 +2191,13 @@
  "  }\n"
  "] }",
  Lower);
-  EXPECT_EQ(nullptr, FS.get());
+  ASSERT_TRUE(FS.get() != nullptr);
+  SmallString<128> expected_path_relative_file_parent("relative/file");
+  SmallString<128> expected_path_relative_file("relative/file/path.h");
+  llvm::sys::fs::make_absolute(expected_path_relative_file_parent);
+  llvm::sys::fs::make_absolute(expected_path_relative_file);
+  checkContents(FS->dir_begin(expected_path_relative_file_parent, EC),
+{expected_path_relative_file});
 
   // Relative directory path.
   FS = getFromYAMLString(
@@ -2191,9 +2207,15 @@
   "  }\n"
   "] }",
   Lower);
-  EXPECT_EQ(nullptr, FS.get());
+  ASSERT_TRUE(FS.get() != nullptr);
+  SmallString<128> expected_path_relative_dir_parent("relative/directory");
+  SmallString<128> expected_path_relative_dir("relative/directory/path.h");
+  llvm::sys::fs::make_absolute(expected_path_relative_dir_parent);
+  llvm::sys::fs::make_absolute(expected_path_relative_dir);
+  checkContents(FS->dir_begin(expected_path_relative_dir_parent, EC),
+{expected_path_relative_dir});
 
-  EXPECT_EQ(3, NumDiagnostics);
+  EXPECT_EQ(0, NumDiagnostics);
 }
 
 TEST_F(VFSFromYAMLTest, NonFallthroughDirectoryIteration) {
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1649,10 +1649,19 @@
 sys::path::Style::windows_backslash)) {
 path_style = sys::path::Style::windows_backslash;
   } else {
-assert(NameValueNode && "Name presence should be checked earlier");
-error(NameValueNode,
+// Relative VFS root entries are made absolute to the current working
+// directory, then we can determine the path style from that.
+auto EC = sys::fs::make_absolute(Name);
+if (EC) {
+  assert(NameValueNode && "Name presence should be checked earlier");
+  error(
+  NameValueNode,
   "entry with relative path at the root level is not discoverable");
-return nullptr;
+  return nullptr;
+}
+path_style = sys::path::is_absolute(Name, sys::path::Style::posix)
+ ? sys::path::Style::posix
+ : sys::path::Style::windows_backslash;
   }
 }
 
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -547,6 +547,9 @@
 /// }
 /// \endverbatim
 ///
+/// The roots may be absolute or relative. If relative they will be made
+/// absolute against the current working directory.
+///
 /// All configuration options are optional.
 ///   'case-sensitive': 
 ///   'use-external-names': 
Index: clang/test/VFS/vfsoverlay-relative-root.c
===
--- /dev/null
+++ clang/test/VFS/vfsoverlay-relative-root.c
@@ -0,0 +1,3 @@
+// RUN: %clang_cc1 -Werror -ivfsoverlay %S/Inputs/vfsoverlay-root-relative.yaml -I virtual -fsyntax-only %s
+
+#include "virtual_header.h"
Index: clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml

[PATCH] D116174: [clang] support relative roots to vfs overlays

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 48.
rmaz added a comment.

Add comment to describe relative root path behaviour


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116174/new/

https://reviews.llvm.org/D116174

Files:
  clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
  clang/test/VFS/vfsoverlay-relative-root.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp


Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1649,10 +1649,19 @@
 sys::path::Style::windows_backslash)) {
 path_style = sys::path::Style::windows_backslash;
   } else {
-assert(NameValueNode && "Name presence should be checked earlier");
-error(NameValueNode,
+// Relative VFS root entries are made absolute to the current working
+// directory, then we can determine the path style from that.
+auto EC = sys::fs::make_absolute(Name);
+if (EC) {
+  assert(NameValueNode && "Name presence should be checked earlier");
+  error(
+  NameValueNode,
   "entry with relative path at the root level is not 
discoverable");
-return nullptr;
+  return nullptr;
+}
+path_style = sys::path::is_absolute(Name, sys::path::Style::posix)
+ ? sys::path::Style::posix
+ : sys::path::Style::windows_backslash;
   }
 }
 
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -547,6 +547,9 @@
 /// }
 /// \endverbatim
 ///
+/// The roots may be absolute or relative. If relative they will be made
+/// absolute against the current working directory.
+///
 /// All configuration options are optional.
 ///   'case-sensitive': 
 ///   'use-external-names': 
Index: clang/test/VFS/vfsoverlay-relative-root.c
===
--- /dev/null
+++ clang/test/VFS/vfsoverlay-relative-root.c
@@ -0,0 +1,3 @@
+// RUN: %clang_cc1 -Werror -ivfsoverlay 
%S/Inputs/vfsoverlay-root-relative.yaml -I virtual -fsyntax-only %s
+
+#include "virtual_header.h"
Index: clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
===
--- /dev/null
+++ clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
@@ -0,0 +1,17 @@
+{
+  'version': 0,
+  'fallthrough': true,
+  'overlay-relative': true,
+  'roots': [
+{ 'name': 'virtual',
+  'type': 'directory',
+  'contents': [
+{
+  'external-contents': 'actual_header.h',
+  'type': 'file',
+  'name': 'virtual_header.h',
+}
+  ]
+}
+  ]
+}


Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1649,10 +1649,19 @@
 sys::path::Style::windows_backslash)) {
 path_style = sys::path::Style::windows_backslash;
   } else {
-assert(NameValueNode && "Name presence should be checked earlier");
-error(NameValueNode,
+// Relative VFS root entries are made absolute to the current working
+// directory, then we can determine the path style from that.
+auto EC = sys::fs::make_absolute(Name);
+if (EC) {
+  assert(NameValueNode && "Name presence should be checked earlier");
+  error(
+  NameValueNode,
   "entry with relative path at the root level is not discoverable");
-return nullptr;
+  return nullptr;
+}
+path_style = sys::path::is_absolute(Name, sys::path::Style::posix)
+ ? sys::path::Style::posix
+ : sys::path::Style::windows_backslash;
   }
 }
 
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -547,6 +547,9 @@
 /// }
 /// \endverbatim
 ///
+/// The roots may be absolute or relative. If relative they will be made
+/// absolute against the current working directory.
+///
 /// All configuration options are optional.
 ///   'case-sensitive': 
 ///   'use-external-names': 
Index: clang/test/VFS/vfsoverlay-relative-root.c
===
--- /dev/null
+++ clang/test/VFS/vfsoverlay-relative-root.c
@@ -0,0 +1,3 @@
+// RUN: %clang_cc1 -Werror -ivfsoverlay %S/Inputs/vfsoverlay-root-relative.yaml -I 

[PATCH] D116174: [clang] support relative roots to vfs overlays

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D116174#3242087 , @benlangmuir 
wrote:

> Each VFS may have its own working directory, so it could be surprising if we 
> use the OS working directory instead of that.  This is complicated by the 
> fact the VFS working directory may not be set yet during parsing the yaml (I 
> haven't checked).  I'm not really sure what to recommend here. If we do 
> change this, we should document the new behaviour in the doc comment for 
> RedirectingFileSystem though.

I don't believe the working directory will be set on the VFS at this point, no. 
If you're happy with the change then i'll update the header docs to describe 
this behaviour, unless there is another way to achieve using relative paths for 
root entries in the overlay yaml?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116174/new/

https://reviews.llvm.org/D116174

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116174: [clang] support relative roots to vfs overlays

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 395896.
rmaz added a comment.

clang-format changes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116174/new/

https://reviews.llvm.org/D116174

Files:
  clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
  clang/test/VFS/vfsoverlay-relative-root.c
  llvm/lib/Support/VirtualFileSystem.cpp


Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1649,10 +1649,19 @@
 sys::path::Style::windows_backslash)) {
 path_style = sys::path::Style::windows_backslash;
   } else {
-assert(NameValueNode && "Name presence should be checked earlier");
-error(NameValueNode,
+// Relative VFS root entries are made absolute to the current working
+// directory, then we can determine the path style from that.
+auto EC = sys::fs::make_absolute(Name);
+if (EC) {
+  assert(NameValueNode && "Name presence should be checked earlier");
+  error(
+  NameValueNode,
   "entry with relative path at the root level is not 
discoverable");
-return nullptr;
+  return nullptr;
+}
+path_style = sys::path::is_absolute(Name, sys::path::Style::posix)
+ ? sys::path::Style::posix
+ : sys::path::Style::windows_backslash;
   }
 }
 
Index: clang/test/VFS/vfsoverlay-relative-root.c
===
--- /dev/null
+++ clang/test/VFS/vfsoverlay-relative-root.c
@@ -0,0 +1,3 @@
+// RUN: %clang_cc1 -Werror -ivfsoverlay 
%S/Inputs/vfsoverlay-root-relative.yaml -I virtual -fsyntax-only %s
+
+#include "virtual_header.h"
Index: clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
===
--- /dev/null
+++ clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
@@ -0,0 +1,17 @@
+{
+  'version': 0,
+  'fallthrough': true,
+  'overlay-relative': true,
+  'roots': [
+{ 'name': 'virtual',
+  'type': 'directory',
+  'contents': [
+{
+  'external-contents': 'actual_header.h',
+  'type': 'file',
+  'name': 'virtual_header.h',
+}
+  ]
+}
+  ]
+}


Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1649,10 +1649,19 @@
 sys::path::Style::windows_backslash)) {
 path_style = sys::path::Style::windows_backslash;
   } else {
-assert(NameValueNode && "Name presence should be checked earlier");
-error(NameValueNode,
+// Relative VFS root entries are made absolute to the current working
+// directory, then we can determine the path style from that.
+auto EC = sys::fs::make_absolute(Name);
+if (EC) {
+  assert(NameValueNode && "Name presence should be checked earlier");
+  error(
+  NameValueNode,
   "entry with relative path at the root level is not discoverable");
-return nullptr;
+  return nullptr;
+}
+path_style = sys::path::is_absolute(Name, sys::path::Style::posix)
+ ? sys::path::Style::posix
+ : sys::path::Style::windows_backslash;
   }
 }
 
Index: clang/test/VFS/vfsoverlay-relative-root.c
===
--- /dev/null
+++ clang/test/VFS/vfsoverlay-relative-root.c
@@ -0,0 +1,3 @@
+// RUN: %clang_cc1 -Werror -ivfsoverlay %S/Inputs/vfsoverlay-root-relative.yaml -I virtual -fsyntax-only %s
+
+#include "virtual_header.h"
Index: clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
===
--- /dev/null
+++ clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
@@ -0,0 +1,17 @@
+{
+  'version': 0,
+  'fallthrough': true,
+  'overlay-relative': true,
+  'roots': [
+{ 'name': 'virtual',
+  'type': 'directory',
+  'contents': [
+{
+  'external-contents': 'actual_header.h',
+  'type': 'file',
+  'name': 'virtual_header.h',
+}
+  ]
+}
+  ]
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116174: [clang] support relative roots to vfs overlays

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 395893.
rmaz added a comment.

lint


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116174/new/

https://reviews.llvm.org/D116174

Files:
  clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
  clang/test/VFS/vfsoverlay-relative-root.c
  llvm/lib/Support/VirtualFileSystem.cpp


Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1649,10 +1649,17 @@
 sys::path::Style::windows_backslash)) {
 path_style = sys::path::Style::windows_backslash;
   } else {
-assert(NameValueNode && "Name presence should be checked earlier");
-error(NameValueNode,
-  "entry with relative path at the root level is not 
discoverable");
-return nullptr;
+// Relative VFS root entries are made absolute to the current working
+// directory, then we can determine the path style from that.
+auto EC = sys::fs::make_absolute(Name);
+if (EC) {
+  assert(NameValueNode && "Name presence should be checked earlier");
+  error(NameValueNode,
+"entry with relative path at the root level is not 
discoverable");
+  return nullptr;
+}
+path_style = sys::path::is_absolute(Name, sys::path::Style::posix) ?
+  sys::path::Style::posix : sys::path::Style::windows_backslash;
   }
 }
 
Index: clang/test/VFS/vfsoverlay-relative-root.c
===
--- /dev/null
+++ clang/test/VFS/vfsoverlay-relative-root.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -Werror -ivfsoverlay 
%S/Inputs/vfsoverlay-root-relative.yaml -I virtual -fsyntax-only %s
+
+#include "virtual_header.h"
+
Index: clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
===
--- /dev/null
+++ clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
@@ -0,0 +1,17 @@
+{
+  'version': 0,
+  'fallthrough': true,
+  'overlay-relative': true,
+  'roots': [
+{ 'name': 'virtual',
+  'type': 'directory',
+  'contents': [
+{
+  'external-contents': 'actual_header.h',
+  'type': 'file',
+  'name': 'virtual_header.h',
+}
+  ]
+}
+  ]
+}


Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1649,10 +1649,17 @@
 sys::path::Style::windows_backslash)) {
 path_style = sys::path::Style::windows_backslash;
   } else {
-assert(NameValueNode && "Name presence should be checked earlier");
-error(NameValueNode,
-  "entry with relative path at the root level is not discoverable");
-return nullptr;
+// Relative VFS root entries are made absolute to the current working
+// directory, then we can determine the path style from that.
+auto EC = sys::fs::make_absolute(Name);
+if (EC) {
+  assert(NameValueNode && "Name presence should be checked earlier");
+  error(NameValueNode,
+"entry with relative path at the root level is not discoverable");
+  return nullptr;
+}
+path_style = sys::path::is_absolute(Name, sys::path::Style::posix) ?
+  sys::path::Style::posix : sys::path::Style::windows_backslash;
   }
 }
 
Index: clang/test/VFS/vfsoverlay-relative-root.c
===
--- /dev/null
+++ clang/test/VFS/vfsoverlay-relative-root.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -Werror -ivfsoverlay %S/Inputs/vfsoverlay-root-relative.yaml -I virtual -fsyntax-only %s
+
+#include "virtual_header.h"
+
Index: clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
===
--- /dev/null
+++ clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
@@ -0,0 +1,17 @@
+{
+  'version': 0,
+  'fallthrough': true,
+  'overlay-relative': true,
+  'roots': [
+{ 'name': 'virtual',
+  'type': 'directory',
+  'contents': [
+{
+  'external-contents': 'actual_header.h',
+  'type': 'file',
+  'name': 'virtual_header.h',
+}
+  ]
+}
+  ]
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116174: [clang] support relative roots to vfs overlays

[Richard Howell via Phabricator via cfe-commits]

rmaz created this revision.
Herald added subscribers: dexonsmith, hiraditya.
rmaz requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

This diff adds support for relative roots to VFS overlays.
The directory root will be made absolute from the current
working directory and will be used to determine the path
style to use.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116174

Files:
  clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
  clang/test/VFS/vfsoverlay-relative-root.c
  llvm/lib/Support/VirtualFileSystem.cpp


Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1649,10 +1649,17 @@
 sys::path::Style::windows_backslash)) {
 path_style = sys::path::Style::windows_backslash;
   } else {
-assert(NameValueNode && "Name presence should be checked earlier");
-error(NameValueNode,
-  "entry with relative path at the root level is not 
discoverable");
-return nullptr;
+// Relative VFS root entries are made absolute to the current working
+// directory, then we can determine the path style from that.
+auto EC = sys::fs::make_absolute(Name);
+if (EC) {
+  assert(NameValueNode && "Name presence should be checked earlier");
+  error(NameValueNode,
+"entry with relative path at the root level is not 
discoverable");
+  return nullptr;
+}
+path_style = sys::path::is_absolute(Name, sys::path::Style::posix) ?
+  sys::path::Style::posix : sys::path::Style::windows_backslash;
   }
 }
 
Index: clang/test/VFS/vfsoverlay-relative-root.c
===
--- /dev/null
+++ clang/test/VFS/vfsoverlay-relative-root.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -Werror -ivfsoverlay 
%S/Inputs/vfsoverlay-root-relative.yaml -I virtual -fsyntax-only %s
+
+#include "virtual_header.h"
+
Index: clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
===
--- /dev/null
+++ clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
@@ -0,0 +1,17 @@
+{
+  'version': 0,
+  'fallthrough': true,
+  'overlay-relative': true,
+  'roots': [
+{ 'name': 'virtual',
+  'type': 'directory',
+  'contents': [
+{
+  'external-contents': 'actual_header.h',
+  'type': 'file',
+  'name': 'virtual_header.h',
+}
+  ]
+}
+  ]
+}


Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1649,10 +1649,17 @@
 sys::path::Style::windows_backslash)) {
 path_style = sys::path::Style::windows_backslash;
   } else {
-assert(NameValueNode && "Name presence should be checked earlier");
-error(NameValueNode,
-  "entry with relative path at the root level is not discoverable");
-return nullptr;
+// Relative VFS root entries are made absolute to the current working
+// directory, then we can determine the path style from that.
+auto EC = sys::fs::make_absolute(Name);
+if (EC) {
+  assert(NameValueNode && "Name presence should be checked earlier");
+  error(NameValueNode,
+"entry with relative path at the root level is not discoverable");
+  return nullptr;
+}
+path_style = sys::path::is_absolute(Name, sys::path::Style::posix) ?
+  sys::path::Style::posix : sys::path::Style::windows_backslash;
   }
 }
 
Index: clang/test/VFS/vfsoverlay-relative-root.c
===
--- /dev/null
+++ clang/test/VFS/vfsoverlay-relative-root.c
@@ -0,0 +1,4 @@
+// RUN: %clang_cc1 -Werror -ivfsoverlay %S/Inputs/vfsoverlay-root-relative.yaml -I virtual -fsyntax-only %s
+
+#include "virtual_header.h"
+
Index: clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
===
--- /dev/null
+++ clang/test/VFS/Inputs/vfsoverlay-root-relative.yaml
@@ -0,0 +1,17 @@
+{
+  'version': 0,
+  'fallthrough': true,
+  'overlay-relative': true,
+  'roots': [
+{ 'name': 'virtual',
+  'type': 'directory',
+  'contents': [
+{
+  'external-contents': 'actual_header.h',
+  'type': 'file',
+  'name': 'virtual_header.h',
+}
+  ]
+}
+  ]
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

> I don't recall any issues on my last benchmark, but i'll run the patch across 
> all of our modular files and see if anything comes up.

I ran the patch against ~1500 sources here and there were no unexpected 
warnings or failures.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D109632#3088479 , @vsapsai wrote:

> Code-wise I'm not aware of any remaining issues. Still need to update the 
> commit message and to re-run the  clang test suite. But you can totally use 
> the patch for testing. I plan to update D110123 
>  for the review today/tomorrow.

Sounds good.

> In my limited internal testing I've seen a single extra warning due to 
> `[(id)specificObject commonMethodName]`. I've discussed it with other 
> Objective-C developers and the consensus is that with calling methods on `id` 
> you cannot predict which exactly method signature will be selected and the 
> recommended solution to cast `specificObject` to correct type with the known 
> method signature. It might be worth running a more extensive test and make 
> sure there are no unintended consequences. That will take me around a week or 
> slightly more.

I don't recall any issues on my last benchmark, but i'll run the patch across 
all of our modular files and see if anything comes up.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz abandoned this revision.
rmaz added a comment.

@vsapsai i'll abandon this diff then, thanks for your extensive feedback on the 
approach. Is D110123  shippable already, or 
are there some more corner cases to cover?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110092: [clang][NFC] encapsulate global method list in GlobalMethodPool

[Richard Howell via Phabricator via cfe-commits]

rmaz abandoned this revision.
rmaz added a comment.

Abandoning in favor of D110123 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110092/new/

https://reviews.llvm.org/D110092

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D109632#3085187 , @dexonsmith 
wrote:

> But another benefit of not double-storing transitively imported methods is 
> that it makes the PCMs more independent, tacking slightly closer to 
> "ImmediateDep1.pcm" being reproducible even when "SharedDep.pcm" adds a 
> method to the global pool. This is a nice property if it's not too expensive. 
> Looking at the numbers above, it doesn't look expensive; the relative 
> performance for @rmaz's motivating use case seems pretty small.
>
> @rmaz, will your goals be achieved by taking @vsapsai's approach? If so, I'm 
> leaning slightly that way.

We can definitely work with it. From my perspective the faster solution would 
be preferred, but if there are potential future benefits from not storing 
dependent modules methods in a pcm them we can ship this solution and look at 
other possible avenues for performance improvement to gain parity with the 
non-modular case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

I am seeing the clean build times behaving the same as the populated ones, just 
slower:

| *File*| *Baseline (s)* | *Set Dedupe* | *No External* |
| IGMFVC.mm | 230| 194  | 195   |
|

So given these numbers are we good to go ahead with set dedupe approach?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D109632#3076586 , @vsapsai wrote:

> Methodology: clear a modules cache, compile a file once to pre-populate the 
> cache, compile file 8 times and measure elapsed time, take the time average.

This is the same approach I used, although with 3 tries.

> So it looks like "no external" approach is slightly but consistently slower  
> than "set dedupe" approach.

This agrees with what I see with our code too.

> I'm curious to get the results for an empty module cache because clean builds 
> are also important for us.

I should measure this too. What would you suggest for the approach, to clean 
the module cache before each build, retry and average? How much weight should 
be given to the clean vs populated module cache numbers?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D109632#3062608 , @vsapsai wrote:

> I can be wrong but I like writing less data as it can result in savings for 
> more projects. For example, if compile time is dominated by method 
> deserialization and not by `Sema::addMethodToGlobalList`, we still should see 
> an improvement. I can try to experiment on a bunch of other projects and 
> their heavy .m files and report the results, if you'd like.

I think that would be a useful comparison, yes. For our code I consistently 
measure this approach as 10-20% slower on average, with very few outliers where 
the set deduplication approach is slower.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111457: [clang][test] Add lit helper for windows paths

[Richard Howell via Phabricator via cfe-commits]

rmaz added inline comments.



Comment at: clang/test/lit.cfg.py:60
+if platform.system() == 'Windows':
+root_sep = 'C:\\'
+else:

keith wrote:
> rmaz wrote:
> > `c:\`?
> Do you mean lowercase or a single slash? I see ~2x the number of uppercase vs 
> lowercase in this codebase now, and the second is just because of escaping
I meant the single slash, yes, wasn't sure if it needed to be escaped or not


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111457/new/

https://reviews.llvm.org/D111457

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111457: [clang][test] Add lit helper for windows paths

[Richard Howell via Phabricator via cfe-commits]

rmaz added inline comments.



Comment at: clang/test/lit.cfg.py:60
+if platform.system() == 'Windows':
+root_sep = 'C:\\'
+else:

`c:\`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111457/new/

https://reviews.llvm.org/D111457

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D109632#3046976 , @vsapsai wrote:

> These are interesting results. I'm curious to measure the time spent in 
> `ASTReader::ReadMethodPool`.

Updated numbers from today, looks like we added a lot more modular deps since 
last week.

| **Method**  | **# Module File Lookups** | **# Module File Hits** | **# 
Instance Methods Deserialized** | **# Class Methods Deserialized** | **ms in 
`ASTReader::ReadMethodPool`** |
| Baseline| 139140| 577| 254304 
 | 119907   | 15759 
|
| Set Dedupe  | 139140| 577| 38815  
 | 19125| 421   
|
| No external | 157677| 1595   | 5036   
 | 2428 | 430   
|
|

The time taken in `ASTReader::ReadMethodPool` ends up very close. This was 
previously dominated by `Sema::addMethodToGlobalList`, and both approaches will 
end up with the same number of calls to this method, so maybe this is not so 
surprising. The difference comes down to: is it faster to descend into all 
build dependent modules to deserialize only the methods you need or is it 
faster to deserialize from the first module with overlap and skip the duplicate 
inserts with a hashmap lookup. The numbers would suggest the latter, and this 
approach also has the benefit of requiring no other changes as the method 
insert order to the global list should be identical.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D109632#3037501 , @vsapsai wrote:

> My assumption was that all dependent modules are in memory at this point. And 
> we visit transitive modules only once, so I don't expect it to be a big 
> performance hit (though I can be wrong). And deserializing methods from 
> different modules shouldn't be more work because we are deserializing fewer 
> methods than with "set dedupe".

I added some stats collection for the number of methods deserialized, here are 
the results from the slowest file in the above table:

| **Method**  | **# Module File Lookups** | **# Module File Hits** | **# 
Instance Methods Deserialized** | **# Class Methods Deserialized** |
| Baseline| 34023 | 129| 82005  
 | 29298|
| Set Dedupe  | 34023 | 129| 13743  
 | 6781 |
| No external | 45265 | 960| 4010   
 | 1704 |
|

So while the no external approach has ~3.5x fewer methods to deserialize, it 
has to visit ~7.5x as many module files to deserialize methods from.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D109632#3032381 , @vsapsai wrote:

> What's interesting, I was able to trigger more diagnostic. Specifically, I 
> got warnings about `length` ambiguity because in NSStatusItem it is CGFloat 
> ,
>  while in NSString it is NSUInteger. I also had more diagnostic that's 
> unclear how it is triggered. It can be a project issue or a bug somewhere 
> else, need to check it more carefully.

Yes, I also had a couple of files fail to compile due to mismatched (or 
differently matched) selectors.

> In theory, "set dedupe" approach should be slower than "no external" as we 
> are iterating through shared dependencies. But in practice it isn't, which is 
> puzzling. My current theory is that "no external" also feeds into 
> correctness, so we might be doing more [correct] method overloading checks.

Well, wouldn't the tradeoff there be that we now have to descend into all 
dependent modules during selector lookup when we didn't have to previously? And 
this would do more work as only a small subset of those modules would have a 
selector match, which in the current case has already been handled and 
serialized on a single method list.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

Avoiding serializing external methods in `ASTMethodPoolTrait` is working 
successfully, although the times are not as good as using set deduplication:

| **file**   | **baseline** | **set dedupe** | **no external** |
| Total  | 149.68   | 92.97  | 109.83  |
| IGMFVC.mm  | 25.75| 5.38   | 6.35|
| IGMFLADS.m | 15.97| 3.30   | 4.61|
| IGIMFVC.mm | 6.70 | 2.98   | 3.77|
| IGSFCVC.m  | 4.57 | 1.53   | 1.87|
| IGVFVC.mm  | 4.19 | 1.61   | 1.85|
| IGMFAHC.mm | 4.00 | 1.93   | 2.41|
| IGMFLCVC.m | 3.89 | 1.55   | 1.96|
| PLRFPPC.mm | 3.19 | 3.26   | 4.05|
| IGBFPVC.m  | 3.18 | 1.11   | 1.36|
|

Where the no external numbers are using D110648 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D109632#3022209 , @vsapsai wrote:

> That patch looks correct, I was experimenting with exactly the same local 
> change. Have you tried D110123  on your 
> original build? In my testing with synthetic test case it looks as good as 
> set deduplication.

I am getting some pretty unexpected results from this approach. For the files I 
could get to complete below are the compilation times for each approach for the 
10 slowest (in the baseline) files:

| **file**   | **baseline** | **set deduplication** | **no external methods in 
pcm** |
| Total  | 95.57| 75.30 | 306.46
 |
| IGSFCVC.m  | 4.57 | 1.53  | 13.71 
 |
| IGVFVC.mm  | 4.19 | 1.61  | 27.93 
 |
| IGMFAHC.mm | 4.00 | 1.93  | 9.94  
 |
| IGMFLCVC.m | 3.89 | 1.55  | 12.30 
 |
| PLRFPPC.mm | 3.19 | 3.26  | 3.99  
 |
| IGBFPVC.m  | 3.18 | 1.11  | 28.92 
 |
| IGBVC.m| 2.37 | 0.98  | 17.42 
 |
| PLRRM.mm   | 1.99 | 1.99  | 2.31  
 |
| IGBSPSC.m  | 1.88 | 0.85  | 14.38 
 |
|

All of the most interesting files were left out of these results as I could not 
get compilation to complete for the no external methods approach.

> I still think it is worth to pursue writing only owned methods in 
> `METHOD_POOL`. Deduplication with a DenseSet works but I still expect it to 
> be more expensive than avoiding duplicates by construction. As for the next 
> step I was thinking about changing `ASTMethodPoolTrait` to skip transitive 
> methods during serialization and it should help with eliminating all the 
> duplicates. With that change we can test and see how it works.

I will take a go at this approach to eliminate all external methods and compare 
to see if this exhibits similar behaviour.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

@vsapsai just checking on where we are with this one. Is the current plan to 
investigate an approach only serializing the methods declared in each module, 
or are we good to go ahead with the set de-duplication approach? I tried 
profiling with D110123  and with the 
following patch:

  diff --git a/clang/lib/Serialization/ASTReader.cpp 
b/clang/lib/Serialization/ASTReader.cpp
  index 279067a8ac8b..aaaca0aff9ab 100644
  --- a/clang/lib/Serialization/ASTReader.cpp
  +++ b/clang/lib/Serialization/ASTReader.cpp
  @@ -8130,7 +8130,7 @@ namespace serialization {
 FactoryBits = Data.FactoryBits;
 InstanceHasMoreThanOneDecl = Data.InstanceHasMoreThanOneDecl;
 FactoryHasMoreThanOneDecl = Data.FactoryHasMoreThanOneDecl;
  -  return true;
  +  return false;
   }

but this was a fair bit slower than the deduplication approach, and for some 
files would never complete, presumably stuck in an infinite loop of dependent 
modules. Is there a way to exclude the serialization of methods from dependent 
modules but make the method lookup more efficient somehow?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

> What folks are thinking about writing less in METHOD_POOL?

I prefer the idea of it, but I think the `ReadMethodPoolVisitor` also has to be 
changed for this to work. When it finds a selector in a module it will return 
true, which causes the search to stop descending into dependent modules:

  if (!Visitor(*CurrentModule))
continue;
  
  // The visitor has requested that cut off visitation of any
  // module that the current module depends on. To indicate this
  // behavior, we mark all of the reachable modules as having been visited.

Wouldn't this logic have to be changed to ensure we pick up all the transitive 
methods from dependent modules?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110092: [clang][NFC] encapsulate global method list in GlobalMethodPool

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 373675.
rmaz added a comment.

remove unnecessary empty check


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110092/new/

https://reviews.llvm.org/D110092

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Serialization/ASTReader.cpp

Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -8178,14 +8178,6 @@
 } // namespace serialization
 } // namespace clang
 
-/// Add the given set of methods to the method list.
-static void addMethodsToPool(Sema , ArrayRef Methods,
- ObjCMethodList ) {
-  for (unsigned I = 0, N = Methods.size(); I != N; ++I) {
-S.addMethodToGlobalList(, Methods[I]);
-  }
-}
-
 void ASTReader::ReadMethodPool(Selector Sel) {
   // Get the selector generation and update it to the current generation.
   unsigned  = SelectorGeneration[Sel];
@@ -8208,20 +8200,12 @@
 return;
 
   Sema  = *getSema();
-  Sema::GlobalMethodPool::iterator Pos =
-  S.MethodPool.insert(std::make_pair(Sel, Sema::GlobalMethodPool::Lists()))
-  .first;
-
-  Pos->second.first.setBits(Visitor.getInstanceBits());
-  Pos->second.first.setHasMoreThanOneDecl(Visitor.instanceHasMoreThanOneDecl());
-  Pos->second.second.setBits(Visitor.getFactoryBits());
-  Pos->second.second.setHasMoreThanOneDecl(Visitor.factoryHasMoreThanOneDecl());
-
-  // Add methods to the global pool *after* setting hasMoreThanOneDecl, since
-  // when building a module we keep every method individually and may need to
-  // update hasMoreThanOneDecl as we add the methods.
-  addMethodsToPool(S, Visitor.getInstanceMethods(), Pos->second.first);
-  addMethodsToPool(S, Visitor.getFactoryMethods(), Pos->second.second);
+  S.MethodPool.addMethodsForSelector(Sel, Visitor.getInstanceMethods(), true,
+ Visitor.getInstanceBits(),
+ Visitor.instanceHasMoreThanOneDecl());
+  S.MethodPool.addMethodsForSelector(Sel, Visitor.getFactoryMethods(), false,
+ Visitor.getFactoryBits(),
+ Visitor.factoryHasMoreThanOneDecl());
 }
 
 void ASTReader::updateOutOfDateSelector(Selector Sel) {
Index: clang/lib/Sema/SemaDeclObjC.cpp
===
--- clang/lib/Sema/SemaDeclObjC.cpp
+++ clang/lib/Sema/SemaDeclObjC.cpp
@@ -3300,8 +3300,8 @@
   return MethodInterface == MethodInListInterface;
 }
 
-void Sema::addMethodToGlobalList(ObjCMethodList *List,
- ObjCMethodDecl *Method) {
+void Sema::GlobalMethodPool::addMethodToGlobalList(ObjCMethodList *List,
+   ObjCMethodDecl *Method) {
   // Record at the head of the list whether there were 0, 1, or >= 2 methods
   // inside categories.
   if (ObjCCategoryDecl *CD =
@@ -3322,11 +3322,11 @@
   ObjCMethodList *ListWithSameDeclaration = nullptr;
   for (; List; Previous = List, List = List->getNext()) {
 // If we are building a module, keep all of the methods.
-if (getLangOpts().isCompilingModule())
+if (SemaRef.getLangOpts().isCompilingModule())
   continue;
 
-bool SameDeclaration = MatchTwoMethodDeclarations(Method,
-  List->getMethod());
+bool SameDeclaration =
+SemaRef.MatchTwoMethodDeclarations(Method, List->getMethod());
 // Looking for method with a type bound requires the correct context exists.
 // We need to insert a method into the list if the context is different.
 // If the method's declaration matches the list
@@ -3389,7 +3389,7 @@
 
   // We have a new signature for an existing method - add it.
   // This is extremely rare. Only 1% of Cocoa selectors are "overloaded".
-  ObjCMethodList *Mem = BumpAlloc.Allocate();
+  ObjCMethodList *Mem = SemaRef.BumpAlloc.Allocate();
 
   // We insert it right before ListWithSameDeclaration.
   if (ListWithSameDeclaration) {
@@ -3425,17 +3425,9 @@
   if (ExternalSource)
 ReadMethodPool(Method->getSelector());
 
-  GlobalMethodPool::iterator Pos = MethodPool.find(Method->getSelector());
-  if (Pos == MethodPool.end())
-Pos = MethodPool
-  .insert(std::make_pair(Method->getSelector(),
- GlobalMethodPool::Lists()))
-  .first;
-
   Method->setDefined(impl);
-
-  ObjCMethodList  = instance ? Pos->second.first : Pos->second.second;
-  addMethodToGlobalList(, Method);
+  assert(Method->isInstanceMethod() == instance);
+  MethodPool.addMethod(Method);
 }
 
 /// Determines if this is an "acceptable" loose mismatch in the global
Index: clang/lib/Sema/Sema.cpp
===
--- 

[PATCH] D110092: [clang][NFC] encapsulate global method list in GlobalMethodPool

[Richard Howell via Phabricator via cfe-commits]

rmaz created this revision.
rmaz requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This change moves the `addMethodToGlobalList` function to be a
private member function of the `GlobalMethodPool` class. This
is a preparatory step to allow for de-duplication of inserted
methods.

Two public methods are added to handle the existing use cases
for adding methods to a global list: `addMethodsForSelector`
and `addMethod`. The former is required to avoid the overhead
of looking up the `Methods` map for each insert when adding
multiple methods for a single selector when deserializing AST
files. It also allows for modifying the list properties first.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D110092

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Serialization/ASTReader.cpp

Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -8178,14 +8178,6 @@
 } // namespace serialization
 } // namespace clang
 
-/// Add the given set of methods to the method list.
-static void addMethodsToPool(Sema , ArrayRef Methods,
- ObjCMethodList ) {
-  for (unsigned I = 0, N = Methods.size(); I != N; ++I) {
-S.addMethodToGlobalList(, Methods[I]);
-  }
-}
-
 void ASTReader::ReadMethodPool(Selector Sel) {
   // Get the selector generation and update it to the current generation.
   unsigned  = SelectorGeneration[Sel];
@@ -8208,20 +8200,12 @@
 return;
 
   Sema  = *getSema();
-  Sema::GlobalMethodPool::iterator Pos =
-  S.MethodPool.insert(std::make_pair(Sel, Sema::GlobalMethodPool::Lists()))
-  .first;
-
-  Pos->second.first.setBits(Visitor.getInstanceBits());
-  Pos->second.first.setHasMoreThanOneDecl(Visitor.instanceHasMoreThanOneDecl());
-  Pos->second.second.setBits(Visitor.getFactoryBits());
-  Pos->second.second.setHasMoreThanOneDecl(Visitor.factoryHasMoreThanOneDecl());
-
-  // Add methods to the global pool *after* setting hasMoreThanOneDecl, since
-  // when building a module we keep every method individually and may need to
-  // update hasMoreThanOneDecl as we add the methods.
-  addMethodsToPool(S, Visitor.getInstanceMethods(), Pos->second.first);
-  addMethodsToPool(S, Visitor.getFactoryMethods(), Pos->second.second);
+  S.MethodPool.addMethodsForSelector(Sel, Visitor.getInstanceMethods(), true,
+ Visitor.getInstanceBits(),
+ Visitor.instanceHasMoreThanOneDecl());
+  S.MethodPool.addMethodsForSelector(Sel, Visitor.getFactoryMethods(), false,
+ Visitor.getFactoryBits(),
+ Visitor.factoryHasMoreThanOneDecl());
 }
 
 void ASTReader::updateOutOfDateSelector(Selector Sel) {
Index: clang/lib/Sema/SemaDeclObjC.cpp
===
--- clang/lib/Sema/SemaDeclObjC.cpp
+++ clang/lib/Sema/SemaDeclObjC.cpp
@@ -3300,8 +3300,8 @@
   return MethodInterface == MethodInListInterface;
 }
 
-void Sema::addMethodToGlobalList(ObjCMethodList *List,
- ObjCMethodDecl *Method) {
+void Sema::GlobalMethodPool::addMethodToGlobalList(ObjCMethodList *List,
+   ObjCMethodDecl *Method) {
   // Record at the head of the list whether there were 0, 1, or >= 2 methods
   // inside categories.
   if (ObjCCategoryDecl *CD =
@@ -3322,11 +3322,11 @@
   ObjCMethodList *ListWithSameDeclaration = nullptr;
   for (; List; Previous = List, List = List->getNext()) {
 // If we are building a module, keep all of the methods.
-if (getLangOpts().isCompilingModule())
+if (SemaRef.getLangOpts().isCompilingModule())
   continue;
 
-bool SameDeclaration = MatchTwoMethodDeclarations(Method,
-  List->getMethod());
+bool SameDeclaration =
+SemaRef.MatchTwoMethodDeclarations(Method, List->getMethod());
 // Looking for method with a type bound requires the correct context exists.
 // We need to insert a method into the list if the context is different.
 // If the method's declaration matches the list
@@ -3389,7 +3389,7 @@
 
   // We have a new signature for an existing method - add it.
   // This is extremely rare. Only 1% of Cocoa selectors are "overloaded".
-  ObjCMethodList *Mem = BumpAlloc.Allocate();
+  ObjCMethodList *Mem = SemaRef.BumpAlloc.Allocate();
 
   // We insert it right before ListWithSameDeclaration.
   if (ListWithSameDeclaration) {
@@ -3425,17 +3425,9 @@
   if (ExternalSource)
 ReadMethodPool(Method->getSelector());
 
-  GlobalMethodPool::iterator Pos = MethodPool.find(Method->getSelector());
-  if (Pos == MethodPool.end())
-Pos = MethodPool
-  

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:1434-1436
+bool addMethod(ObjCMethodDecl *Method) {
+  return AddedMethods.insert(Method).second;
+}

rmaz wrote:
> dexonsmith wrote:
> > Hmm, I was imagining that the set would be more encapsulated than this, not 
> > just stored in the same place.
> > 
> > I'm wondering if the following could be done in a prep commit:
> > 
> > - Change Sema::addMethodToGlobalList to a private member function of 
> > GlobalMethodPool.
> > - Make GlobalMethodPool::insert private
> > - Add `GlobalMethodPool::addMethod(ObjCMethodDecl*,bool,bool)`, which does 
> > the second half of Sema::AddMethodToGlobalPool (the parts that don't need 
> > Sema's other fields), and change the latter to call the former.
> > 
> > WDYT?
> Definitely neater, will take a look at this later today.
This might need a slightly different approach, as for the insert use case we 
have:

```lang=cxx
Sema  = *getSema();
  Sema::GlobalMethodPool::iterator Pos =
  S.MethodPool.insert(std::make_pair(Sel, Sema::GlobalMethodPool::Lists()))
  .first;

  Pos->second.first.setBits(Visitor.getInstanceBits());
  Pos->second.first.setHasMoreThanOneDecl(Visitor.instanceHasMoreThanOneDecl());
  Pos->second.second.setBits(Visitor.getFactoryBits());
  Pos->second.second.setHasMoreThanOneDecl(Visitor.factoryHasMoreThanOneDecl());

  // Add methods to the global pool *after* setting hasMoreThanOneDecl, since
  // when building a module we keep every method individually and may need to
  // update hasMoreThanOneDecl as we add the methods.
  addMethodsToPool(S, Visitor.getInstanceMethods(), Pos->second.first);
  addMethodsToPool(S, Visitor.getFactoryMethods(), Pos->second.second);
```

At the moment we fetch a method list, modify its state and then start inserting 
the methods. If we move to something like `MethodPool.addMethod(ObjCMethodDecl 
*)` we will have to look up the method list for each insert, and we would need 
extra methods to set the state first on the method list. How about something 
like `MethodPool.addMethods(ArrayRef methods, unsigned 
instanceBits, bool hasMoreThanOneDecl)`? Then we only need two list lookups per 
selector as before and we can handle the list state update before insert.
 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:1434-1436
+bool addMethod(ObjCMethodDecl *Method) {
+  return AddedMethods.insert(Method).second;
+}

dexonsmith wrote:
> Hmm, I was imagining that the set would be more encapsulated than this, not 
> just stored in the same place.
> 
> I'm wondering if the following could be done in a prep commit:
> 
> - Change Sema::addMethodToGlobalList to a private member function of 
> GlobalMethodPool.
> - Make GlobalMethodPool::insert private
> - Add `GlobalMethodPool::addMethod(ObjCMethodDecl*,bool,bool)`, which does 
> the second half of Sema::AddMethodToGlobalPool (the parts that don't need 
> Sema's other fields), and change the latter to call the former.
> 
> WDYT?
Definitely neater, will take a look at this later today.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D109632#3005512 , @vsapsai wrote:

> Thanks for the explanation! I'm still curious to reproduce the problem 
> locally and have created a test case generator 
> https://gist.github.com/vsapsai/f9d3603dde95eebd23248da4d7b4f5ec It creates a 
> chain of .m -> Synthesized9 -> Synthesized8 -> Synthesized7 ->... Does it 
> represent the structure of the code you are dealing with?

The case we have is more like:

  .m   -> A -> long list of partially shared deps -> Foundation
   -> B -> long list of partially shared deps -> Foundation
   -> C -> long list of partially shared deps -> Foundation
    * a few hundred

So we have a file that imports a lot of modules, in the hundreds. Each of those 
modules has multiple ObjC interfaces with `-(id)init NS_UNAVAILABLE` and 
imports Foundation, UIKit and also a large number of libraries that are shared 
across the top level imports. This will result in A.pcm, B.pcm and C.pcm 
including hundreds or thousands of init decls that are the same, from system 
frameworks or whatever modules are shared between the top level imports.

IIUC the code currently serializes the entire ObjCMethodList for a module for 
every declared method, including the methods that are not part of that module. 
When deserializing we don't descend into module dependencies as the entire 
method list would already be deserialized, but that doesn't help for modules 
that aren't directly dependent. Is this right? If so it seems another approach 
could be to only serialize the methods declared in that module itself, and 
during deserialization we would have to load the methods from all dependent 
modules.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 373072.
rmaz added a comment.

Update with single DenseSet per GlobalMethodPool


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDeclObjC.cpp


Index: clang/lib/Sema/SemaDeclObjC.cpp
===
--- clang/lib/Sema/SemaDeclObjC.cpp
+++ clang/lib/Sema/SemaDeclObjC.cpp
@@ -3302,6 +3302,10 @@
 
 void Sema::addMethodToGlobalList(ObjCMethodList *List,
  ObjCMethodDecl *Method) {
+  // Do not insert duplicate methods into the method pool.
+  if (!MethodPool.addMethod(Method))
+return;
+
   // Record at the head of the list whether there were 0, 1, or >= 2 methods
   // inside categories.
   if (ObjCCategoryDecl *CD =
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -1431,9 +1431,13 @@
 }
 int count(Selector Sel) const { return Methods.count(Sel); }
 bool empty() const { return Methods.empty(); }
+bool addMethod(ObjCMethodDecl *Method) {
+  return AddedMethods.insert(Method).second;
+}
 
   private:
 llvm::DenseMap Methods;
+llvm::DenseSet AddedMethods;
   };
 
   /// Method Pool - allows efficient lookup when typechecking messages to "id".


Index: clang/lib/Sema/SemaDeclObjC.cpp
===
--- clang/lib/Sema/SemaDeclObjC.cpp
+++ clang/lib/Sema/SemaDeclObjC.cpp
@@ -3302,6 +3302,10 @@
 
 void Sema::addMethodToGlobalList(ObjCMethodList *List,
  ObjCMethodDecl *Method) {
+  // Do not insert duplicate methods into the method pool.
+  if (!MethodPool.addMethod(Method))
+return;
+
   // Record at the head of the list whether there were 0, 1, or >= 2 methods
   // inside categories.
   if (ObjCCategoryDecl *CD =
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -1431,9 +1431,13 @@
 }
 int count(Selector Sel) const { return Methods.count(Sel); }
 bool empty() const { return Methods.empty(); }
+bool addMethod(ObjCMethodDecl *Method) {
+  return AddedMethods.insert(Method).second;
+}
 
   private:
 llvm::DenseMap Methods;
+llvm::DenseSet AddedMethods;
   };
 
   /// Method Pool - allows efficient lookup when typechecking messages to "id".
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109898: [clang][NFC] refactor GlobalMethodPool to encapsulate its map

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 372999.
rmaz added a comment.

- keep `std::pair`, but move to 
`GlobalMethodPool::Lists`
- add `const`
- `val` -> `&`
- move comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109898/new/

https://reviews.llvm.org/D109898

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Serialization/ASTReader.cpp


Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -8208,8 +8208,9 @@
 return;
 
   Sema  = *getSema();
-  Sema::GlobalMethodPool::iterator Pos
-= S.MethodPool.insert(std::make_pair(Sel, Sema::GlobalMethods())).first;
+  Sema::GlobalMethodPool::iterator Pos =
+  S.MethodPool.insert(std::make_pair(Sel, Sema::GlobalMethodPool::Lists()))
+  .first;
 
   Pos->second.first.setBits(Visitor.getInstanceBits());
   
Pos->second.first.setHasMoreThanOneDecl(Visitor.instanceHasMoreThanOneDecl());
Index: clang/lib/Sema/SemaDeclObjC.cpp
===
--- clang/lib/Sema/SemaDeclObjC.cpp
+++ clang/lib/Sema/SemaDeclObjC.cpp
@@ -3427,8 +3427,10 @@
 
   GlobalMethodPool::iterator Pos = MethodPool.find(Method->getSelector());
   if (Pos == MethodPool.end())
-Pos = MethodPool.insert(std::make_pair(Method->getSelector(),
-   GlobalMethods())).first;
+Pos = MethodPool
+  .insert(std::make_pair(Method->getSelector(),
+ GlobalMethodPool::Lists()))
+  .first;
 
   Method->setDefined(impl);
 
@@ -3636,7 +3638,7 @@
   if (Pos == MethodPool.end())
 return nullptr;
 
-  GlobalMethods  = Pos->second;
+  GlobalMethodPool::Lists  = Pos->second;
   for (const ObjCMethodList *Method =  Method;
Method = Method->getNext())
 if (Method->getMethod() &&
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -1419,8 +1419,22 @@
   const llvm::MapVector &
   getMismatchingDeleteExpressions() const;
 
-  typedef std::pair GlobalMethods;
-  typedef llvm::DenseMap GlobalMethodPool;
+  class GlobalMethodPool {
+  public:
+using Lists = std::pair;
+using iterator = llvm::DenseMap::iterator;
+iterator begin() { return Methods.begin(); }
+iterator end() { return Methods.end(); }
+iterator find(Selector Sel) { return Methods.find(Sel); }
+std::pair insert(std::pair &) {
+  return Methods.insert(Val);
+}
+int count(Selector Sel) const { return Methods.count(Sel); }
+bool empty() const { return Methods.empty(); }
+
+  private:
+llvm::DenseMap Methods;
+  };
 
   /// Method Pool - allows efficient lookup when typechecking messages to "id".
   /// We need to maintain a list, since selectors can have differing signatures


Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -8208,8 +8208,9 @@
 return;
 
   Sema  = *getSema();
-  Sema::GlobalMethodPool::iterator Pos
-= S.MethodPool.insert(std::make_pair(Sel, Sema::GlobalMethods())).first;
+  Sema::GlobalMethodPool::iterator Pos =
+  S.MethodPool.insert(std::make_pair(Sel, Sema::GlobalMethodPool::Lists()))
+  .first;
 
   Pos->second.first.setBits(Visitor.getInstanceBits());
   Pos->second.first.setHasMoreThanOneDecl(Visitor.instanceHasMoreThanOneDecl());
Index: clang/lib/Sema/SemaDeclObjC.cpp
===
--- clang/lib/Sema/SemaDeclObjC.cpp
+++ clang/lib/Sema/SemaDeclObjC.cpp
@@ -3427,8 +3427,10 @@
 
   GlobalMethodPool::iterator Pos = MethodPool.find(Method->getSelector());
   if (Pos == MethodPool.end())
-Pos = MethodPool.insert(std::make_pair(Method->getSelector(),
-   GlobalMethods())).first;
+Pos = MethodPool
+  .insert(std::make_pair(Method->getSelector(),
+ GlobalMethodPool::Lists()))
+  .first;
 
   Method->setDefined(impl);
 
@@ -3636,7 +3638,7 @@
   if (Pos == MethodPool.end())
 return nullptr;
 
-  GlobalMethods  = Pos->second;
+  GlobalMethodPool::Lists  = Pos->second;
   for (const ObjCMethodList *Method =  Method;
Method = Method->getNext())
 if (Method->getMethod() &&
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -1419,8 +1419,22 @@
   const llvm::MapVector &
   getMismatchingDeleteExpressions() const;
 
-  typedef std::pair GlobalMethods;
-  typedef llvm::DenseMap GlobalMethodPool;
+  class 

[PATCH] D109898: [clang][NFC] refactor GlobalMethodPool to encapsulate its map

[Richard Howell via Phabricator via cfe-commits]

rmaz created this revision.
rmaz added reviewers: dexonsmith, manmanren, vsapsai.
rmaz requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This refactor changes the GlobalMethodPool to a class that contains
the DenseMap of methods. This is to allow for the addition of a
separate DenseSet in a follow-up diff that will handle method
de-duplication when inserting methods into the global method pool.

Changes:

- the GlobalMethods pair becomes a GlobalMethodLists struct for better clarity
- the GlobalMethodPool becomes a class containing the DenseMap of methods
- pass through methods are added to maintain most of the existing code without 
changing MethodPool -> MethodPool.Methods everywhere


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D109898

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Sema/SemaExprObjC.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp

Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -3122,8 +3122,8 @@
 ObjCMethodList()
   };
   if (F != SemaRef.MethodPool.end()) {
-Data.Instance = F->second.first;
-Data.Factory = F->second.second;
+Data.Instance = F->second.InstanceMethods;
+Data.Factory = F->second.ClassMethods;
   }
   // Only write this selector if it's not in an existing AST or something
   // changed.
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -4006,8 +4006,9 @@
 return;
 
   // Retrieve the appropriate method list.
-  ObjCMethodList  = Method->isInstanceMethod()? Known->second.first
-: Known->second.second;
+  ObjCMethodList  = Method->isInstanceMethod()
+  ? Known->second.InstanceMethods
+  : Known->second.ClassMethods;
   bool Found = false;
   for (ObjCMethodList *List =  List; List = List->getNext()) {
 if (!Found) {
@@ -8208,19 +8209,22 @@
 return;
 
   Sema  = *getSema();
-  Sema::GlobalMethodPool::iterator Pos
-= S.MethodPool.insert(std::make_pair(Sel, Sema::GlobalMethods())).first;
+  Sema::GlobalMethodPool::iterator Pos =
+  S.MethodPool.insert(std::make_pair(Sel, Sema::GlobalMethodLists())).first;
 
-  Pos->second.first.setBits(Visitor.getInstanceBits());
-  Pos->second.first.setHasMoreThanOneDecl(Visitor.instanceHasMoreThanOneDecl());
-  Pos->second.second.setBits(Visitor.getFactoryBits());
-  Pos->second.second.setHasMoreThanOneDecl(Visitor.factoryHasMoreThanOneDecl());
+  Pos->second.InstanceMethods.setBits(Visitor.getInstanceBits());
+  Pos->second.InstanceMethods.setHasMoreThanOneDecl(
+  Visitor.instanceHasMoreThanOneDecl());
+  Pos->second.ClassMethods.setBits(Visitor.getFactoryBits());
+  Pos->second.ClassMethods.setHasMoreThanOneDecl(
+  Visitor.factoryHasMoreThanOneDecl());
 
   // Add methods to the global pool *after* setting hasMoreThanOneDecl, since
   // when building a module we keep every method individually and may need to
   // update hasMoreThanOneDecl as we add the methods.
-  addMethodsToPool(S, Visitor.getInstanceMethods(), Pos->second.first);
-  addMethodsToPool(S, Visitor.getFactoryMethods(), Pos->second.second);
+  addMethodsToPool(S, Visitor.getInstanceMethods(),
+   Pos->second.InstanceMethods);
+  addMethodsToPool(S, Visitor.getFactoryMethods(), Pos->second.ClassMethods);
 }
 
 void ASTReader::updateOutOfDateSelector(Selector Sel) {
Index: clang/lib/Sema/SemaExprObjC.cpp
===
--- clang/lib/Sema/SemaExprObjC.cpp
+++ clang/lib/Sema/SemaExprObjC.cpp
@@ -1215,13 +1215,13 @@
   for (Sema::GlobalMethodPool::iterator b = S.MethodPool.begin(),
e = S.MethodPool.end(); b != e; b++) {
 // first, instance methods
-ObjCMethodList  = b->second.first;
+ObjCMethodList  = b->second.InstanceMethods;
 if (HelperToDiagnoseMismatchedMethodsInGlobalPool(S, AtLoc, LParenLoc, RParenLoc,
   Method, InstMethList))
   Warned = true;
 
 // second, class methods
-ObjCMethodList  = b->second.second;
+ObjCMethodList  = b->second.ClassMethods;
 if (HelperToDiagnoseMismatchedMethodsInGlobalPool(S, AtLoc, LParenLoc, RParenLoc,
   Method, ClsMethList) || Warned)
   return;
@@ -1262,9 +1262,9 @@
 return nullptr;
 
   ObjCMethodDecl *DirectInstance = LookupDirectMethodInMethodList(
-  S, Sel, Iter->second.first, onlyDirect, anyDirect);
+  S, Sel, 

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:1423-1426
+ObjCMethodList InstanceMethods;
+ObjCMethodList FactoryMethods;
+llvm::DenseSet AddedInstanceMethods;
+llvm::DenseSet AddedFactoryMethods;

dexonsmith wrote:
> Do these two sets really need to be per-selector (per `GlobalMethods` object 
> in GlobalMethodPool)? I feel like they could be global to `Sema`, as long as 
> the same `ObjCMethodDecl` is never added to GlobalMethodPool for more than 
> one selector. (It seems plausible we have that property since presumably 
> ObjCMethodDecl::getSelector is always the key used for inserting into 
> GlobalMethodPool below... can you confirm?)
> 
> Assuming you can pull the sets out to represent the GlobalMethodPool as a 
> whole, then I suggest creating a data structure for GlobalMethodPool that 
> encapsulates the DenseMap and the two sets (maybe: rename "GlobalMethods" to 
> "GlobalMethodLists", rename "GlobalMethodPool" to "GlobalMethods", and give 
> the new data structure the name "GlobalMethodPool"). Best to do it as 
> multiple commits: NFC commit(s) to refactor (renames, create new type update 
> call sites to use new APIs, etc.), and a final commit that changes the 
> functionality (adding the set behaviour) but that doesn't need to touch call 
> sites.
> 
> On the other hand, if the sets really need to be per-selector (please explain 
> why if so...), please use SmallPtrSet here instead of DenseSet to avoid 
> allocation in the common case of 1 decl per selector. I'd suggest 
> encapsulating the list/set together somehow (maybe starting with an NFC 
> refactoring to add a "list/set" data structure at the top-level (maybe rename 
> ObjCMethodList => ObjCMethodListNode, and the new list has a private member 
> called "Head" and necessary APIs for insertion/etc), then in a second commit 
> adding the SmallPtrSet into the list data structure and use it to gate the 
> "insert" operation).
> 
> I feel like they could be global to Sema, as long as the same ObjCMethodDecl 
> is never added to GlobalMethodPool for more than one selector. (It seems 
> plausible we have that property since presumably ObjCMethodDecl::getSelector 
> is always the key used for inserting into GlobalMethodPool below... can you 
> confirm?)

Yes, that is correct. We could also use a single set instead of two as the 
instance and factory methods would have different decls anyway.

> Assuming you can pull the sets out to represent the GlobalMethodPool as a 
> whole, then I suggest creating a data structure for GlobalMethodPool that 
> encapsulates the DenseMap and the two sets

I'll go with this approach with the single set, starting with a data structure 
refactor diff.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 372598.
rmaz added a comment.

update to fix lint warnings


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Sema/SemaExprObjC.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp

Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -3122,8 +3122,8 @@
 ObjCMethodList()
   };
   if (F != SemaRef.MethodPool.end()) {
-Data.Instance = F->second.first;
-Data.Factory = F->second.second;
+Data.Instance = F->second.InstanceMethods;
+Data.Factory = F->second.FactoryMethods;
   }
   // Only write this selector if it's not in an existing AST or something
   // changed.
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -4006,8 +4006,9 @@
 return;
 
   // Retrieve the appropriate method list.
-  ObjCMethodList  = Method->isInstanceMethod()? Known->second.first
-: Known->second.second;
+  ObjCMethodList  = Method->isInstanceMethod()
+  ? Known->second.InstanceMethods
+  : Known->second.FactoryMethods;
   bool Found = false;
   for (ObjCMethodList *List =  List; List = List->getNext()) {
 if (!Found) {
@@ -8180,9 +8181,10 @@
 
 /// Add the given set of methods to the method list.
 static void addMethodsToPool(Sema , ArrayRef Methods,
- ObjCMethodList ) {
-  for (unsigned I = 0, N = Methods.size(); I != N; ++I) {
-S.addMethodToGlobalList(, Methods[I]);
+ ObjCMethodList ,
+ llvm::DenseSet ) {
+  for (auto *M : Methods) {
+S.addMethodToGlobalList(, , M);
   }
 }
 
@@ -8211,16 +8213,20 @@
   Sema::GlobalMethodPool::iterator Pos
 = S.MethodPool.insert(std::make_pair(Sel, Sema::GlobalMethods())).first;
 
-  Pos->second.first.setBits(Visitor.getInstanceBits());
-  Pos->second.first.setHasMoreThanOneDecl(Visitor.instanceHasMoreThanOneDecl());
-  Pos->second.second.setBits(Visitor.getFactoryBits());
-  Pos->second.second.setHasMoreThanOneDecl(Visitor.factoryHasMoreThanOneDecl());
+  Pos->second.InstanceMethods.setBits(Visitor.getInstanceBits());
+  Pos->second.InstanceMethods.setHasMoreThanOneDecl(
+  Visitor.instanceHasMoreThanOneDecl());
+  Pos->second.FactoryMethods.setBits(Visitor.getFactoryBits());
+  Pos->second.FactoryMethods.setHasMoreThanOneDecl(
+  Visitor.factoryHasMoreThanOneDecl());
 
   // Add methods to the global pool *after* setting hasMoreThanOneDecl, since
   // when building a module we keep every method individually and may need to
   // update hasMoreThanOneDecl as we add the methods.
-  addMethodsToPool(S, Visitor.getInstanceMethods(), Pos->second.first);
-  addMethodsToPool(S, Visitor.getFactoryMethods(), Pos->second.second);
+  addMethodsToPool(S, Visitor.getInstanceMethods(), Pos->second.InstanceMethods,
+   Pos->second.AddedInstanceMethods);
+  addMethodsToPool(S, Visitor.getFactoryMethods(), Pos->second.FactoryMethods,
+   Pos->second.AddedFactoryMethods);
 }
 
 void ASTReader::updateOutOfDateSelector(Selector Sel) {
Index: clang/lib/Sema/SemaExprObjC.cpp
===
--- clang/lib/Sema/SemaExprObjC.cpp
+++ clang/lib/Sema/SemaExprObjC.cpp
@@ -1215,13 +1215,13 @@
   for (Sema::GlobalMethodPool::iterator b = S.MethodPool.begin(),
e = S.MethodPool.end(); b != e; b++) {
 // first, instance methods
-ObjCMethodList  = b->second.first;
+ObjCMethodList  = b->second.InstanceMethods;
 if (HelperToDiagnoseMismatchedMethodsInGlobalPool(S, AtLoc, LParenLoc, RParenLoc,
   Method, InstMethList))
   Warned = true;
 
 // second, class methods
-ObjCMethodList  = b->second.second;
+ObjCMethodList  = b->second.FactoryMethods;
 if (HelperToDiagnoseMismatchedMethodsInGlobalPool(S, AtLoc, LParenLoc, RParenLoc,
   Method, ClsMethList) || Warned)
   return;
@@ -1262,9 +1262,9 @@
 return nullptr;
 
   ObjCMethodDecl *DirectInstance = LookupDirectMethodInMethodList(
-  S, Sel, Iter->second.first, onlyDirect, anyDirect);
+  S, Sel, Iter->second.InstanceMethods, onlyDirect, anyDirect);
   ObjCMethodDecl *DirectClass = LookupDirectMethodInMethodList(
-  S, Sel, Iter->second.second, onlyDirect, anyDirect);
+  S, Sel, 

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz updated this revision to Diff 372597.
rmaz added a comment.

Updated to use a GlobalMethods struct which contains the global
method lists as well as the sets used to de-duplicate added methods.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Sema/SemaExprObjC.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp

Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -3122,8 +3122,8 @@
 ObjCMethodList()
   };
   if (F != SemaRef.MethodPool.end()) {
-Data.Instance = F->second.first;
-Data.Factory = F->second.second;
+Data.Instance = F->second.InstanceMethods;
+Data.Factory = F->second.FactoryMethods;
   }
   // Only write this selector if it's not in an existing AST or something
   // changed.
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -4006,8 +4006,9 @@
 return;
 
   // Retrieve the appropriate method list.
-  ObjCMethodList  = Method->isInstanceMethod()? Known->second.first
-: Known->second.second;
+  ObjCMethodList  = Method->isInstanceMethod()
+  ? Known->second.InstanceMethods
+  : Known->second.FactoryMethods;
   bool Found = false;
   for (ObjCMethodList *List =  List; List = List->getNext()) {
 if (!Found) {
@@ -8180,9 +8181,10 @@
 
 /// Add the given set of methods to the method list.
 static void addMethodsToPool(Sema , ArrayRef Methods,
- ObjCMethodList ) {
-  for (unsigned I = 0, N = Methods.size(); I != N; ++I) {
-S.addMethodToGlobalList(, Methods[I]);
+ ObjCMethodList ,
+ llvm::DenseSet ) {
+  for (auto *M : Methods) {
+S.addMethodToGlobalList(, , M);
   }
 }
 
@@ -8211,16 +8213,20 @@
   Sema::GlobalMethodPool::iterator Pos
 = S.MethodPool.insert(std::make_pair(Sel, Sema::GlobalMethods())).first;
 
-  Pos->second.first.setBits(Visitor.getInstanceBits());
-  Pos->second.first.setHasMoreThanOneDecl(Visitor.instanceHasMoreThanOneDecl());
-  Pos->second.second.setBits(Visitor.getFactoryBits());
-  Pos->second.second.setHasMoreThanOneDecl(Visitor.factoryHasMoreThanOneDecl());
+  Pos->second.InstanceMethods.setBits(Visitor.getInstanceBits());
+  Pos->second.InstanceMethods.setHasMoreThanOneDecl(
+  Visitor.instanceHasMoreThanOneDecl());
+  Pos->second.FactoryMethods.setBits(Visitor.getFactoryBits());
+  Pos->second.FactoryMethods.setHasMoreThanOneDecl(
+  Visitor.factoryHasMoreThanOneDecl());
 
   // Add methods to the global pool *after* setting hasMoreThanOneDecl, since
   // when building a module we keep every method individually and may need to
   // update hasMoreThanOneDecl as we add the methods.
-  addMethodsToPool(S, Visitor.getInstanceMethods(), Pos->second.first);
-  addMethodsToPool(S, Visitor.getFactoryMethods(), Pos->second.second);
+  addMethodsToPool(S, Visitor.getInstanceMethods(), Pos->second.InstanceMethods,
+   Pos->second.AddedInstanceMethods);
+  addMethodsToPool(S, Visitor.getFactoryMethods(), Pos->second.FactoryMethods,
+   Pos->second.AddedFactoryMethods);
 }
 
 void ASTReader::updateOutOfDateSelector(Selector Sel) {
Index: clang/lib/Sema/SemaExprObjC.cpp
===
--- clang/lib/Sema/SemaExprObjC.cpp
+++ clang/lib/Sema/SemaExprObjC.cpp
@@ -1215,13 +1215,13 @@
   for (Sema::GlobalMethodPool::iterator b = S.MethodPool.begin(),
e = S.MethodPool.end(); b != e; b++) {
 // first, instance methods
-ObjCMethodList  = b->second.first;
+ObjCMethodList  = b->second.InstanceMethods;
 if (HelperToDiagnoseMismatchedMethodsInGlobalPool(S, AtLoc, LParenLoc, RParenLoc,
   Method, InstMethList))
   Warned = true;
 
 // second, class methods
-ObjCMethodList  = b->second.second;
+ObjCMethodList  = b->second.FactoryMethods;
 if (HelperToDiagnoseMismatchedMethodsInGlobalPool(S, AtLoc, LParenLoc, RParenLoc,
   Method, ClsMethList) || Warned)
   return;
@@ -1262,9 +1262,9 @@
 return nullptr;
 
   ObjCMethodDecl *DirectInstance = LookupDirectMethodInMethodList(
-  S, Sel, Iter->second.first, onlyDirect, anyDirect);
+  S, Sel, Iter->second.InstanceMethods, onlyDirect, anyDirect);
   ObjCMethodDecl *DirectClass = 

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added a comment.

In D109632#3000469 , @vsapsai wrote:

> I'm a little bit confused here. Where is the speed-up coming from? Is it 
> because ObjCMethodList for `init` not being serialized? I'm asking because 
> right now I don't see how `seen` helps with that.
>
> My current understanding is that `Sema::addMethodToGlobalList` is too 
> expensive and `seen` reduces the number of calls to it. So maybe it makes 
> sense to invest into making it faster? For example, 
> `Sema::MatchTwoMethodDeclarations` can return early if both method decl 
> pointers are equal. But I haven't done any measurements, that's just an 
> example.

The speedup is coming from reducing the number of times 
`Sema::addMethodToGlobalList` is called when looking up methods loaded from 
modules. From what I can see each serialized module will contain an 
ObjCMethodList with all the methods from all visible modules at the point of 
serialization. This means that if you have a lot of modules that redeclare 
`-(id)init`, which is a common pattern in our code, then each serialized module 
will contain a lot of shared method decls, which we do not de-duplicate. To 
illustrate this, here are the top 5 instance method counts sorted by amount of 
duplicates (using pointer comparison) from a pass of the 
`ReadMethodPoolVisitor` for a single file:

| **selector** | **# methods** | **# unique** | **# duplicated** |
| init | 9825  | 1652 | 8173 |
| init | 2155  | 930  | 1225 |
| copy | 1398  | 633  | 765  |
| init | 1027  | 270  | 757  |
| init | 948   | 410  | 538  |
|

Without having a set to deduplicate I'm not sure how we could make 
`Sema::addMethodToGlobalList` fast enough, wouldn't it require a list traversal 
for each insert, making it O(n^2)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:8188-8190
+  for (auto *L =  L; L = L->getNext()) {
+seen.insert(L->getMethod());
+  }

dexonsmith wrote:
> I find quadratic algorithms a bit scary, even when current benchmarks don't 
> expose problems. For example, it seems like this could blow up on the 
> following workload:
> - one module adds many things to the global method list
> - there are many (fine-grained) modules that transitively load that module
> 
> Or have I misunderstood the complexity here?
Yes, I take your point, if the method pool generation updates inbetween each of 
the later modules then it is possible to hit this case.



Comment at: clang/lib/Serialization/ASTReader.cpp:8194
+if (seen.insert(M).second) {
+  S.addMethodToGlobalList(, M);
+}

dexonsmith wrote:
> rmaz wrote:
> > manmanren wrote:
> > > Does it make sense to check for duplication inside addMethodToGlobalList, 
> > > as the function goes through the list as well? Maybe it is slower, as we 
> > > will need to go through the list for each method, instead of a lookup.
> > Yes, you are right, it is slower as we need to do a list traverse per 
> > insert rather than per selector lookup. I also profiled keeping some global 
> > state along with the `MethodPool` so that the set didn't have to be rebuilt 
> > each time, but the performance difference was negligible.
> Can you take another look at the approach you experimented with, putting the 
> global state in the MethodPool? Besides the performance difference (you 
> measured it as negligible but it'd avoid the concern I have about uncommon 
> workloads hitting quadratic blowups), it'd also provide consistency in 
> behaviour between callers of `addMethodToGlobalList`... and enable you to add 
> a unit test for the API change to MethodPool.
I'll update with this approach, it should allow for moving the set insert logic 
into `addMethodToGlobalList` in this case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:8194
+if (seen.insert(M).second) {
+  S.addMethodToGlobalList(, M);
+}

manmanren wrote:
> Does it make sense to check for duplication inside addMethodToGlobalList, as 
> the function goes through the list as well? Maybe it is slower, as we will 
> need to go through the list for each method, instead of a lookup.
Yes, you are right, it is slower as we need to do a list traverse per insert 
rather than per selector lookup. I also profiled keeping some global state 
along with the `MethodPool` so that the set didn't have to be rebuilt each 
time, but the performance difference was negligible.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109632/new/

https://reviews.llvm.org/D109632

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109632: [clang] de-duplicate methods from AST files

[Richard Howell via Phabricator via cfe-commits]

rmaz created this revision.
rmaz requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This diff will de-duplicate methods read from AST files before
inserting them in to a global method pool list. When reading
ObjCMethodDecl from AST files we can end up with a significant
amount of duplication when modules contain redeclarations of
system framework methods. For instance a common pattern is to
redeclare `-(instancetype)init` with `NS_UNAVAILABLE`, which
results in the entire ObjCMethodList for `init` being serialized
in each module with this redeclaration.

Measuring this against our codebase for files that use `-fmodules`
shows an overall 19% compile time improvement, and in some cases
as much as 79% for files with a lot of modular dependencies.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D109632

Files:
  clang/lib/Serialization/ASTReader.cpp


Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -8181,8 +8181,18 @@
 /// Add the given set of methods to the method list.
 static void addMethodsToPool(Sema , ArrayRef Methods,
  ObjCMethodList ) {
-  for (unsigned I = 0, N = Methods.size(); I != N; ++I) {
-S.addMethodToGlobalList(, Methods[I]);
+  // Methods from visited modules can contain a lot of duplicates
+  // when redeclaring methods from system frameworks, for example
+  // when marking -(instancetype)init as NS_UNAVAILABLE.
+  llvm::DenseSet seen;
+  for (auto *L =  L; L = L->getNext()) {
+seen.insert(L->getMethod());
+  }
+
+  for (auto *M : Methods) {
+if (seen.insert(M).second) {
+  S.addMethodToGlobalList(, M);
+}
   }
 }
 


Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -8181,8 +8181,18 @@
 /// Add the given set of methods to the method list.
 static void addMethodsToPool(Sema , ArrayRef Methods,
  ObjCMethodList ) {
-  for (unsigned I = 0, N = Methods.size(); I != N; ++I) {
-S.addMethodToGlobalList(, Methods[I]);
+  // Methods from visited modules can contain a lot of duplicates
+  // when redeclaring methods from system frameworks, for example
+  // when marking -(instancetype)init as NS_UNAVAILABLE.
+  llvm::DenseSet seen;
+  for (auto *L =  L; L = L->getNext()) {
+seen.insert(L->getMethod());
+  }
+
+  for (auto *M : Methods) {
+if (seen.insert(M).second) {
+  S.addMethodToGlobalList(, M);
+}
   }
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits