[PATCH] D88400: [llvm-objcopy][MachO] Add support for universal binaries

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:424-425
+    // The methods getAsArchive, getAsObjectFile, getAsIRObject of the class
+    // ObjectForArch return an Error in case of the type mismatch which needs to
+    // be consumed.
+    consumeError(ArOrErr.takeError());
----------------
alexshap wrote:
> jhenderson wrote:
> > I think my question is "why is it okay to ignore the type mismatch error?"
> Not sure what's expected here.
> The interface of ObjectForArch is somewhat suboptimal and if I am not mistaken it has already been discussed in the past (i don't remember - online or offline), refining ObjectForArch (from libObject) / doing a global refactoring is a non-goal for the current effort, 
> this interface had been around long before llvm-objcopy showed up, it's used in multiple places, but if i remember the previous discussions correctly changing this interface was not a completely trivial problem.
> Realistically this is what's going on here - the slice can be one of 3 types, the accessors return Expected and as you know the Error owned by an instance of Expected can't (and shouldn't) be ignored. In general, there are specifics of a particular call site, e.g. if the call site expects a slice of a particular type or if it handles just a few types then it can treat this error as a real error or consume it, but at the moment there is no other way to find out the actual type without calling these accessors. This makes sense to some extent but has the aforementioned unfortunate consequences. 
> I see a couple of options for now: 1. the current code (it is similar to llvm-dwarfdump.cpp) 2. one can add one more check that the error being consumed is indeed the type mismatch. Perhaps, isNotObjectErrorInvalidFileType should do the trick. On the functional side this doesn't really change anything (if the slice is not a macho object nor is it an archive we report an error anyway).
> @jhenderson  - does it answer your question / have I missed something ?
Sorry for the confusion. Perhaps just changing the comment slightly to something like the following would clarify things:
```
// The methods getAsArchive, getAsObjectFile, getAsIRObject of the class
// ObjectForArch return an Error in case of the type mismatch. We need to check each
// in turn to see what kind of file this is, so ignore errors produced along the way.
```

(The key bit being that this answers the question why we shouldn't report the error for ArOrErr - because we also need to see if it is an object file)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88400