[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:
> 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?
It's likely there for historical reasons that I'm unaware of. I'm fine with removing it in a separate commit since it's not a part of the fix.


================
Comment at: llvm/unittests/MC/X86/X86MCDisassemblerTest.cpp:90-91
+};
+
+TEST(X86Disassembler, X86MCSymbolizerTest) {
+  if (!getContext())
----------------
skan wrote:
> 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`.
This diff clearly fixes an existing interface and needs a test and the only test possible is the unit test.

The only question is if this interface fix should be committed separately from its usage in D120928. I'm fine either way, but I can see a theoretical benefit in the split of commits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121065