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

[Ellis Hoag via Phabricator via llvm-commits]

ellis added inline comments.


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:4725-4727
+Expected<bool>
+MachOObjectFile::isDsymBundle(const StringRef Path,
+                              std::vector<std::string> &BinaryPaths) {
----------------
jhenderson wrote:
> clayborg wrote:
> > 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.
> 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.
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
```


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:4751
+      break;
+    default: /*ignore*/;
+    }
----------------
jhenderson wrote:
> This "ignore" is slightly surprising to me. Should we not emit an error of some kind for this case?
The rest of the file types seem to be uncommon and I'd rather not change the behavior of `llvm-dwarfdump` without knowing if `.dSYM`s ever use these other cases.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115418