Depends on D72172.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Unit tests: pass. 61237 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
So, as is, this is NFC, it just adds a dead parameter to a virtual method, but eventually it will make its way through the layers to at the very least X86InstPrinterCommon::printPCRelImm, which will change llvm-objdump's behavior.
Do you think this will break any users? Should we add a flag to request the old behavior (print only the PC-relative offset)? I guess I suspect users are relying on our current behavior, but will be able to adapt.
I think we should do this, but let's wait for at least one other person to agree.
I ended up creating the two patches after I reviewed D71453. (The llvm-objdump -d b offset problem has bother me (and likely many others) a long time.)
My plan is:
- Teach MCInstPrinter::printInst and MCInstrPrinter::printInstruction to take an extra parameter Address (D72172 D72180)
- For target ABC, implement printBranchOperand, which is similar to printOperand, but takes Address into account.
- Migrate ABCInstrInfo.td direct branch/call instructions to use PrintMethod = "printBranchOperand", instead of the default "printOperand".
For users who want to retain the old behavior (offset instead of address), they can simply pass 0 to uint64_t Address.
There are a large number of llvm-objdump -d tests that test b offset instead of b address. (There are hundred of tests just in lld/ELF.) I am not very clear how we will migrate those tests. Eventually I hope llvm-objdump -d can behave like objdump -d. But before all tests are migrated, we may have to live with --print-target-address.
This change itself looks good to me. If I understand correctly from @rnk's comment, his suggestion is that there should be an option in llvm-objdump to revert to the old behaviour (which as you point it would be to just pass in 0 instead of the current address). This would mean that people used to the offset behaviour can choose to use that still. I think there is some benefit to this too, but I guess that it should actually be a change to D72172.