[PATCH] D121065: [X86] Fix MCSymbolizer interface for X86Disassembler
Maksim Panchenko via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Mar 6 18:07:25 PST 2022
maksfb added inline comments.
================
Comment at: llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp:1992-1993
- if(!tryAddingSymbolicOperand(immediate + pcrel, isBranch, insn.startLocation,
- insn.immediateOffset, insn.immediateSize,
- mcInst, Dis))
+ if (!tryAddingSymbolicOperand(immediate + pcrel, isBranch, insn.startLocation,
+ insn.immediateOffset, insn.length, mcInst, Dis))
mcInst.addOperand(MCOperand::createImm(immediate));
----------------
skan wrote:
> I am confused what you are fixing here. But it's not a clean fix at least, the comments of the function is not updated. Could you provide a LIT test?
If you check `MCDisassembler::tryAddingSymbolicOperand()` and `MCSymbolizer` interface, both expect instruction length to be passed to the user, but `X86Disassembler` is passing the size of the immediate. There's no current user of this interface on X86, hence I've added the unit test. https://reviews.llvm.org/D120928 includes the usage and a LIT test. Good point about the comments for the local `tryAddingSymbolicOperand()`.
================
Comment at: llvm/unittests/MC/X86/X86MCDisassemblerTest.cpp:79-85
+ bool tryAddingSymbolicOperand(MCInst &Inst, raw_ostream &CStream,
+ int64_t Value, uint64_t Address, bool IsBranch,
+ uint64_t Offset, uint64_t InstSize) override {
+ Operands.push_back({Value, Offset});
+ InstructionSize = InstSize;
+ return false;
+ }
----------------
skan wrote:
> Why redefine the function here?
This is the user of the interface for the unit test. It overrides a pure virtual function.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121065/new/
https://reviews.llvm.org/D121065
More information about the llvm-commits
mailing list