[dwarf][NFC] Move expandBundle() to MachO.h

[ellis]

Summary

The function expandBundle() is defined both in llvm-dwarfdump.cpp and
llvm-gsymutil.cpp in the exact same way. Reduce code duplication by
moving this function to the MachOObjectFile class.

[ellis]

Rename function and change behavior slightly.

[clayborg]

llvm/lib/Object/MachOObjectFile.cpp

4725–4727

My question here is: if an error is returned, will it make any sense to the caller? The only error returned here is if something goes wrong during directory iteration. Which could be a few things:

• Path specifies a directory with ".dSYM" extension (do we are about case here?) and it doesn't the directory Path + "Context/Resources/DWARF" doesn't exist which would return something like "directory does not exist". Will it specify which directory doesn't exist? The user could specify "/path/to/foo.dSYM" that is a valid directory but it contains no DWARF bundle stuff inside. Just wondering if this will make sense to show to the user?
• We can't get sys::fs::file_status on a file in the Path + "Context/Resources/DWARF" + File file which would return something like "permission denied" or something else. If the error doesn't contain the path that caused the problem the user will be confused.

And I wonder if the callers of this function should show those errors to the user and would it make any sense to the user? If the errors don't make sense, then it might be good to just return a "bool" instead of a "Expected<bool>", or just return a std::vector<std::string> that is empty or that contained any successful paths that were already extracted.

If we want to keep the errors we should make the error message more clear and include information about what path caused the problem with the EC error code appended. So for the first case where sys::fs::directory_iterator constructor fills in an error, maybe returning something like "unable to iterate over directory %s: %s" where the first %s is the Path and the second is the error string. Something similar for the second?

Let me know what your thoughts are.

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp

651–655

Avoid having to take the error and test it by restructuring the code.

llvm/tools/llvm-gsymutil/llvm-gsymutil.cpp

389–393

Avoid having to take the error and test it by restructuring the code.

[jhenderson]

llvm/include/llvm/Object/MachO.h

655–657

Probably need to distinguish when the fails with an error versus when it will return false, in the comments.

658–659

a) I wouldn't bother with const on the StringRef, since they're inately const anyway (unless you reassign them of course).
b) I don't think "is*" is a sensible name for a function that does some vector population. Perhaps "loadDsymBundle" or "findDsymObjectMembers" or something to that effect.

llvm/lib/Object/MachOObjectFile.cpp

4725–4727

I think we should return the error, with additional context (i.e. what path is bad etc), and allow client code to decide what to do with it: lower-level libraries shouldn't generally assume that clients don't care about when errors exist.

When issue that is worth thinking about: we could get into a situation where the code returns an error, but the vector has been modified. I'd at least mention that in the event of an error, the vector's content is indeterminate, but perhaps better should be: make a copy of the vector and replace it, in the event of an error after the main for loop has been started.

4740

4751

This "ignore" is slightly surprising to me. Should we not emit an error of some kind for this case?

[ellis]

Update.

llvm/lib/Object/MachOObjectFile.cpp

4725–4727

Thanks for the helpful comments @clayborg and @jhenderson! This function really has three uses

1. Decide if the given path is a .dSYM.
   • From the original code this is done by simply checking if it is a directory that ends in .dSYM. If it's not a bundle, we return an empty list.
2. Report any filesystem errors to the user.
   • This is rather important and I've updated the returned errors to hold more info.
3. Provide a list of binaries in the .dSYM.

The biggest difference is when there are no binaries found but the path looks like a .dSYM.

$ tree
.
└── a.out.dSYM
    └── Contents
        └── Resources
            └── DWARF

4 directories, 0 files

# Original error:
$ llvm-dwarfdump a.out.dSYM/
error: a.out.dSYM/: Is a directory

# New error:
$ llvm-dwarfdump a.out.dSYM/
error: a.out.dSYM/: no objects found in dSYM bundle

4751

The rest of the file types seem to be uncommon and I'd rather not change the behavior of llvm-dwarfdump without knowing if .dSYMs ever use these other cases.

https://en.cppreference.com/w/cpp/filesystem/file_type

[jhenderson]

llvm/include/llvm/Object/MachO.h

655–657

I don't think the updated comment actually resolves this point. Without looking at the function signature for a moment, this comment implies that one of two things will be returned: either an empty vector or a non-empty vector, so returning an Expected isn't expected from the comment. I think it would be useful to add something high-level to explain when an error might be returned.

