[Lldb-commits] [PATCH] D121494: [VFS] Rename `RedirectingFileSystem::dump` to `print`

[Ben Barham via Phabricator via lldb-commits]

bnbarham created this revision.
bnbarham added a reviewer: dexonsmith.
Herald added a project: All.
bnbarham requested review of this revision.
Herald added projects: LLDB, LLVM.
Herald added subscribers: llvm-commits, lldb-commits.

The rest of LLVM uses `print` for the method taking the `raw_ostream`
and `dump` only for the method with no parameters. Use the same for
`RedirectingFileSystem`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121494

Files:
  lldb/source/Commands/CommandObjectReproducer.cpp
  llvm/include/llvm/Support/VirtualFileSystem.h


Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -910,10 +910,10 @@
 
   std::vector getRoots() const;
 
-  void dump(raw_ostream &OS) const;
-  void dumpEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
+  void print(raw_ostream &OS) const;
+  void printEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
-  LLVM_DUMP_METHOD void dump() const;
+  LLVM_DUMP_METHOD void dump() const { print(dbgs()); }
 #endif
 };
 
Index: lldb/source/Commands/CommandObjectReproducer.cpp
===
--- lldb/source/Commands/CommandObjectReproducer.cpp
+++ lldb/source/Commands/CommandObjectReproducer.cpp
@@ -425,7 +425,7 @@
   // Dump the VFS to a buffer.
   std::string str;
   raw_string_ostream os(str);
-  static_cast(*vfs).dump(os);
+  static_cast(*vfs).print(os);
   os.flush();
 
   // Return the string.


Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -910,10 +910,10 @@
 
   std::vector getRoots() const;
 
-  void dump(raw_ostream &OS) const;
-  void dumpEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
+  void print(raw_ostream &OS) const;
+  void printEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
-  LLVM_DUMP_METHOD void dump() const;
+  LLVM_DUMP_METHOD void dump() const { print(dbgs()); }
 #endif
 };
 
Index: lldb/source/Commands/CommandObjectReproducer.cpp
===
--- lldb/source/Commands/CommandObjectReproducer.cpp
+++ lldb/source/Commands/CommandObjectReproducer.cpp
@@ -425,7 +425,7 @@
   // Dump the VFS to a buffer.
   std::string str;
   raw_string_ostream os(str);
-  static_cast(*vfs).dump(os);
+  static_cast(*vfs).print(os);
   os.flush();
 
   // Return the string.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D121494: [VFS] Rename `RedirectingFileSystem::dump` to `print`

[Ben Barham via Phabricator via lldb-commits]

bnbarham added a comment.
Herald added a subscriber: JDevlieghere.

@dexonsmith it'll be faster to run the tests here than locally, so I figured 
I'd put the review up anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121494

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

[Lldb-commits] [PATCH] D121494: [VFS] Rename `RedirectingFileSystem::dump` to `print`

[Ben Barham via Phabricator via lldb-commits]

bnbarham updated this revision to Diff 414763.
bnbarham added a comment.
Herald added a subscriber: hiraditya.

Added VFS.cpp, removed implementation from header


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121494

Files:
  lldb/source/Commands/CommandObjectReproducer.cpp
  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
@@ -1381,14 +1381,14 @@
   return R;
 }
 
-void RedirectingFileSystem::dump(raw_ostream &OS) const {
+void RedirectingFileSystem::print(raw_ostream &OS) const {
   for (const auto &Root : Roots)
-dumpEntry(OS, Root.get());
+printEntry(OS, Root.get());
 }
 
-void RedirectingFileSystem::dumpEntry(raw_ostream &OS,
-  RedirectingFileSystem::Entry *E,
-  int NumSpaces) const {
+void RedirectingFileSystem::printEntry(raw_ostream &OS,
+   RedirectingFileSystem::Entry *E,
+   int NumSpaces) const {
   StringRef Name = E->getName();
   for (int i = 0, e = NumSpaces; i < e; ++i)
 OS << " ";
@@ -1401,12 +1401,12 @@
 
 for (std::unique_ptr &SubEntry :
  llvm::make_range(DE->contents_begin(), DE->contents_end()))
-  dumpEntry(OS, SubEntry.get(), NumSpaces + 2);
+  printEntry(OS, SubEntry.get(), NumSpaces + 2);
   }
 }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
-LLVM_DUMP_METHOD void RedirectingFileSystem::dump() const { dump(dbgs()); }
+LLVM_DUMP_METHOD void RedirectingFileSystem::dump() const { print(dbgs()); }
 #endif
 
 /// A helper class to hold the common YAML parsing state.
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -910,11 +910,9 @@
 
   std::vector getRoots() const;
 
-  void dump(raw_ostream &OS) const;
-  void dumpEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
-#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+  void print(raw_ostream &OS) const;
+  void printEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
   LLVM_DUMP_METHOD void dump() const;
-#endif
 };
 
 /// Collect all pairs of  entries from the
Index: lldb/source/Commands/CommandObjectReproducer.cpp
===
--- lldb/source/Commands/CommandObjectReproducer.cpp
+++ lldb/source/Commands/CommandObjectReproducer.cpp
@@ -425,7 +425,7 @@
   // Dump the VFS to a buffer.
   std::string str;
   raw_string_ostream os(str);
-  static_cast(*vfs).dump(os);
+  static_cast(*vfs).print(os);
   os.flush();
 
   // Return the string.


Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1381,14 +1381,14 @@
   return R;
 }
 
-void RedirectingFileSystem::dump(raw_ostream &OS) const {
+void RedirectingFileSystem::print(raw_ostream &OS) const {
   for (const auto &Root : Roots)
-dumpEntry(OS, Root.get());
+printEntry(OS, Root.get());
 }
 
-void RedirectingFileSystem::dumpEntry(raw_ostream &OS,
-  RedirectingFileSystem::Entry *E,
-  int NumSpaces) const {
+void RedirectingFileSystem::printEntry(raw_ostream &OS,
+   RedirectingFileSystem::Entry *E,
+   int NumSpaces) const {
   StringRef Name = E->getName();
   for (int i = 0, e = NumSpaces; i < e; ++i)
 OS << " ";
@@ -1401,12 +1401,12 @@
 
 for (std::unique_ptr &SubEntry :
  llvm::make_range(DE->contents_begin(), DE->contents_end()))
-  dumpEntry(OS, SubEntry.get(), NumSpaces + 2);
+  printEntry(OS, SubEntry.get(), NumSpaces + 2);
   }
 }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
-LLVM_DUMP_METHOD void RedirectingFileSystem::dump() const { dump(dbgs()); }
+LLVM_DUMP_METHOD void RedirectingFileSystem::dump() const { print(dbgs()); }
 #endif
 
 /// A helper class to hold the common YAML parsing state.
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -910,11 +910,9 @@
 
   std::vector getRoots() const;
 
-  void dump(raw_ostream &OS) const;
-  void dumpEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
-#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+  void print(raw_ostream &OS) const;
+  void printEntr

[Lldb-commits] [PATCH] D121494: [VFS] Rename `RedirectingFileSystem::dump` to `print`

[Ben Barham via Phabricator via lldb-commits]

bnbarham updated this revision to Diff 414770.
bnbarham added a comment.

Removed LLVM_DUMP_METHOD from .cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121494

Files:
  lldb/source/Commands/CommandObjectReproducer.cpp
  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
@@ -1381,14 +1381,14 @@
   return R;
 }
 
