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

[Ellis Hoag via Phabricator via llvm-commits]

ellis added inline comments.


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:663-664
+        llvm::append_range(Objects, *DsymObjectsOrErr);
+    } else
+      error(DsymObjectsOrErr.takeError());
   }
----------------
jhenderson wrote:
> 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.
> 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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115418