[X86] Fix MCSymbolizer interface for X86Disassembler

[maksfb]

Summary

Fix a number of issues with MCSymbolizer::tryAddingSymbolicOperand()
in X86Disassembler:

• Pass instruction size instead of immediate size.
• Correctly adjust the value of PC-relative operands.
• Set operand offset to zero when the operand is specified implicitly.

[Amir]

This is awesome!
LGTM but let's add @skan in the loop.

[skan]

llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp

1992–1994

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?

llvm/unittests/MC/X86/X86MCDisassemblerTest.cpp

80–86

Why redefine the function here?

[maksfb]

llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp

1992–1994

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().

llvm/unittests/MC/X86/X86MCDisassemblerTest.cpp

80–86

This is the user of the interface for the unit test. It overrides a pure virtual function.

[maksfb]

Fix comments.

[skan]

llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp

1992–1994

If what you say is true, I can not see any value of the static version tryAddingSymbolicOperand. Why not just remove it?

llvm/unittests/MC/X86/X86MCDisassemblerTest.cpp

91–92

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.

[maksfb]

llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp

1992–1994

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.

llvm/unittests/MC/X86/X86MCDisassemblerTest.cpp

91–92

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.

[skan]

llvm/unittests/MC/X86/X86MCDisassemblerTest.cpp

91–92

Question: which line of X86MCDisassemblerTest.cpp would fail w/o this patch?

[maksfb]

llvm/unittests/MC/X86/X86MCDisassemblerTest.cpp

91–92

Most of expects.

[skan]

Thanks. LGTM

[maksfb]

Cool. Thanks for the review.

llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp

 /// tryAddingSymbolicOperand - trys to add a symbolic operand in place of the
 /// immediate Value in the MCInst.
 ///
 /// @param Value      - The immediate Value, has had any PC adjustment made by
 ///                     the caller.
 /// @param isBranch   - If the instruction is a branch instruction
 /// @param Address    - The starting address of the instruction
 /// @param Offset     - The byte offset to this immediate in the instruction
-/// @param Width      - The byte width of this immediate in the instruction
+/// @param InstSize   - Size of the instruction in bytes
 ///
 /// If the getOpInfo() function was set when setupForSymbolicDisassembly() was
 /// called then that function is called to get any symbolic information for the
-/// immediate in the instruction using the Address, Offset and Width.  If that
+/// immediate in the instruction using the Address, Offset and InstSize. If that
 /// returns non-zero then the symbolic information it returns is used to create
 /// an MCExpr and that is added as an operand to the MCInst.  If getOpInfo()
 /// returns zero and isBranch is true then a symbol look up for immediate Value
 /// is done and if a symbol is found an MCExpr is created with that, else
 /// an MCExpr with the immediate Value is created.  This function returns true
 /// if it adds an operand to the MCInst and false otherwise.
 static bool tryAddingSymbolicOperand(int64_t Value, bool isBranch,
                                      uint64_t Address, uint64_t Offset,
-                                     uint64_t Width, MCInst &MI,
+                                     uint64_t InstSize, MCInst &MI,
                                      const MCDisassembler *Dis) {
-  return Dis->tryAddingSymbolicOperand(MI, Value, Address, isBranch,
-                                       Offset, Width);
+  return Dis->tryAddingSymbolicOperand(MI, Value, Address, isBranch, Offset,
+                                       InstSize);
 }
 
 /// tryAddingPcLoadReferenceComment - trys to add a comment as to what is being
 /// referenced by a load instruction with the base register that is the rip.
 /// These can often be addresses in a literal pool.  The Address of the
 /// instruction and its immediate Value are used to determine the address
 /// being referenced in the literal pool entry.  The SymbolLookUp call back will
 /// return a pointer to a literal 'C' string if the referenced address is an
