[Lldb-commits] [PATCH] D76672: [lldb/Reproducers] Always collect the whole dSYM in the reproducer

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG38ddb49e5242: [lldb/Reproducers] Always collect the whole 
dSYM in the reproducer (authored by JDevlieghere).

Changed prior to commit:
  https://reviews.llvm.org/D76672?vs=252898=253718#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76672

Files:
  lldb/include/lldb/Utility/Reproducer.h
  lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
  lldb/source/Utility/Reproducer.cpp
  lldb/test/Shell/Reproducer/TestDSYM.test

Index: lldb/test/Shell/Reproducer/TestDSYM.test
===
--- /dev/null
+++ lldb/test/Shell/Reproducer/TestDSYM.test
@@ -0,0 +1,11 @@
+# REQUIRES: system-darwin
+# Ensure that the reproducers captures the whole dSYM bundle.
+
+# RUN: rm -rf %t.repro
+# RUN: %clang_host %S/Inputs/simple.c -g -o %t.out
+# RUN: touch %t.out.dSYM/foo.bar
+
+# RUN: %lldb -x -b --capture --capture-path %t.repro %t.out -o 'b main' -o 'run' -o 'reproducer generate'
+
+# RUN: %lldb -b -o 'reproducer dump -p files -f %t.repro' | FileCheck %s --check-prefix FILES
+# FILES: foo.bar
Index: lldb/source/Utility/Reproducer.cpp
===
--- lldb/source/Utility/Reproducer.cpp
+++ lldb/source/Utility/Reproducer.cpp
@@ -321,6 +321,11 @@
   os << m_cwd << "\n";
 }
 
+void FileProvider::recordInterestingDirectory(const llvm::Twine ) {
+  if (m_collector)
+m_collector->addDirectory(dir);
+}
+
 void ProviderBase::anchor() {}
 char CommandProvider::ID = 0;
 char FileProvider::ID = 0;
Index: lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
===
--- lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
+++ lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
@@ -20,6 +20,7 @@
 #include "lldb/Symbol/LocateSymbolFile.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Target/Target.h"
+#include "lldb/Utility/Reproducer.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/Timer.h"
 