-void RedirectingFileSystem::dump(raw_ostream &OS) const {
+void RedirectingFileSystem::print(raw_ostream &OS) const {
   for (const auto &Root : Roots)
-dumpEntry(OS, Root.get());
+printEntry(OS, Root.get());
 }
 
-void RedirectingFileSystem::dumpEntry(raw_ostream &OS,
-  RedirectingFileSystem::Entry *E,
-  int NumSpaces) const {
+void RedirectingFileSystem::printEntry(raw_ostream &OS,
+   RedirectingFileSystem::Entry *E,
+   int NumSpaces) const {
   StringRef Name = E->getName();
   for (int i = 0, e = NumSpaces; i < e; ++i)
 OS << " ";
@@ -1401,12 +1401,12 @@
 
 for (std::unique_ptr &SubEntry :
  llvm::make_range(DE->contents_begin(), DE->contents_end()))
-  dumpEntry(OS, SubEntry.get(), NumSpaces + 2);
+  printEntry(OS, SubEntry.get(), NumSpaces + 2);
   }
 }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
-LLVM_DUMP_METHOD void RedirectingFileSystem::dump() const { dump(dbgs()); }
+void RedirectingFileSystem::dump() const { print(dbgs()); }
 #endif
 
 /// A helper class to hold the common YAML parsing state.
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -910,8 +910,9 @@
 
   std::vector getRoots() const;
 
-  void dump(raw_ostream &OS) const;
-  void dumpEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
+  void print(raw_ostream &OS) const;
+  void printEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
+
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
   LLVM_DUMP_METHOD void dump() const;
 #endif
Index: lldb/source/Commands/CommandObjectReproducer.cpp
===
--- lldb/source/Commands/CommandObjectReproducer.cpp
+++ lldb/source/Commands/CommandObjectReproducer.cpp
@@ -425,7 +425,7 @@
   // Dump the VFS to a buffer.
   std::string str;
   raw_string_ostream os(str);
-  static_cast(*vfs).dump(os);
+  static_cast(*vfs).print(os);
   os.flush();
 
   // Return the string.


Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1381,14 +1381,14 @@
   return R;
 }
 
-void RedirectingFileSystem::dump(raw_ostream &OS) const {
+void RedirectingFileSystem::print(raw_ostream &OS) const {
   for (const auto &Root : Roots)
-dumpEntry(OS, Root.get());
+printEntry(OS, Root.get());
 }
 
-void RedirectingFileSystem::dumpEntry(raw_ostream &OS,
-  RedirectingFileSystem::Entry *E,
-  int NumSpaces) const {
+void RedirectingFileSystem::printEntry(raw_ostream &OS,
+   RedirectingFileSystem::Entry *E,
+   int NumSpaces) const {
   StringRef Name = E->getName();
   for (int i = 0, e = NumSpaces; i < e; ++i)
 OS << " ";
@@ -1401,12 +1401,12 @@
 
 for (std::unique_ptr &SubEntry :
  llvm::make_range(DE->contents_begin(), DE->contents_end()))
-  dumpEntry(OS, SubEntry.get(), NumSpaces + 2);
+  printEntry(OS, SubEntry.get(), NumSpaces + 2);
   }
 }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
-LLVM_DUMP_METHOD void RedirectingFileSystem::dump() const { dump(dbgs()); }
+void RedirectingFileSystem::dump() const { print(dbgs()); }
 #endif
 
 /// A helper class to hold the common YAML parsing state.
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -910,8 +910,9 @@
 
   std::vector getRoots() const;
 
-  void dump(raw_ostream &OS) const;
-  void dumpEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
+  void print(raw_ostream &OS) const;
+  void printEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
+
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
   LLVM_DUMP_METHOD void dump() const;
 #endif
Index: lldb/source/Commands/Comma

[Lldb-commits] [PATCH] D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API

[Ben Barham via Phabricator via lldb-commits]

bnbarham added inline comments.



Comment at: llvm/include/llvm/Support/Error.h:1284
 
-  StringRef getFileName() { return FileName; }
+  StringRef getFileName() const { return FileName; }
 

Should this be in a change all by itself?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121426

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

[Lldb-commits] [PATCH] D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API

[Ben Barham via Phabricator via lldb-commits]

bnbarham updated this revision to Diff 414718.
bnbarham added a comment.

Handle empty overlay file in clang tidy


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121426

Files:
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/VFS/Inputs/vfsroot.yaml
  clang/test/VFS/directory.c
  clang/test/VFS/multiple-overlays.c
  clang/test/VFS/vfsroot-with-overlay.c
  lldb/source/Commands/CommandObjectReproducer.cpp
  llvm/include/llvm/Support/Error.h
  llvm/tools/dsymutil/Reproducer.cpp
  llvm/tools/dsymutil/Reproducer.h

Index: llvm/tools/dsymutil/Reproducer.h
===
--- llvm/tools/dsymutil/Reproducer.h
+++ llvm/tools/dsymutil/Reproducer.h
@@ -59,8 +59,7 @@
 };
 
 /// Reproducer instance used to use an existing reproducer. The VFS returned by