[ellis]

Update comment.

[ellis]

llvm/include/llvm/Object/MachO.h

655–657

Thanks, I've updated the comment to explain when an error is returned.

[jhenderson]

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp

656–657

My reading of the relatively recently updated coding standards is that if part of an if statement has braces, the corresponding else clause(s) also need braces. Applies in the llvm-gsymutil case too.

Looking at the findDsymObjectMembers, it looks like there is still missing context, in the event of a filesystem error, so you'd end up with a message that says something like: error: foo.dsym: access denied, which doesn't explain the what was happening at the time. I'm not sure the function itself should necessarily add this context, but could be persuaded it should. My thinking was that we should use the Prefix taking error method, and add the context that way, e.g. error("whilst loading objects from DSYM bundle", DsymObjectsOrErr.takeError()). I believe this would result in a message like: error: whilst loading objects from DSYM bundle: foo.dsym: access denied.

Thoughts? Downside is that the "no objects" case will end up repeating the context a bit more than is strictly necessary.

[ellis]

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp

656–657

Looking at the findDsymObjectMembers, it looks like there is still missing context, in the event of a filesystem error, so you'd end up with a message that says something like: error: foo.dsym: access denied, which doesn't explain the what was happening at the time.

I created a fake dSYM with an object with the wrong permissions to simulate a filesystem error. I actually think the error message here is acceptable, but let me know what you think.

The other error cases are when the dSYM directory itself has the wrong permissions or does not have the Contents/Resources/DWARF structure. The error for b.dSYM might be the weakest/most confusing.

$ tree a.dSYM/ b.dSYM/ c.dSYM/
a.dSYM/
└── Contents
    └── Resources
        └── DWARF
            └── bad
b.dSYM/ [error opening dir]
c.dSYM/
$ llvm-dwarfdump a.dSYM
error: a.dSYM/Contents/Resources/DWARF/bad: Permission denied
$ llvm-dwarfdump b.dSYM
error: 'b.dSYM/Contents/Resources/DWARF': Permission denied
$ llvm-dwarfdump c.dSYM
error: 'c.dSYM/Contents/Resources/DWARF': No such file or directory

My thinking was that we should use the Prefix taking error method, and add the context that way

If we pass in the error method we will sacrifice some generality, but we gain the ability to have very specific error messages.

Another option would be to replace createFileError() with createStringError() with a more specific message.

[ellis]

Add brackets.

[jhenderson]

I'm happy to go with whatever others' preference is on the error messages. As you say, the context is reasonable. LGTM, but please wait for @clayborg too.

[clayborg]

LGTM!

llvm/lib/Object/MachOObjectFile.cpp

4727

That second error looks great. You might want to see what the error looks like if you have a ".dSYM" directory but with no paths inside of it. The directory iterator will return an error if any of the "Contents", "Resources" or "DWARF do not exist and that error might be a bit cryptic.

So a few extra things to try just to make sure errors are not too bad:

• no user permissions for a valid dSYM bundle
• missing directories inside a dSYM bundle ("Contents", "Resources", "DWARF")

[ellis]

Add specific error message when bundle.dSYM/Contents/Resources/DWARF is not a directory.

[jhenderson]

llvm/lib/Object/MachOObjectFile.cpp

4737–4739

I guess this should have a test case somewhere :)

llvm/include/llvm/Object/MachO.h

static std::string getVersionString(uint32_t version) {
 
     SmallString<32> Version;
     Version = utostr(major) + "." + utostr(minor);
     if (update != 0)
       Version += "." + utostr(update);
     return std::string(std::string(Version.str()));
   }
 
+  /// If the input path is a .dSYM bundle (as created by the dsymutil tool),
+  /// return a list of individual entries for each of the object files inside
+  /// the bundle otherwise return the input path.
+  static Expected<SmallVector<std::string, 1>>
+  expandBundle(const StringRef InputPath);
+
 private:
   MachOObjectFile(MemoryBufferRef Object, bool IsLittleEndian, bool Is64Bits,
                   Error &Err, uint32_t UniversalCputype = 0,
                   uint32_t UniversalIndex = 0);
 
   uint64_t getSymbolValueImpl(DataRefImpl Symb) const override;
 
   union {

llvm/lib/Object/MachOObjectFile.cpp

 #include "llvm/Object/Error.h"
 #include "llvm/Object/MachO.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Object/SymbolicFile.h"
 #include "llvm/Support/DataExtractor.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/SwapByteOrder.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <cstddef>
 #include <cstdint>
 #include <cstring>
 #include <limits>
return make_error<GenericBinaryError>("Unrecognized MachO magic number",
                                         object_error::invalid_file_type);
 }
 
 StringRef MachOObjectFile::mapDebugSectionName(StringRef Name) const {
   return StringSwitch<StringRef>(Name)
       .Case("debug_str_offs", "debug_str_offsets")
       .Default(Name);
 }