static void translateImmediate(MCInst &mcInst, uint64_t immediate,
   // Sign-extend the immediate if necessary.
 
   OperandType type = (OperandType)operand.type;
 
   bool isBranch = false;
   uint64_t pcrel = 0;
   if (type == TYPE_REL) {
     isBranch = true;
-    pcrel = insn.startLocation +
-            insn.immediateOffset + insn.immediateSize;
+    pcrel = insn.startLocation + insn.length;
     switch (operand.encoding) {
     default:
       break;
     case ENCODING_Iv:
       switch (insn.displacementSize) {
       default:
         break;
       case 1:
static void translateImmediate(MCInst &mcInst, uint64_t immediate,
   case TYPE_ZMM:
     mcInst.addOperand(MCOperand::createReg(X86::ZMM0 + (immediate >> 4)));
     return;
   default:
     // operand is 64 bits wide.  Do nothing.
     break;
   }
 
-  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));
 
   if (type == TYPE_MOFFS) {
     MCOperand segmentReg;
     segmentReg = MCOperand::createReg(segmentRegnums[insn.segmentOverride]);
     mcInst.addOperand(segmentReg);
   }
 }
 
#undef ENTRY
   } else {
     switch (insn.eaBase) {
     case EA_BASE_NONE:
       if (insn.eaDisplacement == EA_DISP_NONE) {
         debug("EA_BASE_NONE and EA_DISP_NONE for ModR/M base");
         return true;
       }
       if (insn.mode == MODE_64BIT){
-        pcrel = insn.startLocation +
-                insn.displacementOffset + insn.displacementSize;
+        pcrel = insn.startLocation + insn.length;
         tryAddingPcLoadReferenceComment(insn.startLocation +
                                         insn.displacementOffset,
                                         insn.displacement + pcrel, Dis);
         // Section 2.2.1.6
         baseReg = MCOperand::createReg(insn.addressSize == 4 ? X86::EIP :
                                                                X86::RIP);
       }
       else
#undef ENTRY
 
   displacement = MCOperand::createImm(insn.displacement);
 
   segmentReg = MCOperand::createReg(segmentRegnums[insn.segmentOverride]);
 
   mcInst.addOperand(baseReg);
   mcInst.addOperand(scaleAmount);
   mcInst.addOperand(indexReg);
-  if(!tryAddingSymbolicOperand(insn.displacement + pcrel, false,
-                               insn.startLocation, insn.displacementOffset,
-                               insn.displacementSize, mcInst, Dis))
+
+  const uint8_t dispOffset =
+      (insn.eaDisplacement == EA_DISP_NONE) ? 0 : insn.displacementOffset;
+
+  if (!tryAddingSymbolicOperand(insn.displacement + pcrel, false,
+                                insn.startLocation, dispOffset, insn.length,
+                                mcInst, Dis))
     mcInst.addOperand(displacement);
   mcInst.addOperand(segmentReg);
   return false;
 }
 
 /// translateRM - Translates an operand stored in the R/M (and possibly SIB)
 ///   byte of an instruction to LLVM form, and appends it to an MCInst.
 ///

llvm/unittests/MC/X86/CMakeLists.txt

+include_directories(
+  ${CMAKE_SOURCE_DIR}/lib/Target/X86
+  ${CMAKE_BINARY_DIR}/lib/Target/X86
+  )
+
+set(LLVM_LINK_COMPONENTS
+  MC
+  MCDisassembler
+  Target
+  X86Desc
+  X86Disassembler
+  )
+
+add_llvm_unittest(X86MCTests
+  X86MCDisassemblerTest.cpp
+  )

llvm/unittests/MC/X86/X86MCDisassemblerTest.cpp

+//===- X86MCDisassemblerTest.cpp - Tests for X86 MCDisassembler -----------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/MC/MCAsmInfo.h"
+#include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCDisassembler/MCDisassembler.h"
+#include "llvm/MC/MCDisassembler/MCSymbolizer.h"
+#include "llvm/MC/MCInst.h"
+#include "llvm/MC/MCRegisterInfo.h"
+#include "llvm/MC/MCSubtargetInfo.h"
+#include "llvm/MC/MCTargetOptions.h"
+#include "llvm/MC/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+struct Context {
+  const char *TripleName = "x86_64-pc-linux";
+  std::unique_ptr<MCRegisterInfo> MRI;
+  std::unique_ptr<MCAsmInfo> MAI;
+  std::unique_ptr<MCContext> Ctx;
+  std::unique_ptr<MCSubtargetInfo> STI;
+  std::unique_ptr<MCDisassembler> DisAsm;
+
+  Context() {
+    LLVMInitializeX86TargetInfo();
+    LLVMInitializeX86TargetMC();
+    LLVMInitializeX86Disassembler();
+
+    // If we didn't build x86, do not run the test.
+    std::string Error;
+    const Target *TheTarget = TargetRegistry::lookupTarget(TripleName, Error);
+    if (!TheTarget)
+      return;
+
+    MRI.reset(TheTarget->createMCRegInfo(TripleName));
+    MAI.reset(TheTarget->createMCAsmInfo(*MRI, TripleName, MCTargetOptions()));
+    STI.reset(TheTarget->createMCSubtargetInfo(TripleName, "", ""));
+    Ctx = std::make_unique<MCContext>(Triple(TripleName), MAI.get(), MRI.get(),
+                                      STI.get());
+
+    DisAsm.reset(TheTarget->createMCDisassembler(*STI, *Ctx));
+  }
+
+  operator bool() { return Ctx.get(); }
+  operator MCContext &() { return *Ctx; };
+};
+
+Context &getContext() {
+  static Context Ctxt;
+  return Ctxt;
+}
+} // namespace
+
+class X86MCSymbolizerTest : public MCSymbolizer {
+public:
+  X86MCSymbolizerTest(MCContext &MC) : MCSymbolizer(MC, nullptr) {}
+  ~X86MCSymbolizerTest() {}
+
+  struct OpInfo {
+    int64_t Value = 0;
+    uint64_t Offset = 0;
+  };
+  std::vector<OpInfo> Operands;
+  uint64_t InstructionSize = 0;
+
+  void reset() {
+    Operands.clear();
+    InstructionSize = 0;
+  }
+
+  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;
+  }
+
+  void tryAddingPcLoadReferenceComment(raw_ostream &cStream, int64_t Value,
+                                       uint64_t Address) override {}
+};
+
+TEST(X86Disassembler, X86MCSymbolizerTest) {
+  if (!getContext())
+    return;
+
+  X86MCSymbolizerTest *TestSymbolizer = new X86MCSymbolizerTest(getContext());
+  getContext().DisAsm->setSymbolizer(
+      std::unique_ptr<MCSymbolizer>(TestSymbolizer));
+
+  MCDisassembler::DecodeStatus Status;
+  MCInst Inst;
+  uint64_t InstSize;
+
+  auto checkBytes = [&](ArrayRef<uint8_t> Bytes) {
+    TestSymbolizer->reset();
+    Status =
+        getContext().DisAsm->getInstruction(Inst, InstSize, Bytes, 0, nulls());
+    ASSERT_TRUE(Status == MCDisassembler::Success);
+    EXPECT_EQ(TestSymbolizer->InstructionSize, InstSize);
+  };
+
+  auto checkOperand = [&](size_t OpNo, int64_t Value, uint64_t Offset) {
+    ASSERT_TRUE(TestSymbolizer->Operands.size() > OpNo);
+    EXPECT_EQ(TestSymbolizer->Operands[OpNo].Value, Value);
+    EXPECT_EQ(TestSymbolizer->Operands[OpNo].Offset, Offset);
+  };
+
+  // movq   $0xffffffffffffefe8,-0x1(%rip)
+  // Test that the value of the rip-relative operand is set correctly.
+  // The instruction address is 0 and the size is 12 bytes.
+  checkBytes(
+      {0x48, 0xc7, 0x05, 0xff, 0xff, 0xff, 0xff, 0xe8, 0xef, 0xff, 0xff});
+  checkOperand(0, /*next instr address*/ 11 - /*disp*/ 1, 3);
+  checkOperand(1, 0xffffffffffffefe8, 7);
+
+  // movq   $0xfffffffffffffef5,(%r12)
+  // Test that the displacement operand has an offset of 0, since it is not
+  // explicitly specified in the instruction.
+  checkBytes({0x49, 0xc7, 0x04, 0x24, 0xf5, 0xfe, 0xff, 0xff});
+  checkOperand(0, 0, 0);
+  checkOperand(1, 0xfffffffffffffef5, 4);
+
+  // movq    $0x80000, 0x80000
+  checkBytes(
+      {0x48, 0xc7, 0x04, 0x25, 0x00, 0x00, 0x08, 0x00, 0x00, 0x00, 0x08, 0x00});
+  checkOperand(0, 0x80000, 4);
+  checkOperand(1, 0x80000, 8);
+}