[PATCH] D100583: [llvm-objdump] Add an llvm-otool tool

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

Thanks!

In D100583#2695291 <https://reviews.llvm.org/D100583#2695291>, @alexshap wrote:

> Hi! Thanks for working on this, I actually had some similar plans in the past.
> One question (I believe that you have already explored it, so I'm asking mostly for the future reference) - did you choose llvm-objdump because it's the closest (from the available options) counterpart to otool featurewise ?
> (e.g. closer than llvm-readobj) ?

Several reasons:

1. llvm-objdump is "philosophically" LLVM's user-facing tool, while readobj is more internal tool for use by tests. otool is user-facing.
2. From a more practical perspective, `llvm-objdump --macho`'s output (with the right flags, which I'm using here) is already extremely close to otool's output
3. `man otool` on macOS refers to a mythological llvm-otool that's based on llvm-objdump, so this patch makes that mythological tool a reality :)



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:104
+    // TODO Replace this with OptTable API once it adds extrahelp support.
+    outs() << "\nPass @FILE as argument to read options from FILE.\n";
+  }
----------------
MaskRay wrote:
> I know the messages are not super consistent in the tool, but https://llvm.org/docs/CodingStandards.html#error-and-warning-messages Usually we don't have a period and don't capitalize.
We're somewhat consistent about this particular string: `% grep -Ri 'Pass @FILE as argument to read options from FILE' llvm/tools` :)

(Also, it's just moving around in this change.)


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2995
   ToolName = argv[0];
+  std::unique_ptr<CommonOptTable> T;
+  OptSpecifier Unknown, HelpFlag, HelpHiddenFlag, VersionFlag;
----------------
MaskRay wrote:
> Use `Optional<CommonOptTable>` and emplace.
>From what I can tell, that doesn't work with subclassing: Optional<CommonOptTable> reserves space for an CommonOptTable, and OtoolOptTable / ObjdumpOptTable might be larger. (Currently they aren't, but OptionalStorage::emplace() also tries to construct a T, in this case a CommonOptTable, and doesn't have anything to construct a subclass)


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:3045
     cl::PrintVersionMessage();
-    outs() << '\n';
-    TargetRegistry::printRegisteredTargetsForVersion(outs());
-    exit(0);
+    if (!Is("otool")) {
+      outs() << '\n';
----------------
MaskRay wrote:
> Oh, otool doesn't like the ` TargetRegistry::printRegisteredTargetsForVersion` message?
Probably not? We can decide, I guess :) Mach-O files are intel or arm in practice, so listing all targets isn't super interesting, and vanilla otool's help output is fairly laconic…


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

https://reviews.llvm.org/D100583