[PATCH] D121065: [X86] Fix MCSymbolizer interface for X86Disassembler

[Maksim Panchenko via Phabricator via llvm-commits]

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