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

[Kan Shengchen via Phabricator via llvm-commits]

skan 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));
----------------
maksfb wrote:
> 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()`.
If what you say is true, I can not see any value of the static version `tryAddingSymbolicOperand`. Why not just remove it?


================
Comment at: llvm/unittests/MC/X86/X86MCDisassemblerTest.cpp:90-91
+};
+
+TEST(X86Disassembler, X86MCSymbolizerTest) {
+  if (!getContext())
----------------
I think this unittest in the wrong place. You test X86MCSymbolizerTest here, which is defined in this unittest. It's meaningless. This unnit test should be added in D120928 and tests the real `X86MCSymbolizer`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121065/new/

https://reviews.llvm.org/D121065