This is an archive of the discontinued LLVM Phabricator instance.

[MC] Add parameter `Address` to MCInstPrinter::printInst
ClosedPublic

Authored by MaskRay on Jan 3 2020, 11:45 AM.

Details

Summary

printInst prints a branch/call instruction as b offset (there are many
variants on various targets) instead of b address.

It is a convention to use address instead of offset in most external
symbolizers/disassemblers. This difference makes llvm-objdump -d
output unsatisfactory.

Add uint64_t Address to printInst, so that it can pass the argument to
printInstruction. The next step is to add the argument to
printInstruction (generated by tablegen from the instruction set
description). We can gradually migrate targets to print address
instead of offset.

Diff Detail

Event Timeline

MaskRay created this revision.Jan 3 2020, 11:45 AM
MaskRay added a comment.EditedJan 3 2020, 11:48 AM

I should mention that MCDisassembler::getInstruction (which is used together with printInst in a dissasembler) accepts uint64_t Address.

virtual DecodeStatus getInstruction(MCInst &Instr, uint64_t &Size,
                                    ArrayRef<uint8_t> Bytes, uint64_t Address,
                                    raw_ostream &VStream,
                                    raw_ostream &CStream) const = 0;

In MCAsmStreamer, a branch/call instruction always takes a kExpr, never a kImmediate. A symbol will be printed.

Unit tests: pass. 61176 tests passed, 0 failed and 729 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

MaskRay updated this revision to Diff 236120.Jan 3 2020, 1:26 PM
-  virtual void printInst(const MCInst *MI, raw_ostream &OS, StringRef Annot,
-                         const MCSubtargetInfo &STI) = 0;
+  virtual void printInst(const MCInst *MI, uint64_t Address, StringRef Annot,
+

Move OS to the last parameter to be consistent with other print* member functions.
We have to touch the code anyway, so just do a bit more work here.

Unit tests: pass. 61176 tests passed, 0 failed and 729 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

jhenderson accepted this revision.Jan 6 2020, 12:56 AM

I don't see any problems with this, so LGTM, although with the caveat that I'm not familiar with some of the usage sites, so it might be worth getting others to confirm the value passed in for the address makes sense in some situations (e.g. llvm-mca).

This revision is now accepted and ready to land.Jan 6 2020, 12:56 AM
This revision was automatically updated to reflect the committed changes.