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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: llvm/test/tools/llvm-lipo/archs-macho-binary-unknown.test:1
+# RUN: yaml2obj %s > %t
+
----------------
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?


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:154
+
+  llvm_unreachable("llvm-lipo action unspecified");
 }
----------------
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.


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:216
+LLVM_ATTRIBUTE_NORETURN
+static void printArchs(ArrayRef<OwningBinary<Binary>> InputBinaries) {
+  assert(InputBinaries.size() == 1 && "Incorrect number of input binaries");
----------------
alexshap wrote:
> alexshap wrote:
> > smeenai wrote:
> > > compnerd wrote:
> > > > Why not just make the parameter an `OwningBinary<Binary>` rather than the `ArrayRef` and then assert that a single entry is provided?
> > > Good idea. We'd probably also wanna take the parameter by const reference?
> > not sure it really matters, the thing is that in general lipo can have multiple input files,
> > different actions expect different number of input files, thus since in main() we dispatch handling of particular commands to different subroutines  i think it's clear / consistent enough. I'd prefer to not to make that "top-level" code less readable, although it's simple either way so i don't insist.
> discussed with @compnerd and we agreed on this, might be worth adding a comment in the code (in main) that the details/specifics of handling particular options (i.e. the number of expected input files) are inside the corresponding subroutines.
>  
What did you have in mind for the comment? I feel like with the way the switch is structured right now it's pretty obvious that all details of handling a particular option are handled inside the function for it, and having a comment saying that seems unnecessary.


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:227
+  else
+    llvm_unreachable("Unexpected binary format");
+
----------------
alexshap wrote:
> alexshap wrote:
> > smeenai wrote:
> > > compnerd wrote:
> > > > I don't think that this is necessarily unreachable - an invalid binary being given to the tool could hit this.  This should be a proper error.
> > > `readInputBinaries` above should have ensured we have a binary in the correct format.
> > i think here llvm_unreachable is appropriate since it should have been checked earlier, so here it's just a defensive assert (rather than do the same check and error reporting in multiple places)
> or convert it to assert
I think `llvm_unreachable` is better here, since the assert equivalent would be an `assert(0)` anyway.


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:208
+  // Prints trailing space and unknown in this format for compatibility with
+  // cctools lipo
+  Triple::ArchType Arch = MachOObjectFile::getArch(CPUType);
----------------
Nit: add a period at the end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62753