-/// this instance is a RedirectingFileSystem that remaps paths to their
-/// counterpart in the reproducer.
+/// this instance remaps paths to their counterpart in the reproducer.
 class ReproducerUse : public Reproducer {
 public:
   ReproducerUse(StringRef Root, std::error_code &EC);
Index: llvm/tools/dsymutil/Reproducer.cpp
===
--- llvm/tools/dsymutil/Reproducer.cpp
+++ llvm/tools/dsymutil/Reproducer.cpp
@@ -48,15 +48,15 @@
 ReproducerUse::ReproducerUse(StringRef Root, std::error_code &EC) {
   SmallString<128> Mapping(Root);
   sys::path::append(Mapping, "mapping.yaml");
-  ErrorOr> Buffer =
-  vfs::getRealFileSystem()->getBufferForFile(Mapping.str());
 
-  if (!Buffer) {
-EC = Buffer.getError();
+  auto OverlayFS = llvm::vfs::getVFSFromYAMLs(Mapping.str());
+  if (auto Err = OverlayFS.takeError()) {
+llvm::handleAllErrors(std::move(Err), [&](const llvm::ErrorInfoBase &E) {
+  EC = E.convertToErrorCode();
+});
 return;
   }
-
-  VFS = llvm::vfs::getVFSFromYAML(std::move(Buffer.get()), nullptr, Mapping);
+  VFS = std::move(*OverlayFS);
 }
 
 llvm::Expected>
Index: llvm/include/llvm/Support/Error.h
===
--- llvm/include/llvm/Support/Error.h
+++ llvm/include/llvm/Support/Error.h
@@ -1281,7 +1281,7 @@
 return OS.str();
   }
 
-  StringRef getFileName() { return FileName; }
+  StringRef getFileName() const { return FileName; }
 
   Error takeError() { return Error(std::move(Err)); }
 
Index: lldb/source/Commands/CommandObjectReproducer.cpp
===
--- lldb/source/Commands/CommandObjectReproducer.cpp
+++ lldb/source/Commands/CommandObjectReproducer.cpp
@@ -409,23 +409,19 @@
 switch (m_options.provider) {
 case eReproducerProviderFiles: {
   FileSpec vfs_mapping = loader->GetFile();
+  std::string overlay_path = vfs_mapping.GetPath();
 
-  // Read the VFS mapping.
-  ErrorOr> buffer =
-  vfs::getRealFileSystem()->getBufferForFile(vfs_mapping.GetPath());
-  if (!buffer) {
-SetError(result, errorCodeToError(buffer.getError()));
+  Expected> vfs =
+  vfs::getVFSFromYAMLs(StringRef(overlay_path));
+  if (auto err = vfs.takeError()) {
+SetError(result, std::move(err));
 return false;
   }
 
-  // Initialize a VFS from the given mapping.
-  IntrusiveRefCntPtr vfs = vfs::getVFSFromYAML(
-  std::move(buffer.get()), nullptr, vfs_mapping.GetPath());
-
   // Dump the VFS to a buffer.
   std::string str;
   raw_string_ostream os(str);
-  static_cast(*vfs).dump(os);
+  (*vfs)->dump(os);
   os.flush();
 
   // Return the string.
Index: clang/test/VFS/vfsroot-with-overlay.c
===
--- clang/test/VFS/vfsroot-with-overlay.c
+++ clang/test/VFS/vfsroot-with-overlay.c
@@ -1,7 +1,7 @@
 // RUN: rm -rf %t
 // RUN: mkdir -p %t
 // RUN: sed -e "s@TEST_DIR@%{/S:regex_replacement}@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsroot.yaml > %t.yaml
-// RUN: sed -e "s@INPUT_DIR@/indirect-vfs-root-files@g" -e "s@OUT_DIR@/overlay-dir@g" %S/Inputs/vfsoverlay.yaml > %t/vfsoverlay.yaml
+// RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@/overlay-dir@g" %S/Inputs/vfsoverlay.yaml > %t/vfsoverlay.yaml
 // RUN: %clang_cc1 -Werror -ivfsoverlay %t.yaml -ivfsoverlay /direct-vfs-root-files/vfsoverlay.yaml -I /overlay-dir -fsyntax-only /tests/vfsroot-with-overlay.c
 
 #include "not_real.h"
Index: clang/test/VFS/multiple-overlays.c
===
--- /dev/null
+++ clang/test/VFS/multiple-overlays.c
@@ -0,0 +1,39 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s@EXTERNAL_DIR@%{/t:regex_replacement}/B@g" -e "s@NAME_DIR@%{/t:regex_replacement}/A@g" %t/vfs/base.yaml > %t/vfs/a-b.yaml
+// RUN: sed -e "s@EXTERNAL_DIR@%{/t:regex_re

[Lldb-commits] [PATCH] D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API

[Ben Barham via Phabricator via lldb-commits]

bnbarham updated this revision to Diff 414751.
bnbarham edited the summary of this revision.
bnbarham added a comment.

Update to single review dependency


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121426

Files:
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/VFS/Inputs/vfsroot.yaml
  clang/test/VFS/directory.c
  clang/test/VFS/multiple-overlays.c
  clang/test/VFS/vfsroot-with-overlay.c
  lldb/source/Commands/CommandObjectReproducer.cpp
  llvm/include/llvm/Support/Error.h
  llvm/tools/dsymutil/Reproducer.cpp
  llvm/tools/dsymutil/Reproducer.h

Index: llvm/tools/dsymutil/Reproducer.h
===
--- llvm/tools/dsymutil/Reproducer.h
+++ llvm/tools/dsymutil/Reproducer.h
@@ -59,8 +59,7 @@
 };
 
 /// Reproducer instance used to use an existing reproducer. The VFS returned by
-/// this instance is a RedirectingFileSystem that remaps paths to their
-/// counterpart in the reproducer.
+/// this instance remaps paths to their counterpart in the reproducer.
 class ReproducerUse : public Reproducer {
 public:
   ReproducerUse(StringRef Root, std::error_code &EC);
Index: llvm/tools/dsymutil/Reproducer.cpp
===
--- llvm/tools/dsymutil/Reproducer.cpp
+++ llvm/tools/dsymutil/Reproducer.cpp
@@ -48,15 +48,15 @@
 ReproducerUse::ReproducerUse(StringRef Root, std::error_code &EC) {
   SmallString<128> Mapping(Root);
   sys::path::append(Mapping, "mapping.yaml");
-  ErrorOr> Buffer =
-  vfs::getRealFileSystem()->getBufferForFile(Mapping.str());
 
-  if (!Buffer) {
-EC = Buffer.getError();
+  auto OverlayFS = llvm::vfs::getVFSFromYAMLs(Mapping.str());
+  if (auto Err = OverlayFS.takeError()) {
+llvm::handleAllErrors(std::move(Err), [&](const llvm::ErrorInfoBase &E) {
+  EC = E.convertToErrorCode();
+});
 return;
   }
-
-  VFS = llvm::vfs::getVFSFromYAML(std::move(Buffer.get()), nullptr, Mapping);
+  VFS = std::move(*OverlayFS);
 }
 
 llvm::Expected>
Index: llvm/include/llvm/Support/Error.h
===
--- llvm/include/llvm/Support/Error.h
+++ llvm/include/llvm/Support/Error.h
@@ -1281,7 +1281,7 @@
 return OS.str();
   }
 
-  StringRef getFileName() { return FileName; }
+  StringRef getFileName() const { return FileName; }
 
   Error takeError() { return Error(std::move(Err)); }
 
Index: lldb/source/Commands/CommandObjectReproducer.cpp
===
--- lldb/source/Commands/CommandObjectReproducer.cpp
+++ lldb/source/Commands/CommandObjectReproducer.cpp
@@ -409,23 +409,19 @@
 switch (m_options.provider) {
 case eReproducerProviderFiles: {
   FileSpec vfs_mapping = loader->GetFile();
+  std::string overlay_path = vfs_mapping.GetPath();
 
-  // Read the VFS mapping.
-  ErrorOr> buffer =
-  vfs::getRealFileSystem()->getBufferForFile(vfs_mapping.GetPath());
-  if (!buffer) {
-SetError(result, errorCodeToError(buffer.getError()));
+  Expected> vfs =
+  vfs::getVFSFromYAMLs(StringRef(overlay_path));
+  if (auto err = vfs.takeError()) {
+SetError(result, std::move(err));
 return false;
   }
 
-  // Initialize a VFS from the given mapping.
-  IntrusiveRefCntPtr vfs = vfs::getVFSFromYAML(
-  std::move(buffer.get()), nullptr, vfs_mapping.GetPath());
-
   // Dump the VFS to a buffer.
   std::string str;
   raw_string_ostream os(str);
-  static_cast(*vfs).dump(os);
+  (*vfs)->dump(os);
   os.flush();
 
   // Return the string.
Index: clang/test/VFS/vfsroot-with-overlay.c
===
--- clang/test/VFS/vfsroot-with-overlay.c
+++ clang/test/VFS/vfsroot-with-overlay.c
@@ -1,7 +1,7 @@
 // RUN: rm -rf %t
 // RUN: mkdir -p %t
 // RUN: sed -e "s@TEST_DIR@%{/S:regex_replacement}@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsroot.yaml > %t.yaml
-// RUN: sed -e "s@INPUT_DIR@/indirect-vfs-root-files@g" -e "s@OUT_DIR@/overlay-dir@g" %S/Inputs/vfsoverlay.yaml > %t/vfsoverlay.yaml
+// RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@/overlay-dir@g" %S/Inputs/vfsoverlay.yaml > %t/vfsoverlay.yaml
 // RUN: %clang_cc1 -Werror -ivfsoverlay %t.yaml -ivfsoverlay /direct-vfs-root-files/vfsoverlay.yaml -I /overlay-dir -fsyntax-only /tests/vfsroot-with-overlay.c
 
 #include "not_real.h"
Index: clang/test/VFS/multiple-overlays.c
===
--- /dev/null
+++ clang/test/VFS/multiple-overlays.c
@@ -0,0 +1,39 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s@EXTERNAL_DIR@%{/t:regex_replacement}/B@g" -e "s@NAME_DIR@%{/t:regex_replacement}/A@g" %t/vfs/base.yaml > %t/vfs/a-b.yaml
+// 

[Lldb-commits] [PATCH] D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API

[Ben Barham via Phabricator via lldb-commits]

bnbarham created this revision.
bnbarham added reviewers: keith, dexonsmith, JDevlieghere, vsapsai.
Herald added a subscriber: carlosgalvezp.
Herald added a project: All.
bnbarham requested review of this revision.
Herald added projects: clang, LLDB, LLVM, clang-tools-extra.
Herald added subscribers: cfe-commits, llvm-commits, lldb-commits.

Includes two test fixes (since chained mappings are no longer allowed)
and adds a new test for multiple overlays.

Uses other than `CompilerInvocation.cpp` are simple 1:1 mappings, but
without the need to read into a buffer first.

Depends on D121421  and D121423 
 and D121424 
 and D121425 



Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121426

Files:
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/VFS/Inputs/vfsroot.yaml
  clang/test/VFS/directory.c
  clang/test/VFS/multiple-overlays.c
  clang/test/VFS/vfsroot-with-overlay.c
  lldb/source/Commands/CommandObjectReproducer.cpp
  llvm/include/llvm/Support/Error.h
  llvm/tools/dsymutil/Reproducer.cpp
  llvm/tools/dsymutil/Reproducer.h

Index: llvm/tools/dsymutil/Reproducer.h
===
--- llvm/tools/dsymutil/Reproducer.h
+++ llvm/tools/dsymutil/Reproducer.h
@@ -59,8 +59,7 @@
 };
 
 /// Reproducer instance used to use an existing reproducer. The VFS returned by
-/// this instance is a RedirectingFileSystem that remaps paths to their
-/// counterpart in the reproducer.
+/// remaps paths to their counterpart in the reproducer.
 class ReproducerUse : public Reproducer {
 public:
   ReproducerUse(StringRef Root, std::error_code &EC);
Index: llvm/tools/dsymutil/Reproducer.cpp
===
--- llvm/tools/dsymutil/Reproducer.cpp
+++ llvm/tools/dsymutil/Reproducer.cpp
@@ -48,15 +48,15 @@
 ReproducerUse::ReproducerUse(StringRef Root, std::error_code &EC) {
   SmallString<128> Mapping(Root);
   sys::path::append(Mapping, "mapping.yaml");
-  ErrorOr> Buffer =
-  vfs::getRealFileSystem()->getBufferForFile(Mapping.str());
 
-  if (!Buffer) {
-EC = Buffer.getError();
+  auto OverlayFS = llvm::vfs::getVFSFromYAMLs(Mapping.str());
+  if (auto Err = OverlayFS.takeError()) {
+llvm::handleAllErrors(std::move(Err), [&](const llvm::ErrorInfoBase &E) {
+  EC = E.convertToErrorCode();
+});
 return;
   }
-
-  VFS = llvm::vfs::getVFSFromYAML(std::move(Buffer.get()), nullptr, Mapping);
+  VFS = std::move(*OverlayFS);
 }
 
 llvm::Expected>
Index: llvm/include/llvm/Support/Error.h
===
--- llvm/include/llvm/Support/Error.h
+++ llvm/include/llvm/Support/Error.h
@@ -1281,7 +1281,7 @@
 return OS.str();
   }
 
-  StringRef getFileName() { return FileName; }
+  StringRef getFileName() const { return FileName; }
 
   Error takeError() { return Error(std::move(Err)); }
 
Index: lldb/source/Commands/CommandObjectReproducer.cpp
===
--- lldb/source/Commands/CommandObjectReproducer.cpp
+++ lldb/source/Commands/CommandObjectReproducer.cpp
@@ -409,23 +409,19 @@
 switch (m_options.provider) {
 case eReproducerProviderFiles: {
   FileSpec vfs_mapping = loader->GetFile();
+  std::string overlay_path = vfs_mapping.GetPath();
 
-  // Read the VFS mapping.
-  ErrorOr> buffer =
-  vfs::getRealFileSystem()->getBufferForFile(vfs_mapping.GetPath());
-  if (!buffer) {
-SetError(result, errorCodeToError(buffer.getError()));
+  Expected> vfs =
+  vfs::getVFSFromYAMLs(StringRef(overlay_path));
+  if (auto err = vfs.takeError()) {
+SetError(result, std::move(err));
 return false;
   }
 
-  // Initialize a VFS from the given mapping.
-  IntrusiveRefCntPtr vfs = vfs::getVFSFromYAML(
-  std::move(buffer.get()), nullptr, vfs_mapping.GetPath());
-
   // Dump the VFS to a buffer.
   std::string str;
   raw_string_ostream os(str);
-  static_cast(*vfs).dump(os);
+  (*vfs)->dump(os);
   os.flush();
 
   // Return the string.
Index: clang/test/VFS/vfsroot-with-overlay.c
===
--- clang/test/VFS/vfsroot-with-overlay.c
+++ clang/test/VFS/vfsroot-with-overlay.c
@@ -1,7 +1,7 @@
 // RUN: rm -rf %t
 // RUN: mkdir -p %t
 // RUN: sed -e "s@TEST_DIR@%{/S:regex_replacement}@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsroot.yaml > %t.yaml
-// RUN: sed -e "s@INPUT_DIR@/indirect-vfs-root-files@g" -e "s@OUT_DIR@/overlay-dir@g" %S/Inputs/vfsoverlay.yaml > %t/vfsoverlay.yaml
+// RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@/overlay-dir@g" %S/Inputs/vfsoverlay.yaml > %t/vfsoverlay.yaml
 // RUN: %clang_c

[Lldb-commits] [PATCH] D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API

[Ben Barham via Phabricator via lldb-commits]

bnbarham added a comment.

In D121426#3376442 , @dexonsmith 
wrote:

>> Includes two test fixes (since chained mappings are no longer allowed)
>> and adds a new test for multiple overlays.
>
> Are we sure no one needs to support chained mappings? Has there been a 
> ~~clang-dev~~ discourse discussion about it already? Just concerned that some 
> vendor might rely on being able to support this.

I'm not *positive*, no, but I would be fairly surprised. You could just add the 
`A -> C` mapping if you really do want it. But I can start up that conversation 
if you think it needs having.

I actually didn't initially realise that there was a test for this case - 
`vfsroot-with-overlay.c` did test "indirection", but I completely missed it 
when I was looking through. I *thought* the only case was the one Nathan added 
in `directory.c` (and in that case what we really wanted was was what's now 
`fallback`).

@vsapsai do you know any clients of the chaining/nesting/indirection?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121426

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

[Lldb-commits] [PATCH] D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API

[Ben Barham via Phabricator via lldb-commits]

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



Comment at: llvm/include/llvm/Support/Error.h:1284
 
-  StringRef getFileName() { return FileName; }
+  StringRef getFileName() const { return FileName; }
 

dexonsmith wrote:
> bnbarham wrote:
> > Should this be in a change all by itself?
> Yes, but I also know you already split this out so I guess you just need to 
> rebase :).
Heh, when you suggested I pull the rename out I basically thought "welll, that 
answer this question"

So yep, it's out and I'll rebase this one :). Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121426

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

[Lldb-commits] [PATCH] D121494: [VFS] Rename `RedirectingFileSystem::dump` to `print`

[Ben Barham via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGcc63ae42d772: [VFS] Rename `RedirectingFileSystem::dump` to 
`print` (authored by bnbarham).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121494

Files:
  lldb/source/Commands/CommandObjectReproducer.cpp
  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
@@ -1381,14 +1381,14 @@
   return R;
 }
 
-void RedirectingFileSystem::dump(raw_ostream &OS) const {
+void RedirectingFileSystem::print(raw_ostream &OS) const {
   for (const auto &Root : Roots)
-dumpEntry(OS, Root.get());
+printEntry(OS, Root.get());
 }
 
-void RedirectingFileSystem::dumpEntry(raw_ostream &OS,
-  RedirectingFileSystem::Entry *E,
-  int NumSpaces) const {
+void RedirectingFileSystem::printEntry(raw_ostream &OS,
+   RedirectingFileSystem::Entry *E,
+   int NumSpaces) const {
   StringRef Name = E->getName();
   for (int i = 0, e = NumSpaces; i < e; ++i)
 OS << " ";
@@ -1401,12 +1401,12 @@
 
 for (std::unique_ptr &SubEntry :
  llvm::make_range(DE->contents_begin(), DE->contents_end()))
-  dumpEntry(OS, SubEntry.get(), NumSpaces + 2);
+  printEntry(OS, SubEntry.get(), NumSpaces + 2);
   }
 }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
-LLVM_DUMP_METHOD void RedirectingFileSystem::dump() const { dump(dbgs()); }
+void RedirectingFileSystem::dump() const { print(dbgs()); }
 #endif
 
 /// A helper class to hold the common YAML parsing state.
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -910,8 +910,9 @@
 
   std::vector getRoots() const;
 
-  void dump(raw_ostream &OS) const;
-  void dumpEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
+  void print(raw_ostream &OS) const;
+  void printEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
+
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
   LLVM_DUMP_METHOD void dump() const;
 #endif
Index: lldb/source/Commands/CommandObjectReproducer.cpp
===
--- lldb/source/Commands/CommandObjectReproducer.cpp
+++ lldb/source/Commands/CommandObjectReproducer.cpp
@@ -425,7 +425,7 @@
   // Dump the VFS to a buffer.
   std::string str;
   raw_string_ostream os(str);
-  static_cast(*vfs).dump(os);
+  static_cast(*vfs).print(os);
   os.flush();
 
   // Return the string.


Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1381,14 +1381,14 @@
   return R;
 }
 
-void RedirectingFileSystem::dump(raw_ostream &OS) const {
+void RedirectingFileSystem::print(raw_ostream &OS) const {
   for (const auto &Root : Roots)
-dumpEntry(OS, Root.get());
+printEntry(OS, Root.get());
 }
 
-void RedirectingFileSystem::dumpEntry(raw_ostream &OS,
-  RedirectingFileSystem::Entry *E,
-  int NumSpaces) const {
+void RedirectingFileSystem::printEntry(raw_ostream &OS,
+   RedirectingFileSystem::Entry *E,
+   int NumSpaces) const {
   StringRef Name = E->getName();
   for (int i = 0, e = NumSpaces; i < e; ++i)
 OS << " ";
@@ -1401,12 +1401,12 @@
 
 for (std::unique_ptr &SubEntry :
  llvm::make_range(DE->contents_begin(), DE->contents_end()))
-  dumpEntry(OS, SubEntry.get(), NumSpaces + 2);
+  printEntry(OS, SubEntry.get(), NumSpaces + 2);
   }
 }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
-LLVM_DUMP_METHOD void RedirectingFileSystem::dump() const { dump(dbgs()); }
+void RedirectingFileSystem::dump() const { print(dbgs()); }
 #endif
 
 /// A helper class to hold the common YAML parsing state.
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -910,8 +910,9 @@
 
   std::vector getRoots() const;
 
-  void dump(raw_ostream &OS) const;
-  void dumpEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
+  void print(raw_ostream &OS) const;
+  void printEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
+

[Lldb-commits] [PATCH] D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API

[Ben Barham via Phabricator via lldb-commits]

bnbarham marked an inline comment as done.
bnbarham added a comment.

Merged into D121425  instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121426

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

[Lldb-commits] [PATCH] D121425: [VFS] Add a new getVFSFromYAMLs API

[Ben Barham via Phabricator via lldb-commits]

bnbarham updated this revision to Diff 415615.
bnbarham edited the summary of this revision.
bnbarham added a comment.
Herald added subscribers: cfe-commits, lldb-commits, carlosgalvezp.
Herald added projects: clang, LLDB, clang-tools-extra.

Re-order to be before D121424  and merge with 
D121426 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121425

Files:
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/VFS/Inputs/vfsroot.yaml
  clang/test/VFS/directory.c
  clang/test/VFS/multiple-overlays.c
  clang/test/VFS/vfsroot-with-overlay.c
  lldb/source/Commands/CommandObjectReproducer.cpp
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/tools/dsymutil/Reproducer.cpp
  llvm/tools/dsymutil/Reproducer.h
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -14,6 +14,7 @@
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SourceMgr.h"
+#include "llvm/Testing/Support/Error.h"
 #include "llvm/Testing/Support/SupportHelpers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -2571,7 +2572,7 @@
 }
 
 TEST_F(VFSFromYAMLTest, WorkingDirectory) {
-  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  IntrusiveRefCntPtr Lower(new ErrorDummyFileSystem());
   Lower->addDirectory("//root/");
   Lower->addDirectory("//root/foo");
   Lower->addRegularFile("//root/foo/a");
@@ -2593,6 +2594,7 @@
   "}",
   Lower);
   ASSERT_NE(FS.get(), nullptr);
+
   std::error_code EC = FS->setCurrentWorkingDirectory("//root/bar");
   ASSERT_FALSE(EC);
 
@@ -2621,6 +2623,14 @@
   ASSERT_TRUE(WorkingDir);
   EXPECT_EQ(*WorkingDir, "//root/");
 
+  Status = FS->status("bar/a");
+  ASSERT_FALSE(Status.getError());
+  EXPECT_TRUE(Status->exists());
+
+  Status = FS->status("foo/a");
+  ASSERT_FALSE(Status.getError());
+  EXPECT_TRUE(Status->exists());
+
   EC = FS->setCurrentWorkingDirectory("bar");
   ASSERT_FALSE(EC);
   WorkingDir = FS->getCurrentWorkingDirectory();
@@ -2710,43 +2720,6 @@
   EXPECT_TRUE(Status->exists());
 }
 
-TEST_F(VFSFromYAMLTest, WorkingDirectoryFallthroughInvalid) {
-  IntrusiveRefCntPtr Lower(new ErrorDummyFileSystem());
-  Lower->addDirectory("//root/");
-  Lower->addDirectory("//root/foo");
-  Lower->addRegularFile("//root/foo/a");
-  Lower->addRegularFile("//root/foo/b");
-  Lower->addRegularFile("//root/c");
-  IntrusiveRefCntPtr FS = getFromYAMLString(
-  "{ 'use-external-names': false,\n"
-  "  'roots': [\n"
-  "{\n"
-  "  'type': 'directory',\n"
-  "  'name': '//root/bar',\n"
-  "  'contents': [ {\n"
-  "  'type': 'file',\n"
-  "  'name': 'a',\n"
-  "  'external-contents': '//root/foo/a'\n"
-  "}\n"
-  "  ]\n"
-  "}\n"
-  "]\n"
-  "}",
-  Lower);
-  ASSERT_NE(FS.get(), nullptr);
-  std::error_code EC = FS->setCurrentWorkingDirectory("//root/");
-  ASSERT_FALSE(EC);
-  ASSERT_NE(FS.get(), nullptr);
-
-  llvm::ErrorOr Status = FS->status("bar/a");
-  ASSERT_FALSE(Status.getError());
-  EXPECT_TRUE(Status->exists());
-
-  Status = FS->status("foo/a");
-  ASSERT_FALSE(Status.getError());
-  EXPECT_TRUE(Status->exists());
-}
-
 TEST_F(VFSFromYAMLTest, VirtualWorkingDirectory) {
   IntrusiveRefCntPtr Lower(new ErrorDummyFileSystem());
   Lower->addDirectory("//root/");
@@ -3207,3 +3180,168 @@
 "  DummyFileSystem (RecursiveContents)\n",
 Output);
 }
+
+static std::unique_ptr
+getVFSOrNull(ArrayRef YAMLOverlays,
+ IntrusiveRefCntPtr ExternalFS) {
+  SmallVector OverlayRefs;
+  for (const auto &Overlay : YAMLOverlays) {
+OverlayRefs.emplace_back(Overlay, "");
+  }
+
+  auto ExpectedFS = vfs::getVFSFromYAMLs(OverlayRefs, ExternalFS);
+  if (auto Err = ExpectedFS.takeError()) {
+consumeError(std::move(Err));
+return nullptr;
+  }
+  return std::move(*ExpectedFS);
+}
+
+static std::string createSimpleOverlay(StringRef RedirectKind, StringRef From,
+   StringRef To) {
+  return ("{\n"
+  "  'version': 0,\n"
+  "  'redirecting-with': '" +
+  RedirectKind +
+  "'\n"
+  "  'roots': [\n"
+  " {\n"
+  "   'type': 'directory-remap',\n"
+  "   'name': '" +
+  From +
+  "',\n"
+  "   'external-contents': '" +
+  To +
+  "',\n"
+  "   }]\n"
+  " }"
+  "  ]")
+  .str();
+}
+
+// Make sure that overlays are not transitive. Giv

[Lldb-commits] [PATCH] D121425: [VFS] Add a new getVFSFromYAMLs API

[Ben Barham via Phabricator via lldb-commits]

bnbarham added a comment.

There's two failing tests with this change:

- VFSFromYAMLTest.ReturnsExternalPathVFSHit
- VFSFromYAMLTest.ReturnsInternalPathVFSHit

Apparently we allow relative paths in external-contents *without* specifying 
`overlay-relative: true`. In this case the relative paths are resolved based on 
the CWD at the time of the operation. Do we really want that? I believe this 
wouldn't have worked prior to https://reviews.llvm.org/D109128 as there was no 
canonicalising before then as well. @keith was that change intentional? If we 
want it then it will *require* setting CWD on each FS in OverlayFS, which I was 
really hoping to avoid.

To give the concrete case:

  OverlayFileSystem
RedirectingFileSystem
  /a/foo -> bar
RealFileSystem
  /a/bar
  /b/bar
  
  cd /a
  cat foo # /a/bar
  
  cd /b # would fail previously since it doesn't exist in RedirectingFS but 
would still setCWD for RealFS
  cat foo # and thus return /a/bar since RedirectingFS is still in /a

This now fails completely because OverlayFS doesn't setCWD on the underlying 
filesystems. So RedirectingFS has the CWD of whatever ExternalFS was at 
creation. In D121424  it will fail because 
there's no longer any canonicalise after mapping the path in RedirectingFS (I 
assumed this was unintentional and missed the test case).

If we want to allow this then I think we'll need to go with the previous plan 
for OverlayFS, ie. keep track of filesystems that setCWD failed on and don't 
call them at all until they succeed again. That then doesn't allow certain 
cases that the current method allows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121425

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

[Lldb-commits] [PATCH] D121425: [VFS] Add a new getVFSFromYAMLs API

[Ben Barham via Phabricator via lldb-commits]

bnbarham added a comment.

In D121425#3384188 , @bnbarham wrote:

> There's two failing tests with this change:
>
> - VFSFromYAMLTest.ReturnsExternalPathVFSHit
> - VFSFromYAMLTest.ReturnsInternalPathVFSHit
>
> Apparently we allow relative paths in external-contents *without* specifying 
> `overlay-relative: true`. In this case the relative paths are resolved based 
> on the CWD at the time of the operation. Do we really want that? I believe 
> this wouldn't have worked prior to https://reviews.llvm.org/D109128 as there 
> was no canonicalising before then as well. @keith was that change 
> intentional? If we want it then it will *require* setting CWD on each FS in 
> OverlayFS, which I was really hoping to avoid.

I spoke to Keith offline. This has always worked - it previously worked by 
`RedirectingFileSystem` setting CWD on `ExternalFS` when `setCWD` was called on 
it. It's also important to keep supporting as it's used in Bazel 
(https://github.com/bazelbuild/rules_swift/blob/c1d7d1df6969c2675c7826ecf1202d78016b1753/swift/internal/vfsoverlay.bzl#L41-L55).

I'm hoping I can fix this by resolving the paths when the overlay is created. 
I'll see if that works (it'll depend on when -working-directory is actually 
used by the frontend).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121425

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

[Lldb-commits] [PATCH] D121425: [VFS] Add a new getVFSFromYAMLs API

[Ben Barham via Phabricator via lldb-commits]

bnbarham added a comment.

In D121425#3384492 , @dexonsmith 
wrote:

> Can you be more detailed about the overall state at the time of `cat`, which 
> causes it to fail?

Sure. I think there's also some confusion with names here but I'm generally 
trying to use the names as they are used in the overlay YAML or the name of the 
members in RedirectingFS.

"external-contents" is just the path that we're mapping to, so in my example 
above (/a/foo -> bar), "bar" is what I was referring to by "external-contents". 
So let's use that again -

  OverlayFileSystem
RedirectingFileSystem
  /a/foo -> bar
  
RealFileSystem
  /a/bar
  /b/bar

After D121423 , `cd /a` will result in the 
following -

- OverlayFS: CWD set to `/a`
- RedirectingFS and RealFS: CWD unchanged, so let's assume they were `/` at the 
time of creation and that's what they are now

`cat foo` -

- OverlayFS canonicalises to `/a/foo` and passes that to `openFileForRead` in 
RedirectingFS
- RedirectingFS has a `/a/foo` -> `bar` mapping, so it now runs 
`openFileForRead` on its ExternalFS (which is the RealFS)
- RealFS is at `/` and there is no `/bar` and thus fails
- RedirectingFS also fails
- Back in OverlayFS now, and because we failed we now try RealFS with `/a/foo`
- RealFS has no `/a/foo`, it fails
- Back in OverlayFS, all FS have now failed, return a failure

How this used to work really depends on when we're talking about 😆. Prior to 
D121423  the FS would have looked like:

  RedirectingFileSystem
/a/foo -> bar
  ExternalFS
RealFileSystem
  /a/bar
  /b/bar

Prior to Jonas' change, `setCWD` on RedirectingFS would run `setCWD` on 
`ExternalFS`. So `cd /a` would set CWD to `/a` for both RedirectingFS and 
RealFileSystem. Then `cat /a/foo` would give `bar` and `openFileForRead` on 
`ExternalFS` (RealFS) (which has `/a` CWD) would open `/a/bar`.

After Jonas' *and* Keith's change, `setCWD` no longer runs on `ExternalFS` but 
instead the path was canonicalised before sending it along. So `/a/foo` maps to 
`bar` as before, but then it would be canonicalised using `RedirectingFS` CWD 
and thus do an open for `/a/bar` on `ExternalFS`.

In D121425#3384499 , @dexonsmith 
wrote:

> (Would this mean resolving everything in the `.yaml` file eagerly at launch? 
> That sounds a bit scary...)

Yes, depending on what you mean by "resolving". All I mean is "prefix relative 
paths with CWD". The main issue with doing this is that it prohibits 
canonicalising paths to a virtual CWD, since you wouldn't be able to set CWD to 
a virtual path before making the overlay. Perhaps that's what you mean by "a 
bit scary" though.

We currently prefix relative paths with `ExternalContentsPrefixDir` if 
`overlay-relative: true` is set. That path is a combination of CWD + the 
directory to the overlay. This is to handle overlays that have absolute paths 
that we want to remap (specifically for the crash dump use case I believe). But 
if `overlay-relative: false` is set then we just canonicalise the path on 
open/etc.

I just ran a few tests with the frontend though:

- By default, paths come in relative and will become absolute from the 
process-wide CWD
- If `-working-directory` *is* set then paths come in absolute from 
`FileManager` - `setCWD` is never run

I'm not sure if there's a good reason it's done this way or it's just that CWD 
was only added to FileSystem relatively recently, but it does mean that that 
we're actually canonicalising the paths with the process-wide CWD regardless at 
the moment (at least from the frontend). This isn't the case if something was 
to use the API directly, but in that case I'm not sure that using the current 
CWD makes any more sense than using the CWD when the overlay is created. The 
semantics would be less confusing if that was the case (IMO), but again would 
prohibit canonicalising to a virtual CWD.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121425

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

[Lldb-commits] [PATCH] D121425: [VFS] Add a new getVFSFromYAMLs API

[Ben Barham via Phabricator via lldb-commits]

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

Blocked on the dependent


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121425

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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Ben Barham via Phabricator via lldb-commits]

bnbarham added inline comments.



Comment at: clang/lib/Index/USRGeneration.cpp:1032
+  case TemplateArgument::UncommonValue:
+// FIXME: Visit value.
+break;

akyrtzi wrote:
> erichkeane wrote:
> > bolshakov-a wrote:
> > > aaron.ballman wrote:
> > > > Any particular reason this isn't being handled now?
> > > I need some guidance here. Which characters are allowed in the USR? Could 
> > > the mangling algorithm from `CXXNameMangler::mangleValueInTemplateArg` be 
> > > moved into some common place and reused here?
> > I have no idea what is valid here.  BUT @akyrtzi and @gribozavr (or 
> > @gribozavr2 ?) seem to be the ones that touch these files the most?
> Adding @bnbarham to review the `Index` changes.
Just visiting the underlying type seems reasonable, ie. 
`VisitType(Arg.getUncommonValueType());`. If it needed to be differentiated 
between a `TemplateArgument::Type` you could add a prefix character (eg. `U`), 
but that doesn't seem needed to me.


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

https://reviews.llvm.org/D140996

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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Ben Barham via Phabricator via lldb-commits]

bnbarham added inline comments.



Comment at: clang/lib/Index/USRGeneration.cpp:1032
+  case TemplateArgument::UncommonValue:
+// FIXME: Visit value.
+break;

bolshakov-a wrote:
> bnbarham wrote:
> > akyrtzi wrote:
> > > erichkeane wrote:
> > > > bolshakov-a wrote:
> > > > > aaron.ballman wrote:
> > > > > > Any particular reason this isn't being handled now?
> > > > > I need some guidance here. Which characters are allowed in the USR? 
> > > > > Could the mangling algorithm from 
> > > > > `CXXNameMangler::mangleValueInTemplateArg` be moved into some common 
> > > > > place and reused here?
> > > > I have no idea what is valid here.  BUT @akyrtzi and @gribozavr (or 
> > > > @gribozavr2 ?) seem to be the ones that touch these files the most?
> > > Adding @bnbarham to review the `Index` changes.
> > Just visiting the underlying type seems reasonable, ie. 
> > `VisitType(Arg.getUncommonValueType());`. If it needed to be differentiated 
> > between a `TemplateArgument::Type` you could add a prefix character (eg. 
> > `U`), but that doesn't seem needed to me.
> Doesn't the holded value be added so as to distinguish e.g. `Tpl<1.5>` from 
> `Tpl<2.5>`?
Ah I see, yeah, we would. And there's no USR generation for APValue currently, 
which I assume is why your original question came up.

In general a USR just wants to uniquely identify an entity across compilations 
and isn't as restricted as the mangled name. For basically everything but 
`LValue` it seems like you'd be fine to print the value (for eg. int, float, 
etc), visit the underlying type (array, vector), or the visit the underlying 
decl (struct, union, member pointer). That's almost true for `LValue` as well, 
just with the extra parts that are also added to the ODR hash.

Alternatively, you could also just print the hash from `Profile` with the same 
handling as ODR hash. Worst case we'd accidentally merge specializations, but 
if that's good enough for the ODR hash it's probably good enough here as well.


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

https://reviews.llvm.org/D140996

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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Ben Barham via Phabricator via lldb-commits]

bnbarham added inline comments.



Comment at: clang/lib/Index/USRGeneration.cpp:1032
+  case TemplateArgument::UncommonValue:
+// FIXME: Visit value.
+break;

bolshakov-a wrote:
> bnbarham wrote:
> > bolshakov-a wrote:
> > > bnbarham wrote:
> > > > akyrtzi wrote:
> > > > > erichkeane wrote:
> > > > > > bolshakov-a wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > Any particular reason this isn't being handled now?
> > > > > > > I need some guidance here. Which characters are allowed in the 
> > > > > > > USR? Could the mangling algorithm from 
> > > > > > > `CXXNameMangler::mangleValueInTemplateArg` be moved into some 
> > > > > > > common place and reused here?
> > > > > > I have no idea what is valid here.  BUT @akyrtzi and @gribozavr (or 
> > > > > > @gribozavr2 ?) seem to be the ones that touch these files the most?
> > > > > Adding @bnbarham to review the `Index` changes.
> > > > Just visiting the underlying type seems reasonable, ie. 
> > > > `VisitType(Arg.getUncommonValueType());`. If it needed to be 
> > > > differentiated between a `TemplateArgument::Type` you could add a 
> > > > prefix character (eg. `U`), but that doesn't seem needed to me.
> > > Doesn't the holded value be added so as to distinguish e.g. `Tpl<1.5>` 
> > > from `Tpl<2.5>`?
> > Ah I see, yeah, we would. And there's no USR generation for APValue 
> > currently, which I assume is why your original question came up.
> > 
> > In general a USR just wants to uniquely identify an entity across 
> > compilations and isn't as restricted as the mangled name. For basically 
> > everything but `LValue` it seems like you'd be fine to print the value (for 
> > eg. int, float, etc), visit the underlying type (array, vector), or the 
> > visit the underlying decl (struct, union, member pointer). That's almost 
> > true for `LValue` as well, just with the extra parts that are also added to 
> > the ODR hash.
> > 
> > Alternatively, you could also just print the hash from `Profile` with the 
> > same handling as ODR hash. Worst case we'd accidentally merge 
> > specializations, but if that's good enough for the ODR hash it's probably 
> > good enough here as well.
> > it seems like you'd be fine to print the value (for eg. int, float, etc)
> 
> I'm in doubt about the dot inside a floating point value representation. 
> Minus sign is allowed, as I can see for `TemplateArgument::Integral` case.
As long as there's a prefix for APValue and its kind, the dot is fine (eg. 
maybe `@AP@` and then `f` for float, `i` for integer, etc).


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