+
+Expected<SmallVector<std::string, 1>>
+MachOObjectFile::expandBundle(const StringRef InputPath) {
+  SmallVector<std::string, 1> BundlePaths;
+  SmallString<256> BundlePath(InputPath);
+  // Normalize input path. This is necessary to accept `bundle.dSYM/`.
+  sys::path::remove_dots(BundlePath);
+  // Manually open up the bundle to avoid introducing additional dependencies.
+  if (sys::fs::is_directory(BundlePath) &&
+      sys::path::extension(BundlePath) == ".dSYM") {
+    std::error_code EC;
+    sys::path::append(BundlePath, "Contents", "Resources", "DWARF");
+    for (sys::fs::directory_iterator Dir(BundlePath, EC), DirEnd;
+         Dir != DirEnd && !EC; Dir.increment(EC)) {
+      const std::string &Path = Dir->path();
+      sys::fs::file_status Status;
+      if (auto EC = sys::fs::status(Path, Status))
+        return errorCodeToError(EC);
+      switch (Status.type()) {
+      case sys::fs::file_type::regular_file:
+      case sys::fs::file_type::symlink_file:
+      case sys::fs::file_type::type_unknown:
+        BundlePaths.push_back(Path);
+        break;
+      default: /*ignore*/;
+      }
+    }
+    if (EC)
+      return errorCodeToError(EC);
+  }
+  if (!BundlePaths.size())
+    BundlePaths.push_back(InputPath.str());
+  return BundlePaths;
+}

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp

 #include "llvm/Object/Archive.h"
 #include "llvm/Object/MachOUniversal.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/MemoryBuffer.h"
-#include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/ToolOutputFile.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cstdlib>
 
 using namespace llvm;
static bool handleFile(StringRef Filename, HandlerFn HandleObj,
                        raw_ostream &OS) {
   ErrorOr<std::unique_ptr<MemoryBuffer>> BuffOrErr =
       MemoryBuffer::getFileOrSTDIN(Filename);
   error(Filename, BuffOrErr.getError());
   std::unique_ptr<MemoryBuffer> Buffer = std::move(BuffOrErr.get());
   return handleBuffer(Filename, *Buffer, HandleObj, OS);
 }
 
-/// If the input path is a .dSYM bundle (as created by the dsymutil tool),
-/// replace it with individual entries for each of the object files inside the
-/// bundle otherwise return the input path.
-static std::vector<std::string> expandBundle(const std::string &InputPath) {
-  std::vector<std::string> BundlePaths;
-  SmallString<256> BundlePath(InputPath);
-  // Normalize input path. This is necessary to accept `bundle.dSYM/`.
-  sys::path::remove_dots(BundlePath);
-  // Manually open up the bundle to avoid introducing additional dependencies.
-  if (sys::fs::is_directory(BundlePath) &&
-      sys::path::extension(BundlePath) == ".dSYM") {
-    std::error_code EC;
-    sys::path::append(BundlePath, "Contents", "Resources", "DWARF");
-    for (sys::fs::directory_iterator Dir(BundlePath, EC), DirEnd;
-         Dir != DirEnd && !EC; Dir.increment(EC)) {
-      const std::string &Path = Dir->path();
-      sys::fs::file_status Status;
-      EC = sys::fs::status(Path, Status);
-      error(Path, EC);
-      switch (Status.type()) {
-      case sys::fs::file_type::regular_file:
-      case sys::fs::file_type::symlink_file:
-      case sys::fs::file_type::type_unknown:
-        BundlePaths.push_back(Path);
-        break;
-      default: /*ignore*/;
-      }
-    }
-    error(BundlePath, EC);
-  }
-  if (!BundlePaths.size())
-    BundlePaths.push_back(InputPath);
-  return BundlePaths;
-}
-
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
 
   // Flush outs() when printing to errs(). This avoids interleaving output
   // between the two.
   errs().tie(&outs());
 
   llvm::InitializeAllTargetInfos();
