[lldb/Reproducers] Always collect the whole dSYM in the reproducer

[JDevlieghere]

Summary

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.

[labath]

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?

[JDevlieghere]

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.

[aprantl]

lldb/include/lldb/Utility/FileCollector.h

26 ↗

(On Diff #252233)

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

lldb/source/Utility/FileCollector.cpp

1 ↗

(On Diff #252233)

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

[labath]

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)?

[JDevlieghere]

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.

[aprantl]

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.

[jingham]

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.

[labath]

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 disinterest, 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.

[JDevlieghere]

Make the whole thing generic.

[labath]

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).

[JDevlieghere]

Implement Pavel's suggestion

lldb/include/lldb/Utility/Reproducer.h

     return m_collector;
   }
 
+  void recordInterestingDirectory(const llvm::Twine &dir);
+
   void Keep() override {
     auto mapping = GetRoot().CopyByAppendingPathComponent(Info::file);
     // Temporary files that are removed during execution can cause copy errors.

lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp

 #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"
 

     }
 
     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 =

       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.
-                        // If DBGVersion 3, trust & use the source path remappings as-is.
-                        //
-
-                        bool new_style_source_remapping_dictionary = false;
-                        bool do_truncate_remapping_names = false;
-                        std::string original_DBGSourcePath_value =
-                            DBGSourcePath;
-                        if (plist_sp->GetAsDictionary()->HasKey("DBGVersion")) {
-                          std::string version_string =
-                              std::string(plist_sp->GetAsDictionary()
-                                              ->GetValueForKey("DBGVersion")
-                                              ->GetStringValue(""));
-                          if (!version_string.empty() &&
-                              isdigit(version_string[0])) {
-                            int version_number = atoi(version_string.c_str());
-                            if (version_number > 1) {
-                              new_style_source_remapping_dictionary = true;
-                            }
-                            if (version_number == 2) {
-                                do_truncate_remapping_names = true;
-                            }
+              if (!uuid_str.empty() && !dsym_root.empty()) {
+                char dsym_uuid_plist_path[PATH_MAX];
+                snprintf(dsym_uuid_plist_path, sizeof(dsym_uuid_plist_path),
+                         "%s%s.plist", dsym_root.c_str(), 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.
+                      // If DBGVersion 3, trust & use the source path remappings
+                      // as-is.
+                      //
+
+                      bool new_style_source_remapping_dictionary = false;
+                      bool do_truncate_remapping_names = false;
+                      std::string original_DBGSourcePath_value = DBGSourcePath;
+                      if (plist_sp->GetAsDictionary()->HasKey("DBGVersion")) {
+                        std::string version_string =
+                            std::string(plist_sp->GetAsDictionary()
+                                            ->GetValueForKey("DBGVersion")
+                                            ->GetStringValue(""));
+                        if (!version_string.empty() &&
+                            isdigit(version_string[0])) {
+                          int version_number = atoi(version_string.c_str());
+                          if (version_number > 1) {
+                            new_style_source_remapping_dictionary = true;
+                          }
+                          if (version_number == 2) {
+                            do_truncate_remapping_names = true;
                           }
                         }
+                      }
 
-                        StructuredData::Dictionary *remappings_dict =
-                            plist_sp->GetAsDictionary()
-                                ->GetValueForKey("DBGSourcePathRemapping")
-                                ->GetAsDictionary();
-                        remappings_dict->ForEach(
-                            [&module_sp, new_style_source_remapping_dictionary,
-                             original_DBGSourcePath_value, do_truncate_remapping_names](
-                                ConstString key,
-                                StructuredData::Object *object) -> bool {
-                              if (object && object->GetAsString()) {
-
-                                // key is DBGBuildSourcePath
-                                // object is DBGSourcePath
-                                std::string DBGSourcePath =
-                                    std::string(object->GetStringValue());
-                                if (!new_style_source_remapping_dictionary &&
-                                    !original_DBGSourcePath_value.empty()) {
-                                  DBGSourcePath = original_DBGSourcePath_value;
-                                }
-                                if (DBGSourcePath[0] == '~') {
-                                  FileSpec resolved_source_path(
-                                      DBGSourcePath.c_str());
-                                  FileSystem::Instance().Resolve(
-                                      resolved_source_path);
-                                  DBGSourcePath =
-                                      resolved_source_path.GetPath();
-                                }
+                           original_DBGSourcePath_value,
+                           do_truncate_remapping_names](
+                              ConstString key,
+                              StructuredData::Object *object) -> bool {
+                            if (object && object->GetAsString()) {
+
+                              // key is DBGBuildSourcePath
+                              // object is DBGSourcePath
+                              std::string DBGSourcePath =
+                                  std::string(object->GetStringValue());
+                              if (!new_style_source_remapping_dictionary &&
+                                  !original_DBGSourcePath_value.empty()) {
+                                DBGSourcePath = original_DBGSourcePath_value;
+                              }
+                              if (DBGSourcePath[0] == '~') {
+                                FileSpec resolved_source_path(
+                                    DBGSourcePath.c_str());
+                                FileSystem::Instance().Resolve(
+                                    resolved_source_path);
+                                DBGSourcePath = resolved_source_path.GetPath();
+                              module_sp->GetSourceMappingList().Append(
+                                  key, ConstString(DBGSourcePath), true);
+                              // With version 2 of DBGSourcePathRemapping, we
+                              // can chop off the last two filename parts
+                              // from the source remapping and get a more
+                              // general source remapping that still works.
+                              // Add this as another option in addition to
+                              // the full source path remap.
+                              if (do_truncate_remapping_names) {
+                                FileSpec build_path(key.AsCString());
+                                FileSpec source_path(DBGSourcePath.c_str());
+                                build_path.RemoveLastPathComponent();
+                                build_path.RemoveLastPathComponent();
+                                source_path.RemoveLastPathComponent();
+                                source_path.RemoveLastPathComponent();
                                 module_sp->GetSourceMappingList().Append(
-                                    key, ConstString(DBGSourcePath), true);
-                                // With version 2 of DBGSourcePathRemapping, we
-                                // can chop off the last two filename parts
-                                // from the source remapping and get a more
-                                // general source remapping that still works.
-                                // Add this as another option in addition to
-                                // the full source path remap.
-                                if (do_truncate_remapping_names) {
-                                  FileSpec build_path(key.AsCString());
-                                  FileSpec source_path(DBGSourcePath.c_str());
-                                  build_path.RemoveLastPathComponent();
-                                  build_path.RemoveLastPathComponent();
-                                  source_path.RemoveLastPathComponent();
-                                  source_path.RemoveLastPathComponent();
-                                  module_sp->GetSourceMappingList().Append(
-                                      ConstString(build_path.GetPath().c_str()),
-                                      ConstString(source_path.GetPath().c_str()), true);
-                                }
+                                    ConstString(build_path.GetPath().c_str()),
+                                    ConstString(source_path.GetPath().c_str()),
+                                    true);
                               }
-                              return true;
-                            });
-                      }
+                            }
+                            return true;
+                          });
+                    }
 
-                      // If we have a DBGBuildSourcePath + DBGSourcePath pair,
-                      // append those to the source path remappings.
-
-                      plist.GetValueAsString("DBGBuildSourcePath",
-                                             DBGBuildSourcePath);
-                      plist.GetValueAsString("DBGSourcePath", DBGSourcePath);
-                      if (!DBGBuildSourcePath.empty() &&
-                          !DBGSourcePath.empty()) {
-                        if (DBGSourcePath[0] == '~') {
-                          FileSpec resolved_source_path(DBGSourcePath.c_str());
-                          FileSystem::Instance().Resolve(resolved_source_path);
-                          DBGSourcePath = resolved_source_path.GetPath();
-                        }
-                        module_sp->GetSourceMappingList().Append(
-                            ConstString(DBGBuildSourcePath),
-                            ConstString(DBGSourcePath), true);
+
+                    if (!DBGBuildSourcePath.empty() && !DBGSourcePath.empty()) {
+                      if (DBGSourcePath[0] == '~') {
+                        FileSpec resolved_source_path(DBGSourcePath.c_str());
+                        FileSystem::Instance().Resolve(resolved_source_path);
+                        DBGSourcePath = resolved_source_path.GetPath();
                       }
+                      module_sp->GetSourceMappingList().Append(
+                          ConstString(DBGBuildSourcePath),
+                          ConstString(DBGSourcePath), true);
                     }
                   }
                 }

         }
 
         symbol_vendor->AddSymbolFileRepresentation(dsym_objfile_sp);
+        if (repro::Generator *g =
+                repro::Reproducer::Instance().GetGenerator()) {
+          repro::FileProvider &fp = g->GetOrCreate<repro::FileProvider>();
+          fp.recordInterestingDirectory(dsym_root);
+        }
         return symbol_vendor;
       }
     }

lldb/source/Utility/Reproducer.cpp

   os << m_cwd << "\n";
 }
 
+void FileProvider::recordInterestingDirectory(const llvm::Twine &dir) {
+  if (m_collector)
+    m_collector->addDirectory(dir);
+}
+
 void ProviderBase::anchor() {}
 char CommandProvider::ID = 0;
 char FileProvider::ID = 0;

lldb/test/Shell/Reproducer/TestDSYM.test

+# 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