[PATCH] D62753: [llvm-lipo] Implement -archs

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: llvm/test/tools/llvm-lipo/archs-macho-binary-unknown.test:1
+# RUN: yaml2obj %s > %t
+
----------------
smeenai wrote:
> alexshap wrote:
> > compnerd wrote:
> > > Can you avoid the temporary using:
> > > 
> > > `yaml2obj %s | llvm-lip - -archs | FileCheck %s`
> > i think lipo doesn't support this (i.e. cat a.o | lipo - -archs wouldn't work), if we need this feature, we can add a proper support in llvm-lipo in the future, but i think it's a bit beyond the scope of the current patch.
> Seems like It Just Works™. I guess the command line parsing just handles it?
it works with llvm-lipo because in https://llvm.org/doxygen/Binary_8cpp_source.html#l00092
MemoryBuffer::getFileOrSTDIN does the thing https://llvm.org/doxygen/MemoryBuffer_8cpp_source.html#l00143 


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:154
+
+  llvm_unreachable("llvm-lipo action unspecified");
 }
----------------
smeenai wrote:
> smeenai wrote:
> > alexshap wrote:
> > > smeenai wrote:
> > > > alexshap wrote:
> > > > > smeenai wrote:
> > > > > > compnerd wrote:
> > > > > > > I don't think that this should be unreachable - if the action is unspecified, that can occur normally in the way that the user invokes the tool.  Perhaps this should be a proper error?
> > > > > > Lines 123 through 133 should ensure we have a single action argument of the expected type. My switch statement suggestion above should make that structure clearer.
> > > > > llvm_unreachable is for internal logical errors, here if the specified command line arguments are incorrect we should make use of reportError, the tool should not crash.
> > > > I actually think this would be an internal logic error:
> > > > 
> > > > * If the user hasn't specified exactly one action argument, we'll have errored out already.
> > > > * The only action arguments we currently support are `-verify_arch` and `-archs`.
> > > > * Therefore, if our option isn't one of them, something is pretty wrong with our program logic.
> > > > 
> > > > I'm okay with using `reportError` for this though.
> > > imo it's not. llvm_unreachable wouldn't be the right choice here for the following reason: for example, if a user has misspelled a flag or option the tool should not crash with assert/llvm_unreachable, it should exit normally (report error and exit with non-zero code)
> > > 
> > I agree, but if the user had misspelled a flag or option, we would have also handled it already (we check for unknown arguments explicitly above).
> > 
> > Basically, in order to have gotten to this switch statement, we should have performed all our input validation already. If we're ending up with an unknown option here, that means something is wrong with our input validation logic.
> Basically, under which circumstances could the `default` case be reached? I believe that with our current code we should never be able to reach it.
depending on how good our validation will be in the future, this invariant ("unreachable") might be preserved, if one day this invariant gets violated - okay, the tool will start crashing on some incorrectly specified flags. So this was more like a general consideration what the tool should do if a flag or a combination of flags has been specified incorrectly, in particular, it should not crash. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62753