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

Tue Apr 20 03:54:55 PDT 2021

thakis added a comment.

In D100583#2700448 <https://reviews.llvm.org/D100583#2700448>, @mstorsjo wrote:

> I think the new frontend also needs to be added to `LLVM_TEST_DEPENDS` in llvm/test/CMakeLists.txt, otherwise the symlink isn't created if you e.g. do `check-llvm` straight from a fresh build dir.

Done.

(All the other comments too.)

================
Comment at: llvm/test/tools/llvm-objdump/MachO/relocations.test:14
+
+NONVERBOSE:      Relocation information (__TEXT,__text) 2 entries
+NONVERBOSE-NEXT: address  pcrel length extern type    scattered symbolnum/value
----------------
MaskRay wrote:
> It'd be good to change CHECK to VERBOSE and NONVERBOSE to a normal prefix.
changed CHECK to VERBOSE. What's a "normal prefix"? But added `llvm-objdump --macho -r --non-verbose` to the test too to make it clearer where the NONVERBOSE prefix comes from.

================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2870
+    FilterSections.push_back("__TEXT,__text");
+  NonVerbose = !(InputArgs.hasArg(OTOOL_v) || InputArgs.hasArg(OTOOL_V) ||
+                 InputArgs.hasArg(OTOOL_o));
----------------
MaskRay wrote:
> Use "negative booleans", which may easily lead to double negations, especially that what have the positive option names now...
Yes, will rename in a follow-up. I think with the cl::opt parsing, it had to be named like this, but now it doesn't have to.

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

https://reviews.llvm.org/D100583