This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump] Implement `-Mreg-names-raw`/`-std` options.
ClosedPublic

Authored by ikudrin on Feb 4 2019, 4:54 AM.

Details

Summary

The --disassembler-options, or -M, is used to customize the disassembler
and affect its output.

This patch implements two options which allow selecting names for registers
for ARM architecture. The -raw is what I came across in practice, and -std
is the same as the default name set.

Other options and architectures might be added later.

Diff Detail

Repository
rL LLVM

Event Timeline

ikudrin created this revision.Feb 4 2019, 4:54 AM
evgeny777 added inline comments.Feb 4 2019, 5:09 AM
lib/Target/ARM/ARMRegisterInfo.td
16

Can we simply use "r" + string(Enc) for -Mreg-names-raw ?

grimar added inline comments.Feb 4 2019, 5:30 AM
lib/MC/MCInstPrinter.cpp
33

Maybe inline it?

lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp
83

You can avoid using else after return:

if (Opt == "reg-names-std") {
  DefaultAltIdx = ARM::NoRegAltName;
  return true;
} 
if (Opt == "reg-names-raw") {
  DefaultAltIdx = ARM::RegNamesRaw;
  return true;
} 
return false;
tools/llvm-objdump/llvm-objdump.cpp
1474

You do not need curly bracers here:

for (const auto &Opt : DisassemblerOptions)
  if (!IP->applyTargetSpecificCLOption(Opt))
    error("Unrecognized disassembler option: " + Opt);

I would also don't use auto because the type is not obvious.

ikudrin updated this revision to Diff 185281.Feb 5 2019, 4:17 AM

Addressed review comments.

ikudrin marked 3 inline comments as done.Feb 5 2019, 4:20 AM
ikudrin added inline comments.
lib/Target/ARM/ARMRegisterInfo.td
16

I don't think I understand you. Could you explain where in the code you expect that line to be added?

evgeny777 added inline comments.Feb 5 2019, 5:27 AM
lib/Target/ARM/ARMRegisterInfo.td
16

Well, this patch is actually needed for llvm-objdump/ARM to be compatible with GNU objdump and not much else.
So instead of trying to be smart I suggest implementing the subclass of ARMInstPrinter in llvm-objdump

class ARMObjdumpInstPrinter : public ARMInstPrinter {
  bool DumpRaw = false;

  const char *getRegisterNameRaw(unsigned RegNo);
public:
  void printRegName(raw_ostream &OS, unsigned RegNo) const override {
     if (!DumpRaw) {
        ARMInstPrinter::printRegName(OS, RegNo);
        return;
     }
     OS << markup("<reg:") << getRegisterNameRaw(RegNo) << markup(">");
  }
};
ikudrin marked an inline comment as done.Feb 6 2019, 9:16 AM
ikudrin added inline comments.
lib/Target/ARM/ARMRegisterInfo.td
16

Well, that could work. However, the instruction printer is created in a general way, through a common interface of targets. Moreover, the declaration of ARMInstrPrinter, laying inside the lib folders hierarchy, may be considered as an internal thing, and, as such, is not expected to be used outside the library.

evgeny777 added inline comments.Feb 6 2019, 10:17 AM
lib/Target/ARM/ARMRegisterInfo.td
16

May be proxy MCInstPrinter? You only have to implement printInst and printRegName

class ObjdumpInstPrinter : public MCInstPrinter {
  MCInstPrinter *Real;
  bool DumpRawOnArm;
public:
   void printInst(...) override { return Real->printInst(...); }
   const char* printRegName(...) override {
      if (!isArm() || !DumpRawOnArm)
         return Real->printRegName(...);
      // do stuff ...
   }
};
evgeny777 added inline comments.Feb 6 2019, 10:28 AM
lib/Target/ARM/ARMRegisterInfo.td
16

Nope, seems this won't work either as printRegName is called inside printInst

jhenderson added inline comments.Feb 19 2019, 1:53 AM
include/llvm/MC/MCInstPrinter.h
68

This comment should not mention llvm-objdump. It might be the only user now, but there's nothing stopping other users in the future.

69

False -> false. Use the @returns syntax too, so that it gets sensibly formatted by doxygen.

lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp
83

There's still an else if a few lines up here, which can just become a regular if, as @grimar proposed.

test/tools/llvm-objdump/ARM/reg-names.s
8

May be worth testing what happens if you specify both (i.e. show last-one-wins).

tools/llvm-objdump/llvm-objdump.cpp
1471

StringRef?

ikudrin updated this revision to Diff 187530.Feb 20 2019, 1:22 AM
  • Made DefaultAltIdx private.
  • Extended the test.
  • Addressed other review comments.
ikudrin marked 6 inline comments as done.Feb 20 2019, 1:25 AM
ikudrin added inline comments.
lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp
83

Oops! Didn't notice that. Thanks!

jhenderson added inline comments.Feb 20 2019, 3:56 AM
test/tools/llvm-objdump/ARM/reg-names.s
8–9

This is a duplicate of the check immediately above?

tools/llvm-objdump/llvm-objdump.cpp
1473

Test case for this error message?

ikudrin updated this revision to Diff 187574.Feb 20 2019, 6:54 AM
ikudrin marked an inline comment as done.

Updated the test.

ikudrin marked 2 inline comments as done.Feb 20 2019, 6:56 AM
jhenderson accepted this revision.Feb 21 2019, 2:00 AM

LGTM, although I don't really know anything about the ARM register stuff at all. Perhaps worth getting a second person to review before landing.

This revision is now accepted and ready to land.Feb 21 2019, 2:00 AM

Thanks! I'll wait a bit, but not very long. I believe the patch is safe because the default behavior is not changed.

This revision was automatically updated to reflect the committed changes.