[PATCH] D60927: [WIP][llvm-objdump] Switch between ARM/Thumb based on mapping symbols.

[Eli Friedman via Phabricator via llvm-commits]

efriedma marked 2 inline comments as done.
efriedma added a comment.

In terms of how to generally improve disassembleObject, I guess there are a few different ways to think about it...

Currently, MCDisassembler::getInstruction is a "stateless" API which takes some bytes, and outputs a single instruction or instruction bundle.  (We currently cheat a little on Thumb: the Thumb disassembler has a "mutable" member that keeps track of "it" blocks on Thumb.  But that's mostly orthogonal.)  That has some nice properties: essentially, it's easy to understand what it's doing.  But it seems like it isn't rich enough for disassembling object files, so we have a bunch of code in llvm-objdump which has to deal with target-specific aspects of object files.  We could instead add a disassembler API which is intended specifically for disassembling object files, and formally has state: essentially, the llvm-objdump would just pass the symbols and raw bits, and the API would handle the various target-specific aspects of disassembly.

Not sure it's worth the effort, though; I'm not sure there would be any use for it outside of llvm-objdump itself.



================
Comment at: lib/Target/ARM/Disassembler/ARMDisassembler.cpp:441
                                              raw_ostream &CS) const {
+  if (STI.getFeatureBits()[ARM::ModeThumb])
+    return getThumbInstruction(MI, Size, Bytes, Address, OS, CS);
----------------
srhines wrote:
> Add logic to check if you are defaulting to Thumb mode here (i.e. that was the triple that was passed).
Not sure how that would work; getFeatureBits() only returns "on" or "off".  There's no reasonable way to check the default.  We should inherit the right default anyway when the target is used to construct the subtarget.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:576
 
+static bool isArm32Elf(const ObjectFile *Obj) {
+  return (Obj->isELF() &&
----------------
MaskRay wrote:
> Since you are adding `isArm32Elf`, can you add `isArm64Elf` and delete `isArmElf` if that orthogonal property is desired?
In some places the property we want is "can this object file have mapping symbols", which is currently equivalent to isArmElf.  I guess I can add a separate helper for that, and make it check "isArmElf32 || isArmElf64"?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60927