https://reviews.llvm.org/D140996

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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Ben Barham via Phabricator via lldb-commits]

bnbarham added inline comments.



Comment at: clang/lib/Index/USRGeneration.cpp:1032
+  case TemplateArgument::UncommonValue:
+// FIXME: Visit value.
+break;

bolshakov-a wrote:
> bnbarham wrote:
> > bolshakov-a wrote:
> > > bnbarham wrote:
> > > > bolshakov-a wrote:
> > > > > bnbarham wrote:
> > > > > > akyrtzi wrote:
> > > > > > > erichkeane wrote:
> > > > > > > > bolshakov-a wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > Any particular reason this isn't being handled now?
> > > > > > > > > I need some guidance here. Which characters are allowed in 
> > > > > > > > > the USR? Could the mangling algorithm from 
> > > > > > > > > `CXXNameMangler::mangleValueInTemplateArg` be moved into some 
> > > > > > > > > common place and reused here?
> > > > > > > > I have no idea what is valid here.  BUT @akyrtzi and @gribozavr 
> > > > > > > > (or @gribozavr2 ?) seem to be the ones that touch these files 
> > > > > > > > the most?
> > > > > > > Adding @bnbarham to review the `Index` changes.
> > > > > > Just visiting the underlying type seems reasonable, ie. 
> > > > > > `VisitType(Arg.getUncommonValueType());`. If it needed to be 
> > > > > > differentiated between a `TemplateArgument::Type` you could add a 
> > > > > > prefix character (eg. `U`), but that doesn't seem needed to me.
> > > > > Doesn't the holded value be added so as to distinguish e.g. 
> > > > > `Tpl<1.5>` from `Tpl<2.5>`?
> > > > Ah I see, yeah, we would. And there's no USR generation for APValue 
> > > > currently, which I assume is why your original question came up.
> > > > 
> > > > In general a USR just wants to uniquely identify an entity across 
> > > > compilations and isn't as restricted as the mangled name. For basically 
> > > > everything but `LValue` it seems like you'd be fine to print the value 
> > > > (for eg. int, float, etc), visit the underlying type (array, vector), 
> > > > or the visit the underlying decl (struct, union, member pointer). 
> > > > That's almost true for `LValue` as well, just with the extra parts that 
> > > > are also added to the ODR hash.
> > > > 
> > > > Alternatively, you could also just print the hash from `Profile` with 
> > > > the same handling as ODR hash. Worst case we'd accidentally merge 
> > > > specializations, but if that's good enough for the ODR hash it's 
> > > > probably good enough here as well.
> > > > it seems like you'd be fine to print the value (for eg. int, float, etc)
> > > 
> > > I'm in doubt about the dot inside a floating point value representation. 
> > > Minus sign is allowed, as I can see for `TemplateArgument::Integral` case.
> > As long as there's a prefix for APValue and its kind, the dot is fine (eg. 
> > maybe `@AP@` and then `f` for float, `i` for integer, etc).
> Thank you! I've decided to go the simplest way, i. e. to use `ODRHash` here. 
> Should I write a test case (or some test cases), or they could become fragile 
> due to possible `ODRHash` implementation changes? I've checked USR locally a 
> little.
You could add a test that checks the ref has the same USR as the def, but yeah, 
I wouldn't specifically check the USR here.


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

https://reviews.llvm.org/D140996

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

[Lldb-commits] [PATCH] D136844: [libclang] Expose completion result kind in `CXCompletionResult`

[Ben Barham via Phabricator via lldb-commits]

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

Thanks Egor!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136844

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