#undef HANDLE_DWARF_SECTION
 
   // Defaults to a.out if no filenames specified.
   if (InputFilenames.empty())
     InputFilenames.push_back("a.out");
 
   // Expand any .dSYM bundles to the individual object files contained therein.
   std::vector<std::string> Objects;
   for (const auto &F : InputFilenames) {
-    auto Objs = expandBundle(F);
-    llvm::append_range(Objects, Objs);
+    auto ObjsOrErr = MachOObjectFile::expandBundle(F);
+    if (auto Err = ObjsOrErr.takeError())
+      error(F, std::move(Err));
+    llvm::append_range(Objects, *ObjsOrErr);
   }
 
   bool Success = true;
   if (Verify) {
     for (auto Object : Objects)
       Success &= handleFile(Object, verifyObjectFile, OutputFile.os());
   } else if (Statistics) {
     for (auto Object : Objects)
       Success &= handleFile(Object, collectStatsForObjectFile, OutputFile.os());
   } else if (ShowSectionSizes) {
     for (auto Object : Objects)
       Success &= handleFile(Object, collectObjectSectionSizes, OutputFile.os());
   } else {
     for (auto Object : Objects)
       Success &= handleFile(Object, dumpObjectFile, OutputFile.os());
   }
 
   return Success ? EXIT_SUCCESS : EXIT_FAILURE;
 }

llvm/tools/llvm-gsymutil/llvm-gsymutil.cpp

 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Object/MachOUniversal.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/MemoryBuffer.h"
-#include "llvm/Support/Path.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cstring>
 #include <inttypes.h>

 
 static void error(StringRef Prefix, std::error_code EC) {
   if (!EC)
     return;
   errs() << Prefix << ": " << EC.message() << "\n";
   exit(1);
 }
 
-/// If the input path is a .dSYM bundle (as created by the dsymutil tool),
-/// replace it with individual entries for each of the object files inside the
-/// bundle otherwise return the input path.
-static std::vector<std::string> expandBundle(const std::string &InputPath) {
-  std::vector<std::string> BundlePaths;
-  SmallString<256> BundlePath(InputPath);
-  // Manually open up the bundle to avoid introducing additional dependencies.
-  if (sys::fs::is_directory(BundlePath) &&
-      sys::path::extension(BundlePath) == ".dSYM") {
-    std::error_code EC;
-    sys::path::append(BundlePath, "Contents", "Resources", "DWARF");
-    for (sys::fs::directory_iterator Dir(BundlePath, EC), DirEnd;
-         Dir != DirEnd && !EC; Dir.increment(EC)) {
-      const std::string &Path = Dir->path();
-      sys::fs::file_status Status;
-      EC = sys::fs::status(Path, Status);
-      error(Path, EC);
-      switch (Status.type()) {
-      case sys::fs::file_type::regular_file:
-      case sys::fs::file_type::symlink_file:
-      case sys::fs::file_type::type_unknown:
-        BundlePaths.push_back(Path);
-        break;
-      default: /*ignore*/;
-      }
-    }
-    error(BundlePath, EC);
-  }
-  if (!BundlePaths.size())
-    BundlePaths.push_back(InputPath);
-  return BundlePaths;
-}
-
 static uint32_t getCPUType(MachOObjectFile &MachO) {
   if (MachO.is64Bit())
     return MachO.getHeader64().cputype;
   else
     return MachO.getHeader().cputype;
 }
 
 /// Return true if the object file has not been filtered by an --arch option.
static llvm::Error convertFileToGSYM(raw_ostream &OS) {
   std::string OutFile = OutputFilename;
   if (OutFile.empty()) {
     OutFile = ConvertFilename;
     OutFile += ".gsym";
   }
 
   OS << "Input file: " << ConvertFilename << "\n";
 
-  auto Objs = expandBundle(ConvertFilename);
-  llvm::append_range(Objects, Objs);
+  auto ObjsOrErr = MachOObjectFile::expandBundle(ConvertFilename);
+  if (auto Err = ObjsOrErr.takeError())
+    error(ConvertFilename, std::move(Err));
+  llvm::append_range(Objects, *ObjsOrErr);
 
   for (auto Object : Objects) {
     if (auto Err = handleFileConversionToGSYM(Object, OutFile))
       return Err;
   }
   return Error::success();
 }
 
 static void doLookup(GsymReader &Gsym, uint64_t Addr, raw_ostream &OS) {