@@ -145,6 +146,11 @@
 }
 
 if (dsym_fspec) {
+  // Compute dSYM root.
+  std::string dsym_root = dsym_fspec.GetPath();
+  const size_t pos = dsym_root.find("/Contents/Resources/");
+  dsym_root = pos != std::string::npos ? dsym_root.substr(0, pos) : "";
+
   DataBufferSP dsym_file_data_sp;
   lldb::offset_t dsym_file_data_offset = 0;
   dsym_objfile_sp =
@@ -154,136 +160,132 @@
   if (UUIDsMatch(module_sp.get(), dsym_objfile_sp.get(), feedback_strm)) {
 // We need a XML parser if we hope to parse a plist...
 if (XMLDocument::XMLEnabled()) {
-  char dsym_path[PATH_MAX];
-  if (module_sp->GetSourceMappingList().IsEmpty() &&
-  dsym_fspec.GetPath(dsym_path, sizeof(dsym_path))) {
+  if (module_sp->GetSourceMappingList().IsEmpty()) {
 lldb_private::UUID dsym_uuid = dsym_objfile_sp->GetUUID();
 if (dsym_uuid) {
   std::string uuid_str = dsym_uuid.GetAsString();
-  if (!uuid_str.empty()) {
-char *resources = strstr(dsym_path, "/Contents/Resources/");
-if (resources) {
-  char dsym_uuid_plist_path[PATH_MAX];
-  resources[strlen("/Contents/Resources/")] = '\0';
-  snprintf(dsym_uuid_plist_path, sizeof(dsym_uuid_plist_path),
-   "%s%s.plist", dsym_path, uuid_str.c_str());
-  FileSpec dsym_uuid_plist_spec(dsym_uuid_plist_path);
-  if (FileSystem::Instance().Exists(dsym_uuid_plist_spec)) {
-ApplePropertyList plist(dsym_uuid_plist_path);
-if (plist) {
-  std::string DBGBuildSourcePath;
-  std::string DBGSourcePath;
-
-  // DBGSourcePathRemapping is a dictionary in the plist
-  // with keys which are DBGBuildSourcePath file paths and
-  // values which are DBGSourcePath file paths
-
-  StructuredData::ObjectSP plist_sp =
-  plist.GetStructuredData();
-  if (plist_sp.get() && plist_sp->GetAsDictionary() &&
-  plist_sp->GetAsDictionary()->HasKey(
-  "DBGSourcePathRemapping") &&
-  plist_sp->GetAsDictionary()
-  ->GetValueForKey("DBGSourcePathRemapping")
-  ->GetAsDictionary()) {
-
-// If DBGVersion 1 or DBGVersion missing, ignore DBGSourcePathRemapping.
-// If DBGVersion 2, strip last two components of path remappings from
-//  entries to fix an issue with a specific 

[Lldb-commits] [PATCH] D76672: [lldb/Reproducers] Always collect the whole dSYM in the reproducer

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 252898.
JDevlieghere added a comment.

Implement Pavel's suggestion


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

https://reviews.llvm.org/D76672

Files:
  lldb/include/lldb/Utility/Reproducer.h
  lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
  lldb/source/Utility/Reproducer.cpp
  lldb/test/Shell/Reproducer/TestDSYM.test

Index: lldb/test/Shell/Reproducer/TestDSYM.test
===
--- /dev/null
+++ lldb/test/Shell/Reproducer/TestDSYM.test
@@ -0,0 +1,11 @@
+# REQUIRES: system-darwin
+# Ensure that the reproducers captures the whole dSYM bundle.
+
+# RUN: rm -rf %t.repro
+# RUN: %clang_host %S/Inputs/simple.c -g -o %t.out
+# RUN: touch %t.out.dSYM/foo.bar
+
+# RUN: %lldb -x -b --capture --capture-path %t.repro %t.out -o 'b main' -o 'run' -o 'reproducer generate'
+
+# RUN: %lldb -b -o 'reproducer dump -p files -f %t.repro' | FileCheck %s --check-prefix FILES
+# FILES: foo.bar
Index: lldb/source/Utility/Reproducer.cpp
===
--- lldb/source/Utility/Reproducer.cpp
+++ lldb/source/Utility/Reproducer.cpp
@@ -321,6 +321,11 @@
   os << m_cwd << "\n";
 }
 
+void FileProvider::recordInterestingDirectory(const llvm::Twine ) {
+  if (m_collector)
+m_collector->addDirectory(dir);
+}
+
 void ProviderBase::anchor() {}
 char CommandProvider::ID = 0;
 char FileProvider::ID = 0;
Index: lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
===
--- lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
+++ lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
@@ -20,6 +20,7 @@
 #include "lldb/Symbol/LocateSymbolFile.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Target/Target.h"
+#include "lldb/Utility/Reproducer.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/Timer.h"
 
@@ -145,6 +146,14 @@
 }
 
 if (dsym_fspec) {
+  // Compute dSYM root.
+  std::string dsym_root = dsym_fspec.GetPath();
+  const size_t pos = dsym_root.find("/Contents/Resources/");
+  if (pos == std::string::npos)
+dsym_root.clear();
+  else
+dsym_root = dsym_root.substr(0, pos);
+
   DataBufferSP dsym_file_data_sp;
   lldb::offset_t dsym_file_data_offset = 0;
   dsym_objfile_sp =
@@ -154,136 +163,131 @@
   if (UUIDsMatch(module_sp.get(), dsym_objfile_sp.get(), feedback_strm)) {
 // We need a XML parser if we hope to parse a plist...
 if (XMLDocument::XMLEnabled()) {
-  char dsym_path[PATH_MAX];
-  if (module_sp->GetSourceMappingList().IsEmpty() &&
-  dsym_fspec.GetPath(dsym_path, sizeof(dsym_path))) {
+  if (module_sp->GetSourceMappingList().IsEmpty()) {
 lldb_private::UUID dsym_uuid = dsym_objfile_sp->GetUUID();
 if (dsym_uuid) {
   std::string uuid_str = dsym_uuid.GetAsString();
-  if (!uuid_str.empty()) {
-char *resources = strstr(dsym_path, "/Contents/Resources/");
-if (resources) {
-  char dsym_uuid_plist_path[PATH_MAX];
-  resources[strlen("/Contents/Resources/")] = '\0';
-  snprintf(dsym_uuid_plist_path, sizeof(dsym_uuid_plist_path),
-   "%s%s.plist", dsym_path, uuid_str.c_str());
-  FileSpec dsym_uuid_plist_spec(dsym_uuid_plist_path);
-  if (FileSystem::Instance().Exists(dsym_uuid_plist_spec)) {
-ApplePropertyList plist(dsym_uuid_plist_path);
-if (plist) {
-  std::string DBGBuildSourcePath;
-  std::string DBGSourcePath;
-
-  // DBGSourcePathRemapping is a dictionary in the plist
-  // with keys which are DBGBuildSourcePath file paths and
-  // values which are DBGSourcePath file paths
-
-  StructuredData::ObjectSP plist_sp =
-  plist.GetStructuredData();
-  if (plist_sp.get() && plist_sp->GetAsDictionary() &&
-  plist_sp->GetAsDictionary()->HasKey(
-  "DBGSourcePathRemapping") &&
-  plist_sp->GetAsDictionary()
-  ->GetValueForKey("DBGSourcePathRemapping")
-  ->GetAsDictionary()) {
-
-// If DBGVersion 1 or DBGVersion missing, ignore DBGSourcePathRemapping.
-// If DBGVersion 2, strip last two components of path remappings from
-//  entries to fix an issue with a specific set of
-//  DBGSourcePathRemapping entries that lldb worked
-//  with.
-

[Lldb-commits] [PATCH] D76672: [lldb/Reproducers] Always collect the whole dSYM in the reproducer

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Wow. This is a lot more generic than I had mind. But at the same time, it does 
not seem generic enough. :( Like, for instance, using only component-based 
matching, one couldn't ignore `/usr/lib` while keeping `$BUILD/lib` or whatever.

What I had in mind for your use case was a simple function (*) like 
`repro::???->recordInterestingDirectory(StringRef)`, and calling this 
dynamically from SymbolVendorMacOSX when it opens a dsym (so, approximately 
around here 
,
 maybe after some refactoring to avoid computing the dsym directory twice). 
That seems to be a lot more flexible, requires less plumbing, and about as 
equally obtrusive for the resulting code as attempting to statically describe 
the interesting directories via these matchers.

It's true that may need some kind of matchers for ignoring directories (**), 
but these should probably work on the full path, not individual components. And 
maybe they too shouldn't be fully static, so that they can be controlled by the 
context, or the user... I don't know -- we don't have to design this part now.

So what do you think about just adding the `recordInterestingDirectory` 
function? Would that be sufficient for your needs?

(*) I don't know which class should that function me a member of (if any). 
However, I think it would be nice if it was something in the repro namespace 
(and not the FileSystem class) to make it clear that this is happening for the 
sake of reproducers.

(**) I can see how my previous comment could be interpreted as a request to 
implement ignoring directories. That was not my intention, I am sorry. I was 
just musing about possible future directions (e.g. some function like 
`ignoreUniniterestingDirectory`).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D76672



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

[Lldb-commits] [PATCH] D76672: [lldb/Reproducers] Always collect the whole dSYM in the reproducer

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 252649.
JDevlieghere added a comment.

Make the whole thing generic.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D76672

Files:
  lldb/include/lldb/Host/FileSystem.h
  lldb/include/lldb/Utility/FileCollector.h
  lldb/include/lldb/Utility/Reproducer.h
  lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
  lldb/source/Utility/CMakeLists.txt
  lldb/source/Utility/FileCollector.cpp
  lldb/test/Shell/Reproducer/TestDSYM.test
  lldb/unittests/Utility/CMakeLists.txt
  lldb/unittests/Utility/FileCollectorTest.cpp
  llvm/include/llvm/Support/FileCollector.h
  llvm/lib/Support/FileCollector.cpp

Index: llvm/lib/Support/FileCollector.cpp
===
--- llvm/lib/Support/FileCollector.cpp
+++ llvm/lib/Support/FileCollector.cpp
@@ -61,26 +61,25 @@
   return true;
 }
 
-void FileCollector::addFile(const Twine ) {
-  std::lock_guard lock(Mutex);
-  std::string FileStr = file.str();
-  if (markAsSeen(FileStr))
-addFileImpl(FileStr);
-}
+void FileCollector::addFile(const Twine ) { addFileImpl(file.str()); }
 
 void FileCollector::addDirectory(const Twine ) {
   assert(sys::fs::is_directory(dir));
   std::error_code EC;
   sys::fs::recursive_directory_iterator Iter(dir, EC);
   sys::fs::recursive_directory_iterator End;
-  addFile(dir); // Add the directory in case it's empty.
+  addFileImpl(dir.str()); // Add the directory in case it's empty.
   for (; Iter != End && !EC; Iter.increment(EC)) {
 if (sys::fs::is_regular_file(Iter->path()))
-  addFile(Iter->path());
+  addFileImpl(Iter->path());
   }
 }
 
 void FileCollector::addFileImpl(StringRef SrcPath) {
+  std::lock_guard lock(Mutex);
+  if (!markAsSeen(SrcPath))
+return;
+
   // We need an absolute src path to append to the root.
   SmallString<256> AbsoluteSrc = SrcPath;
   sys::fs::make_absolute(AbsoluteSrc);
Index: llvm/include/llvm/Support/FileCollector.h
===
--- llvm/include/llvm/Support/FileCollector.h
+++ llvm/include/llvm/Support/FileCollector.h
@@ -45,7 +45,7 @@
   createCollectorVFS(IntrusiveRefCntPtr BaseFS,
  std::shared_ptr Collector);
 
-private:
+protected:
   void addFileImpl(StringRef SrcPath);
 
   bool markAsSeen(StringRef Path) {
@@ -60,7 +60,6 @@
 VFSWriter.addFileMapping(VirtualPath, RealPath);
   }
 
-protected:
   /// Synchronizes adding files.
   std::mutex Mutex;
 
Index: lldb/unittests/Utility/FileCollectorTest.cpp
===
--- /dev/null
+++ lldb/unittests/Utility/FileCollectorTest.cpp
@@ -0,0 +1,112 @@
+//===-- ReproducerTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/FileCollector.h"
+
+using namespace lldb_private;
+
+class TestFileCollector : public FileCollector {
+public:
+  TestFileCollector() : FileCollector("", ""){};
+
+  using FileCollector::getAction;
+  void addDirectory(const llvm::Twine ) override {
+directories.emplace_back(file.str());
+  }
+  std::vector directories;
+};
+
+TEST(FileCollectorTest, GetActionTest) {
+  {
+TestFileCollector collector;
+collector.registerMatcher("foo", FileCollector::Action::KeepContents,
+  FileCollector::Matcher::Full);
+
+auto action = collector.getAction("foo");
+ASSERT_TRUE(static_cast(action));
+EXPECT_EQ(FileCollector::Action::KeepContents, *action);
+
+action = collector.getAction("bar");
+ASSERT_FALSE(static_cast(action));
+
+action = collector.getAction("foobar");
+ASSERT_FALSE(static_cast(action));
+
+// Register a new matcher that overrides the previous one.
+collector.registerMatcher("foo", FileCollector::Action::KeepContents,
+  FileCollector::Matcher::Substring);
+
+action = collector.getAction("foo");
+ASSERT_TRUE(static_cast(action));
+EXPECT_EQ(FileCollector::Action::KeepContents, *action);
+
+action = collector.getAction("bar");
+ASSERT_FALSE(static_cast(action));
+
+action = collector.getAction("foobar");
+ASSERT_TRUE(static_cast(action));
+EXPECT_EQ(FileCollector::Action::KeepContents, *action);
+  }
+
+  {
+TestFileCollector collector;
+collector.registerMatcher(".foo", FileCollector::Action::KeepContents,
+  FileCollector::Matcher::Extension);
+
+auto action = collector.getAction("foo");
+ASSERT_FALSE(static_cast(action));
+
+action = collector.getAction("bar");
+  

[Lldb-commits] [PATCH] D76672: [lldb/Reproducers] Always collect the whole dSYM in the reproducer

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Ok, you really want to collect the full dsym, then who am I to stop you. :)

But implementing that collection in the FileCollector does not seem right, 
because at the filesystem level, dsyms are not special (which is why you needed 
to reverse-engineer them from the path). Maybe if we create some sort of an API 
to register files/directories of "special interest", then we could have 
something (`SymbolVendorMacOSX`, most likely) call it to record the full dsym 
when it knows it has encountered it.

Then we could have a similar api to register files of special **dis**interest, 
which could be used to selectively omit things from the reproducer. That part 
may be also interesting for using reproducers internally at google, because the 
binaries there are too big to be shipped around all the time.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D76672



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

[Lldb-commits] [PATCH] D76672: [lldb/Reproducers] Always collect the whole dSYM in the reproducer

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Also, the dSYM bundle for a binary contains both the debug information and 
scripts that may be useful in debugging that binary - data formatters and the 
like.  So even if the original debugging session crashed before they got around 
to using any of the facilities in their dSYM, they are still likely to come in 
handy in analyzing the state of the app in the reproducer.  For this reason, I 
think we should treat dSYM's as special and capture them in full.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D76672



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

[Lldb-commits] [PATCH] D76672: [lldb/Reproducers] Always collect the whole dSYM in the reproducer

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

In D76672#1939583 , @labath wrote:

> In D76672#1939377 , @JDevlieghere 
> wrote:
>
> > In D76672#1938629 , @labath wrote:
> >
> > > It's not clear to me why this is needed.
> > >
> > > I mean, if lldb touches any of the files inside the dsym bundle, then 
> > > they should be automatically recorded already, right? And if it doesn't 
> > > then it does not need them...
> >
> >
> > The dSYM can contain other resources than just the Mach-O companion file, 
> > such as script for the OS plugins or opt remarks, which might not be used 
> > by the reproducers or even LLDB at all. Once you have the reproducer on 
> > your system, tools will find and use it because spotlight indexed it. 
> > Having only a partial dSYM is really undesirable as it can break LLDB and 
> > other tools in really unexpected ways.
>
>
> Interesting. Could that be fixed by adding the funny `.noindex` suffix to the 
> reproducer name (or some subdirectory of it)?


Yes, .noindex would prevent Spotlight from finding it. lldb might still find it 
though if it happens to be next to the executable.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D76672



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

[Lldb-commits] [PATCH] D76672: [lldb/Reproducers] Always collect the whole dSYM in the reproducer

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D76672#1939583 , @labath wrote:

> In D76672#1939377 , @JDevlieghere 
> wrote:
>
> > In D76672#1938629 , @labath wrote:
> >
> > > It's not clear to me why this is needed.
> > >
> > > I mean, if lldb touches any of the files inside the dsym bundle, then 
> > > they should be automatically recorded already, right? And if it doesn't 
> > > then it does not need them...
> >
> >
> > The dSYM can contain other resources than just the Mach-O companion file, 
> > such as script for the OS plugins or opt remarks, which might not be used 
> > by the reproducers or even LLDB at all. Once you have the reproducer on 
> > your system, tools will find and use it because spotlight indexed it. 
> > Having only a partial dSYM is really undesirable as it can break LLDB and 
> > other tools in really unexpected ways.
>
>
> Interesting. Could that be fixed by adding the funny `.noindex` suffix to the 
> reproducer name (or some subdirectory of it)?


Yes, it's an alternative I've considered. I prefer this approach because (1) 
conceptually a dSYM is a single unit, the fact that the bundle is actually a 
directory on disk is just an implementation detail and (2) it allows you to use 
the dSYM even when reproducer replay fails. As we're discovering bugs in the 
reproducers, we can usually still use the files inside it to the debug the 
issue, even if replay fails.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D76672



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

[Lldb-commits] [PATCH] D76672: [lldb/Reproducers] Always collect the whole dSYM in the reproducer

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D76672#1939377 , @JDevlieghere 
wrote:

> In D76672#1938629 , @labath wrote:
>
> > It's not clear to me why this is needed.
> >
> > I mean, if lldb touches any of the files inside the dsym bundle, then they 
> > should be automatically recorded already, right? And if it doesn't then it 
> > does not need them...
>
>
> The dSYM can contain other resources than just the Mach-O companion file, 
> such as script for the OS plugins or opt remarks, which might not be used by 
> the reproducers or even LLDB at all. Once you have the reproducer on your 
> system, tools will find and use it because spotlight indexed it. Having only 
> a partial dSYM is really undesirable as it can break LLDB and other tools in 
> really unexpected ways.


Interesting. Could that be fixed by adding the funny `.noindex` suffix to the 
reproducer name (or some subdirectory of it)?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D76672



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

[Lldb-commits] [PATCH] D76672: [lldb/Reproducers] Always collect the whole dSYM in the reproducer

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/include/lldb/Utility/FileCollector.h:26
+private:
+  bool addDSYM(const llvm::Twine );
+};

This is something I've struggled with before. If I'm inheriting from an LLVM 
class do I use LLVM or LLDB naming schemes?



Comment at: lldb/source/Utility/FileCollector.cpp:1
+//===-- FileCollector.cpp ---*- C++ 
-*-===//
+//

The C++ is redundant. This is a .cpp file, so it's not ambiguous which language 
this is.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D76672



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

[Lldb-commits] [PATCH] D76672: [lldb/Reproducers] Always collect the whole dSYM in the reproducer

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It's not clear to me why this is needed.

I mean, if lldb touches any of the files inside the dsym bundle, then they 
should be automatically recorded already, right? And if it doesn't then it does 
not need them...

It sounds to me like we are failing to capture some of the file accesses. Can 
this be fixed by catching those instead?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D76672



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

[Lldb-commits] [PATCH] D76672: [lldb/Reproducers] Always collect the whole dSYM in the reproducer

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: labath, arphaman, aprantl.
JDevlieghere added a project: LLDB.
Herald added subscribers: teemperor, abidh, dexonsmith, hiraditya, mgorny.
JDevlieghere added a parent revision: D76671: [FileCollector] Add a method to 
add a whole directory and it contents..

The FileCollector in LLDB collects every files that's used during a debug 
session when capture is enabled. This ensures that the reproducer only contains 
the files necessary to reproduce. This approach is not a good fit for the dSYM 
bundle, which is a directory on disk, but should be treated as a single unit. 
On macOS LLDB have automatically find the matching dSYM for a binary by its 
UUID. Having a incomplete dSYM in a reproducer can break debugging even when 
reproducers are disabled.

This patch adds a custom FileCollector to LLDB that is aware of dSYM. When it 
encounters a dSYM bundle, it iterates over everything inside it and adds it to 
the reproducer. While this might seem like overkill right now, having custom 
FileCollector is going to be inevitable as soon as we want to be smarter in 
what we include in the reproducer. For example, to keep its size small, we 
might want to skip system libraries in the future.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D76672

Files:
  lldb/include/lldb/Host/FileSystem.h
  lldb/include/lldb/Utility/FileCollector.h
  lldb/include/lldb/Utility/Reproducer.h
  lldb/source/Utility/CMakeLists.txt
  lldb/source/Utility/FileCollector.cpp
  lldb/test/Shell/Reproducer/TestDSYM.test
  llvm/include/llvm/Support/FileCollector.h
  llvm/lib/Support/FileCollector.cpp

Index: llvm/lib/Support/FileCollector.cpp
===
--- llvm/lib/Support/FileCollector.cpp
+++ llvm/lib/Support/FileCollector.cpp
@@ -61,26 +61,25 @@
   return true;
 }
 
-void FileCollector::addFile(const Twine ) {
-  std::lock_guard lock(Mutex);
-  std::string FileStr = file.str();
-  if (markAsSeen(FileStr))
-addFileImpl(FileStr);
-}
+void FileCollector::addFile(const Twine ) { addFileImpl(file.str()); }
 
 void FileCollector::addDirectory(const Twine ) {
   assert(sys::fs::is_directory(dir));
   std::error_code EC;
   sys::fs::recursive_directory_iterator Iter(dir, EC);
   sys::fs::recursive_directory_iterator End;
-  addFile(dir); // Add the directory in case it's empty.
+  addFileImpl(dir.str()); // Add the directory in case it's empty.
   for (; Iter != End && !EC; Iter.increment(EC)) {
 if (sys::fs::is_regular_file(Iter->path()))
-  addFile(Iter->path());
+  addFileImpl(Iter->path());
   }
 }
 
 void FileCollector::addFileImpl(StringRef SrcPath) {
+  std::lock_guard lock(Mutex);
+  if (!markAsSeen(SrcPath))
+return;
+
   // We need an absolute src path to append to the root.
   SmallString<256> AbsoluteSrc = SrcPath;
   sys::fs::make_absolute(AbsoluteSrc);
Index: llvm/include/llvm/Support/FileCollector.h
===
--- llvm/include/llvm/Support/FileCollector.h
+++ llvm/include/llvm/Support/FileCollector.h
@@ -45,7 +45,7 @@
   createCollectorVFS(IntrusiveRefCntPtr BaseFS,
  std::shared_ptr Collector);
 
-private:
+protected:
   void addFileImpl(StringRef SrcPath);
 
   bool markAsSeen(StringRef Path) {
@@ -60,7 +60,6 @@
 VFSWriter.addFileMapping(VirtualPath, RealPath);
   }
 
-protected:
   /// Synchronizes adding files.
   std::mutex Mutex;
 
Index: lldb/test/Shell/Reproducer/TestDSYM.test
===
--- /dev/null
+++ lldb/test/Shell/Reproducer/TestDSYM.test
@@ -0,0 +1,11 @@
+# REQUIRES: system-darwin
+# Ensure that the reproducers captures the whole dSYM bundle.
+
+# RUN: rm -rf %t.repro
+# RUN: %clang_host %S/Inputs/simple.c -g -o %t.out
+# RUN: touch %t.out.dSYM/foo.bar
+
+# RUN: %lldb -x -b --capture --capture-path %t.repro %t.out -o 'b main' -o 'run' -o 'reproducer generate'
+
+# RUN: %lldb -b -o 'reproducer dump -p files -f %t.repro' | FileCheck %s --check-prefix FILES
+# FILES: foo.bar
Index: lldb/source/Utility/FileCollector.cpp
===
--- /dev/null
+++ lldb/source/Utility/FileCollector.cpp
@@ -0,0 +1,43 @@
+//===-- FileCollector.cpp ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Utility/FileCollector.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/Process.h"
+
+using namespace lldb_private;
+
+void FileCollector::addFile(const llvm::Twine ) {
+  // Add dSYM bundle if applicable.
+  if