[MCDisassembler] Disambiguate Size parameter in tryAddingSymbolicOperand()

[maksfb]

Summary

MCSymbolizer::tryAddingSymbolicOperand() overloaded the Size parameter
to specify either the instruction size or the operand size depending on
the architecture. However, for proper symbolic disassembly on X86, we
need to know both sizes, as an instruction can have two operands and
the instruction size cannot be calculated based on an operand offset
and its size. Hence, split Size into OpSize and InstSize.

For X86, the new interface allows to fix a couple of issues:

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

[maksfb]

Original attempt to fix X86 issue was committed and reverted in https://reviews.llvm.org/D121065. This is a proper fix that changes the interface.

[MaskRay]

llvm/unittests/MC/X86/CMakeLists.txt

11

Add X86Info

otherwise -DBUILD_SHARED_LIBS=on build will have a link error.

llvm/unittests/MC/X86/X86MCDisassemblerTest.cpp

26

or use x86_64-unknown-elf

63

Place X86MCSymbolizerTest in an anonymous namespace, too.

95

Delete

122

Consider testing something more complex, like 291(%rax,%r14,8)

128

Additionally test InstSize

137

Suggest reordering the instructions from simple to complex.

[MaskRay]

However, for proper symbolic disassembly on X86 we need to know both sizes.

Add some description why it is the case. IIUC InstSize allows we to know the size of the only operand. If there are multiple operands, we don't know their sizes.

[MaskRay]

llvm/unittests/MC/X86/CMakeLists.txt

2

Delete

[MaskRay]

Fix MCSymbolizer::tryAddingSymbolicOperand() interface

Add OpSize parameter to MCSymbolizer::tryAddingSymbolicOperand. "Fix" isn't clear.

[maksfb]

Thank you for the review.

llvm/unittests/MC/X86/X86MCDisassemblerTest.cpp

128

InstSize is checked in checkBytes().

[maksfb]

Address feedback.

[MaskRay]

Perhaps wait a day or so in case others have opinions.

llvm/lib/Target/SystemZ/Disassembler/SystemZDisassembler.cpp

78

The official syntax is /*InstSize=*/0

[maksfb]

Fix in-line comments.

[skan]

llvm/lib/Target/Sparc/Disassembler/SparcDisassembler.cpp

512

Why is "0" used as argument here? Is it unused?

llvm/unittests/MC/X86/X86MCDisassemblerTest.cpp

54

Is this function unused?

[maksfb]

llvm/lib/Target/Sparc/Disassembler/SparcDisassembler.cpp

512

Correct. Although for SPARC it's safe to use 4.

llvm/unittests/MC/X86/X86MCDisassemblerTest.cpp

54

Yes, thanks.

[maksfb]

Address comments.

[skan]

LGTM for X86 part.

[maksfb]

Rebase.

llvm/include/llvm-c/DisassemblerTypes.h

  * get the symbolic information for an operand of an instruction.  Typically
  * this is from the relocation information, symbol table, etc.  That block of
  * information is saved when the disassembler context is created and passed to
  * the call back in the DisInfo parameter.  The instruction containing operand
  * is at the PC parameter.  For some instruction sets, there can be more than
  * one operand with symbolic information.  To determine the symbolic operand
  * information for each operand, the bytes for the specific operand in the
  * instruction are specified by the Offset parameter and its byte widith is the
- * size parameter.  For instructions sets with fixed widths and one symbolic
- * operand per instruction, the Offset parameter will be zero and Size parameter
- * will be the instruction width.  The information is returned in TagBuf and is
- * Triple specific with its specific information defined by the value of
- * TagType for that Triple.  If symbolic information is returned the function
- * returns 1, otherwise it returns 0.
+ * OpSize parameter.  For instructions sets with fixed widths and one symbolic
+ * operand per instruction, the Offset parameter will be zero and InstSize
+ * parameter will be the instruction width.  The information is returned in
+ * TagBuf and is Triple specific with its specific information defined by the
+ * value of TagType for that Triple.  If symbolic information is returned the
+ * function * returns 1, otherwise it returns 0.
  */
-typedef int (*LLVMOpInfoCallback)(void *DisInfo, uint64_t PC,
-                                  uint64_t Offset, uint64_t Size,
+typedef int (*LLVMOpInfoCallback)(void *DisInfo, uint64_t PC, uint64_t Offset,
+                                  uint64_t OpSize, uint64_t InstSize,
                                   int TagType, void *TagBuf);
 
 /**
  * The initial support in LLVM MC for the most general form of a relocatable
  * expression is "AddSymbol - SubtractSymbol + Offset".  For some Darwin targets
  * this full form is encoded in the relocation information so that AddSymbol and
  * SubtractSymbol can be link edited independent of each other.  Many other
  * platforms only allow a relocatable expression of the form AddSymbol + Offset

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

 
 protected:
   // Subtarget information, for instruction decoding predicates if required.
   const MCSubtargetInfo &STI;
   std::unique_ptr<MCSymbolizer> Symbolizer;
 
 public:
   // Helpers around MCSymbolizer
-  bool tryAddingSymbolicOperand(MCInst &Inst,
-                                int64_t Value,
-                                uint64_t Address, bool IsBranch,
-                                uint64_t Offset, uint64_t InstSize) const;
+  bool tryAddingSymbolicOperand(MCInst &Inst, int64_t Value, uint64_t Address,
+                                bool IsBranch, uint64_t Offset, uint64_t OpSize,
+                                uint64_t InstSize) const;
 
   void tryAddingPcLoadReferenceComment(int64_t Value, uint64_t Address) const;
 
   /// Set \p Symzer as the current symbolizer.
   /// This takes ownership of \p Symzer, and deletes the previously set one.
   void setSymbolizer(std::unique_ptr<MCSymbolizer> Symzer);
 
   MCContext& getContext() const { return Ctx; }

llvm/include/llvm/MC/MCDisassembler/MCExternalSymbolizer.h

MCExternalSymbolizer(MCContext &Ctx,
                        std::unique_ptr<MCRelocationInfo> RelInfo,
                        LLVMOpInfoCallback getOpInfo,
                        LLVMSymbolLookupCallback symbolLookUp, void *disInfo)
     : MCSymbolizer(Ctx, std::move(RelInfo)), GetOpInfo(getOpInfo),
       SymbolLookUp(symbolLookUp), DisInfo(disInfo) {}
 
   bool tryAddingSymbolicOperand(MCInst &MI, raw_ostream &CommentStream,
                                 int64_t Value, uint64_t Address, bool IsBranch,
-                                uint64_t Offset, uint64_t InstSize) override;
+                                uint64_t Offset, uint64_t OpSize,
+                                uint64_t InstSize) override;
   void tryAddingPcLoadReferenceComment(raw_ostream &CommentStream,
                                        int64_t Value,
                                        uint64_t Address) override;
 };
 
 }
 
 #endif

llvm/include/llvm/MC/MCDisassembler/MCSymbolizer.h

public:
   /// symbol or an offset from a symbol. Relocations can also be used to enrich
   /// the symbolic expression.
   /// \param Inst      - The MCInst where to insert the symbolic operand.
   /// \param cStream   - Stream to print comments and annotations on.
   /// \param Value     - Operand value, pc-adjusted by the caller if necessary.
   /// \param Address   - Load address of the instruction.
   /// \param IsBranch  - Is the instruction a branch?
   /// \param Offset    - Byte offset of the operand inside the inst.
+  /// \param OpSize    - Size of the operand in bytes.
   /// \param InstSize  - Size of the instruction in bytes.
   /// \return Whether a symbolic operand was added.
   virtual bool tryAddingSymbolicOperand(MCInst &Inst, raw_ostream &cStream,
                                         int64_t Value, uint64_t Address,
                                         bool IsBranch, uint64_t Offset,
-                                        uint64_t InstSize) = 0;
+                                        uint64_t OpSize, uint64_t InstSize) = 0;
 
   /// Try to add a comment on the PC-relative load.
   /// For instance, in Mach-O, this is used to add annotations to instructions
   /// that use C string literals, as found in __cstring.
   virtual void tryAddingPcLoadReferenceComment(raw_ostream &cStream,
                                                int64_t Value,
                                                uint64_t Address) = 0;
 

llvm/lib/MC/MCDisassembler/MCDisassembler.cpp

 MCDisassembler::onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
                               ArrayRef<uint8_t> Bytes, uint64_t Address,
                               raw_ostream &CStream) const {
   return None;
 }
 
 bool MCDisassembler::tryAddingSymbolicOperand(MCInst &Inst, int64_t Value,
                                               uint64_t Address, bool IsBranch,
-                                              uint64_t Offset,
+                                              uint64_t Offset, uint64_t OpSize,
                                               uint64_t InstSize) const {
   if (Symbolizer)
-    return Symbolizer->tryAddingSymbolicOperand(
-        Inst, *CommentStream, Value, Address, IsBranch, Offset, InstSize);
+    return Symbolizer->tryAddingSymbolicOperand(Inst, *CommentStream, Value,
+                                                Address, IsBranch, Offset,
+                                                OpSize, InstSize);
   return false;
 }
 
 void MCDisassembler::tryAddingPcLoadReferenceComment(int64_t Value,
                                                      uint64_t Address) const {
   if (Symbolizer)
     Symbolizer->tryAddingPcLoadReferenceComment(*CommentStream, Value, Address);
 }

llvm/lib/MC/MCDisassembler/MCExternalSymbolizer.cpp

 // setupForSymbolicDisassembly() call then that function is called to get any
 // symbolic information at the Address for this instruction. 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 Value is done and if a symbol
 // is found an MCExpr is created with that, else an MCExpr with Value is
 // created. This function returns true if it adds an operand to the MCInst and
 // false otherwise.
-bool MCExternalSymbolizer::tryAddingSymbolicOperand(MCInst &MI,
-                                                    raw_ostream &cStream,
-                                                    int64_t Value,
-                                                    uint64_t Address,
-                                                    bool IsBranch,
-                                                    uint64_t Offset,
-                                                    uint64_t InstSize) {
+bool MCExternalSymbolizer::tryAddingSymbolicOperand(
+    MCInst &MI, raw_ostream &cStream, int64_t Value, uint64_t Address,
+    bool IsBranch, uint64_t Offset, uint64_t OpSize, uint64_t InstSize) {
   struct LLVMOpInfo1 SymbolicOp;
   std::memset(&SymbolicOp, '\0', sizeof(struct LLVMOpInfo1));
   SymbolicOp.Value = Value;
 
   if (!GetOpInfo ||
-      !GetOpInfo(DisInfo, Address, Offset, InstSize, 1, &SymbolicOp)) {
+      !GetOpInfo(DisInfo, Address, Offset, OpSize, InstSize, 1, &SymbolicOp)) {
     // Clear SymbolicOp.Value from above and also all other fields.
     std::memset(&SymbolicOp, '\0', sizeof(struct LLVMOpInfo1));
 
     // At this point, GetOpInfo() did not find any relocation information about
     // this operand and we are left to use the SymbolLookUp() call back to guess
     // if the Value is the address of a symbol.  In the case this is a branch
     // that always makes sense to guess.  But in the case of an immediate it is
     // a bit more questionable if it is an address of a symbol or some other
     // reference.  So if the immediate Value comes from a width of 1 byte,
-    // InstSize, we will not guess it is an address of a symbol.  Because in
+    // OpSize, we will not guess it is an address of a symbol.  Because in
     // object files assembled starting at address 0 this usually leads to
     // incorrect symbolication.
-    if (!SymbolLookUp || (InstSize == 1 && !IsBranch))
+    if (!SymbolLookUp || (OpSize == 1 && !IsBranch))
       return false;
 
     uint64_t ReferenceType;
     if (IsBranch)
        ReferenceType = LLVMDisassembler_ReferenceType_In_Branch;
     else
        ReferenceType = LLVMDisassembler_ReferenceType_InOut_None;
     const char *ReferenceName;

llvm/lib/Target/AArch64/Disassembler/AArch64Disassembler.cpp

static DecodeStatus DecodePCRelLabel19(MCInst &Inst, unsigned Imm,
                                        const MCDisassembler *Decoder) {
   int64_t ImmVal = Imm;
 
   // Sign-extend 19-bit immediate.
   if (ImmVal & (1 << (19 - 1)))
     ImmVal |= ~((1LL << 19) - 1);
 
   if (!Decoder->tryAddingSymbolicOperand(
-          Inst, ImmVal * 4, Addr, Inst.getOpcode() != AArch64::LDRXl, 0, 4))
+          Inst, ImmVal * 4, Addr, Inst.getOpcode() != AArch64::LDRXl, 0, 0, 4))
     Inst.addOperand(MCOperand::createImm(ImmVal));
   return Success;
 }
 
 static DecodeStatus DecodeMemExtend(MCInst &Inst, unsigned Imm,
                                     uint64_t Address,
                                     const MCDisassembler *Decoder) {
   Inst.addOperand(MCOperand::createImm((Imm  >> 1) & 1));
case AArch64::STRHui:
     break;
   case AArch64::LDRBui:
   case AArch64::STRBui:
     DecodeFPR8RegisterClass(Inst, Rt, Addr, Decoder);
     break;
   }
 
   DecodeGPR64spRegisterClass(Inst, Rn, Addr, Decoder);
-  if (!Decoder->tryAddingSymbolicOperand(Inst, offset, Addr, Fail, 0, 4))
+  if (!Decoder->tryAddingSymbolicOperand(Inst, offset, Addr, Fail, 0, 0, 4))
     Inst.addOperand(MCOperand::createImm(offset));
   return Success;
 }
 
 static DecodeStatus DecodeSignedLdStInstruction(MCInst &Inst, uint32_t insn,
                                                 uint64_t Addr,
                                                 const MCDisassembler *Decoder) {
   unsigned Rt = fieldFromInstruction(insn, 0, 5);
static DecodeStatus DecodeAdrInstruction(MCInst &Inst, uint32_t insn,
   int64_t imm = fieldFromInstruction(insn, 5, 19) << 2;
   imm |= fieldFromInstruction(insn, 29, 2);
 
   // Sign-extend the 21-bit immediate.
   if (imm & (1 << (21 - 1)))
     imm |= ~((1LL << 21) - 1);
 
   DecodeGPR64RegisterClass(Inst, Rd, Addr, Decoder);
-  if (!Decoder->tryAddingSymbolicOperand(Inst, imm, Addr, Fail, 0, 4))
+  if (!Decoder->tryAddingSymbolicOperand(Inst, imm, Addr, Fail, 0, 0, 4))
     Inst.addOperand(MCOperand::createImm(imm));
 
   return Success;
 }
 
 static DecodeStatus DecodeAddSubImmShift(MCInst &Inst, uint32_t insn,
                                          uint64_t Addr,
                                          const MCDisassembler *Decoder) {
static DecodeStatus DecodeAddSubImmShift(MCInst &Inst, uint32_t insn,
   } else {
     if (Rd == 31 && !S)
       DecodeGPR32spRegisterClass(Inst, Rd, Addr, Decoder);
     else
       DecodeGPR32RegisterClass(Inst, Rd, Addr, Decoder);
     DecodeGPR32spRegisterClass(Inst, Rn, Addr, Decoder);
   }
 
-  if (!Decoder->tryAddingSymbolicOperand(Inst, Imm, Addr, Fail, 0, 4))
+  if (!Decoder->tryAddingSymbolicOperand(Inst, Imm, Addr, Fail, 0, 0, 4))
     Inst.addOperand(MCOperand::createImm(ImmVal));
   Inst.addOperand(MCOperand::createImm(12 * ShifterVal));
   return Success;
 }
 
 static DecodeStatus DecodeUnconditionalBranch(MCInst &Inst, uint32_t insn,
                                               uint64_t Addr,
                                               const MCDisassembler *Decoder) {
   int64_t imm = fieldFromInstruction(insn, 0, 26);
 
   // Sign-extend the 26-bit immediate.
   if (imm & (1 << (26 - 1)))
     imm |= ~((1LL << 26) - 1);
 
-  if (!Decoder->tryAddingSymbolicOperand(Inst, imm * 4, Addr, true, 0, 4))
+  if (!Decoder->tryAddingSymbolicOperand(Inst, imm * 4, Addr, true, 0, 0, 4))
     Inst.addOperand(MCOperand::createImm(imm));
 
   return Success;
 }
 
 static DecodeStatus
 DecodeSystemPStateInstruction(MCInst &Inst, uint32_t insn, uint64_t Addr,
                               const MCDisassembler *Decoder) {
static DecodeStatus DecodeTestAndBranch(MCInst &Inst, uint32_t insn,
   if (dst & (1 << (14 - 1)))
     dst |= ~((1LL << 14) - 1);
 
   if (fieldFromInstruction(insn, 31, 1) == 0)
     DecodeGPR32RegisterClass(Inst, Rt, Addr, Decoder);
   else
     DecodeGPR64RegisterClass(Inst, Rt, Addr, Decoder);
   Inst.addOperand(MCOperand::createImm(bit));
-  if (!Decoder->tryAddingSymbolicOperand(Inst, dst * 4, Addr, true, 0, 4))
+  if (!Decoder->tryAddingSymbolicOperand(Inst, dst * 4, Addr, true, 0, 0, 4))
     Inst.addOperand(MCOperand::createImm(dst));
 
   return Success;
 }
 
 static DecodeStatus
 DecodeGPRSeqPairsClassRegisterClass(MCInst &Inst, unsigned RegClassID,
                                     unsigned RegNo, uint64_t Addr,

llvm/lib/Target/AArch64/Disassembler/AArch64ExternalSymbolizer.h

AArch64ExternalSymbolizer(MCContext &Ctx,
                             LLVMOpInfoCallback GetOpInfo,
                             LLVMSymbolLookupCallback SymbolLookUp,
                             void *DisInfo)
       : MCExternalSymbolizer(Ctx, std::move(RelInfo), GetOpInfo, SymbolLookUp,
                              DisInfo) {}
 
   bool tryAddingSymbolicOperand(MCInst &MI, raw_ostream &CommentStream,
                                 int64_t Value, uint64_t Address, bool IsBranch,
-                                uint64_t Offset, uint64_t InstSize) override;
+                                uint64_t Offset, uint64_t OpSize,
+                                uint64_t InstSize) override;
 };
 
 } // namespace llvm
 
 #endif

llvm/lib/Target/AArch64/Disassembler/AArch64ExternalSymbolizer.cpp

 /// that, else an MCExpr with Address + Value is created.  If GetOpInfo()
 /// returns zero and isBranch is Fail then the Opcode of the MCInst is
 /// tested and for ADRP an other instructions that help to load of pointers
 /// a symbol look up is done to see it is returns a specific reference type
 /// to add to the comment stream.  This function returns Success if it adds
 /// an operand to the MCInst and Fail otherwise.
 bool AArch64ExternalSymbolizer::tryAddingSymbolicOperand(
     MCInst &MI, raw_ostream &CommentStream, int64_t Value, uint64_t Address,
-    bool IsBranch, uint64_t Offset, uint64_t InstSize) {
+    bool IsBranch, uint64_t Offset, uint64_t OpSize, uint64_t InstSize) {
   if (!SymbolLookUp)
     return false;
   // FIXME: This method shares a lot of code with
   //        MCExternalSymbolizer::tryAddingSymbolicOperand. It may be possible
   //        refactor the MCExternalSymbolizer interface to allow more of this
   //        implementation to be shared.
   //
   struct LLVMOpInfo1 SymbolicOp;
   memset(&SymbolicOp, '\0', sizeof(struct LLVMOpInfo1));
   SymbolicOp.Value = Value;
   uint64_t ReferenceType;
   const char *ReferenceName;
-  if (!GetOpInfo ||
-      !GetOpInfo(DisInfo, Address, 0 /* Offset */, InstSize, 1, &SymbolicOp)) {
+  if (!GetOpInfo || !GetOpInfo(DisInfo, Address, /*Offset=*/0, OpSize, InstSize,
+                               1, &SymbolicOp)) {
     if (IsBranch) {
       ReferenceType = LLVMDisassembler_ReferenceType_In_Branch;
       const char *Name = SymbolLookUp(DisInfo, Address + Value, &ReferenceType,
                                       Address, &ReferenceName);
       if (Name) {
         SymbolicOp.AddSymbol.Name = Name;
         SymbolicOp.AddSymbol.Present = true;
         SymbolicOp.Value = 0;

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h

private:
   std::vector<uint64_t> ReferencedAddresses;
 
 public:
   AMDGPUSymbolizer(MCContext &Ctx, std::unique_ptr<MCRelocationInfo> &&RelInfo,
                    void *disInfo)
                    : MCSymbolizer(Ctx, std::move(RelInfo)), DisInfo(disInfo) {}
 
   bool tryAddingSymbolicOperand(MCInst &Inst, raw_ostream &cStream,
-                                int64_t Value, uint64_t Address,
-                                bool IsBranch, uint64_t Offset,
+                                int64_t Value, uint64_t Address, bool IsBranch,
+                                uint64_t Offset, uint64_t OpSize,
                                 uint64_t InstSize) override;
 
   void tryAddingPcLoadReferenceComment(raw_ostream &cStream,
                                        int64_t Value,
                                        uint64_t Address) override;
 
   ArrayRef<uint64_t> getReferencedAddresses() const override {
     return ReferencedAddresses;
   }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_AMDGPU_DISASSEMBLER_AMDGPUDISASSEMBLER_H

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp

static DecodeStatus decodeSoppBrTarget(MCInst &Inst, unsigned Imm,
                                        const MCDisassembler *Decoder) {
   auto DAsm = static_cast<const AMDGPUDisassembler*>(Decoder);
 
   // Our branches take a simm16, but we need two extra bits to account for the
   // factor of 4.
   APInt SignedOffset(18, Imm * 4, true);
   int64_t Offset = (SignedOffset.sext(64) + 4 + Addr).getSExtValue();
 
-  if (DAsm->tryAddingSymbolicOperand(Inst, Offset, Addr, true, 2, 2))
+  if (DAsm->tryAddingSymbolicOperand(Inst, Offset, Addr, true, 2, 2, 0))
     return MCDisassembler::Success;
   return addOperand(Inst, MCOperand::createImm(Imm));
 }
 
 static DecodeStatus decodeSMEMOffset(MCInst &Inst, unsigned Imm, uint64_t Addr,
                                      const MCDisassembler *Decoder) {
   auto DAsm = static_cast<const AMDGPUDisassembler*>(Decoder);
   int64_t Offset;
AMDGPUDisassembler::onSymbolStart(SymbolInfoTy &Symbol, uint64_t &Size,
   return None;
 }
 
 //===----------------------------------------------------------------------===//
 // AMDGPUSymbolizer
 //===----------------------------------------------------------------------===//
 
 // Try to find symbol name for specified label
-bool AMDGPUSymbolizer::tryAddingSymbolicOperand(MCInst &Inst,
-                                raw_ostream &/*cStream*/, int64_t Value,
-                                uint64_t /*Address*/, bool IsBranch,
-                                uint64_t /*Offset*/, uint64_t /*InstSize*/) {
+bool AMDGPUSymbolizer::tryAddingSymbolicOperand(
+    MCInst &Inst, raw_ostream & /*cStream*/, int64_t Value,
+    uint64_t /*Address*/, bool IsBranch, uint64_t /*Offset*/,
+    uint64_t /*OpSize*/, uint64_t /*InstSize*/) {
 
   if (!IsBranch) {
     return false;
   }
 
   auto *Symbols = static_cast<SectionSymbolsTy *>(DisInfo);
   if (!Symbols)
     return false;

llvm/lib/Target/ARC/Disassembler/ARCDisassembler.cpp

static DecodeStatus DecodeMEMrs9(MCInst &Inst, unsigned Insn, uint64_t Address,
   return MCDisassembler::Success;
 }
 
 static bool DecodeSymbolicOperand(MCInst &Inst, uint64_t Address,
                                   uint64_t Value,
                                   const MCDisassembler *Decoder) {
   static const uint64_t AtLeast = 2;
   return (nullptr != Decoder && Decoder->tryAddingSymbolicOperand(
-                                    Inst, Value, Address, true, 0, AtLeast));
+                                    Inst, Value, Address, true, 0, AtLeast, 0));
 }
 
 static void DecodeSymbolicOperandOff(MCInst &Inst, uint64_t Address,
                                      uint64_t Offset,
                                      const MCDisassembler *Decoder) {
   uint64_t NextAddress = Address + Offset;
 
   if (!DecodeSymbolicOperand(Inst, Address, NextAddress, Decoder))

llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp

 /// an MCExpr with Value is created.  This function returns true if it adds an
 /// operand to the MCInst and false otherwise.
 static bool tryAddingSymbolicOperand(uint64_t Address, int32_t Value,
                                      bool isBranch, uint64_t InstSize,
                                      MCInst &MI,
                                      const MCDisassembler *Decoder) {
   // FIXME: Does it make sense for value to be negative?
   return Decoder->tryAddingSymbolicOperand(MI, (uint32_t)Value, Address,
-                                           isBranch, /* Offset */ 0, InstSize);
+                                           isBranch, /*Offset=*/0, /*OpSize=*/0,
+                                           InstSize);
 }
 
 /// tryAddingPcLoadReferenceComment - trys to add a comment as to what is being
 /// referenced by a load instruction with the base register that is the Pc.
 /// These can often be values in a literal pool near the Address of the
 /// instruction.  The Address of the instruction and its immediate Value are
 /// used as a possible literal pool entry.  The SymbolLookUp call back will
 /// return the name of a symbol referenced by the literal pool's entry if

llvm/lib/Target/Hexagon/Disassembler/HexagonDisassembler.cpp

static DecodeStatus brtargetDecoder(MCInst &MI, unsigned tmp, uint64_t Address,
                                     const MCDisassembler *Decoder) {
   HexagonDisassembler const &Disassembler = disassembler(Decoder);
   unsigned Bits = HexagonMCInstrInfo::getExtentBits(*Disassembler.MCII, MI);
   // r13_2 is not extendable, so if there are no extent bits, it's r13_2
   if (Bits == 0)
     Bits = 15;
   uint64_t FullValue = fullValue(Disassembler, MI, SignExtend64(tmp, Bits));
   uint32_t Extended = FullValue + Address;
-  if (!Disassembler.tryAddingSymbolicOperand(MI, Extended, Address, true, 0, 4))
+  if (!Disassembler.tryAddingSymbolicOperand(MI, Extended, Address, true, 0, 0,
+                                             4))
     HexagonMCInstrInfo::addConstant(MI, Extended, Disassembler.getContext());
   return MCDisassembler::Success;
 }
 
 static const uint16_t SysRegDecoderTable[] = {
     Hexagon::SGP0,       Hexagon::SGP1,      Hexagon::STID,
     Hexagon::ELR,        Hexagon::BADVA0,    Hexagon::BADVA1,
     Hexagon::SSR,        Hexagon::CCR,       Hexagon::HTID,

llvm/lib/Target/Lanai/Disassembler/LanaiDisassembler.cpp

static DecodeStatus decodeSplsValue(MCInst &Inst, unsigned Insn,
   return MCDisassembler::Success;
 }
 
 static bool tryAddingSymbolicOperand(int64_t Value, bool IsBranch,
                                      uint64_t Address, uint64_t Offset,
                                      uint64_t Width, MCInst &MI,
                                      const MCDisassembler *Decoder) {
   return Decoder->tryAddingSymbolicOperand(MI, Value, Address, IsBranch, Offset,
-                                           Width);
+                                           Width, /*InstSize=*/0);
 }
 
 static DecodeStatus decodeBranch(MCInst &MI, unsigned Insn, uint64_t Address,
                                  const MCDisassembler *Decoder) {
   if (!tryAddingSymbolicOperand(Insn + Address, false, Address, 2, 23, MI,
                                 Decoder))
     MI.addOperand(MCOperand::createImm(Insn));
   return MCDisassembler::Success;

llvm/lib/Target/Sparc/Disassembler/SparcDisassembler.cpp

return DecodeMem(Inst, insn, Address, Decoder, false,
                    DecodeCPPairRegisterClass);
 }
 
 static bool tryAddingSymbolicOperand(int64_t Value, bool isBranch,
                                      uint64_t Address, uint64_t Offset,
                                      uint64_t Width, MCInst &MI,
                                      const MCDisassembler *Decoder) {
   return Decoder->tryAddingSymbolicOperand(MI, Value, Address, isBranch, Offset,
-                                           Width);
+                                           Width, /*InstSize=*/4);
 }
 
 static DecodeStatus DecodeCall(MCInst &MI, unsigned insn, uint64_t Address,
                                const MCDisassembler *Decoder) {
   unsigned tgt = fieldFromInstruction(insn, 0, 30);
   tgt <<= 2;
   if (!tryAddingSymbolicOperand(tgt+Address, false, Address,
                                 0, 30, MI, Decoder))

llvm/lib/Target/SystemZ/Disassembler/SystemZDisassembler.cpp

 /// 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,
                                      const MCDisassembler *Decoder) {
   return Decoder->tryAddingSymbolicOperand(MI, Value, Address, isBranch, Offset,
-                                           Width);
+                                           Width, /*InstSize=*/0);
 }
 
 static DecodeStatus decodeRegisterClass(MCInst &Inst, uint64_t RegNo,
                                         const unsigned *Regs, unsigned Size) {
   assert(RegNo < Size && "Invalid register");
   RegNo = Regs[RegNo];
   if (RegNo == 0)
     return MCDisassembler::Fail;

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

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 (!Dis->tryAddingSymbolicOperand(mcInst, immediate + pcrel,
-                                     insn.startLocation, isBranch,
-                                     insn.immediateOffset, insn.immediateSize))
+  if (!Dis->tryAddingSymbolicOperand(
+          mcInst, immediate + pcrel, insn.startLocation, isBranch,
+          insn.immediateOffset, insn.immediateSize, insn.length))
     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;
         Dis->tryAddingPcLoadReferenceComment(insn.displacement + pcrel,
                                              insn.startLocation +
                                                  insn.displacementOffset);
         // 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);
+
+  const uint8_t dispSize =
+      (insn.eaDisplacement == EA_DISP_NONE) ? 0 : insn.displacementSize;
+
   if (!Dis->tryAddingSymbolicOperand(
           mcInst, insn.displacement + pcrel, insn.startLocation, false,
-          insn.displacementOffset, insn.displacementSize))
+          insn.displacementOffset, dispSize, insn.length))
     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/tools/llvm-objdump/MachODump.cpp

 // at the Pc parameter.  The immediate value the operand has is passed in
 // op_info->Value and is at Offset past the start of the instruction and has a
 // byte Size of 1, 2 or 4. The symbolc information is returned in TagBuf is the
 // LLVMOpInfo1 struct defined in the header "llvm-c/Disassembler.h" as symbol
 // names and addends of the symbolic expression to add for the operand.  The
 // value of TagType is currently 1 (for the LLVMOpInfo1 struct). If symbolic
 // information is returned then this function returns 1 else it returns 0.
 static int SymbolizerGetOpInfo(void *DisInfo, uint64_t Pc, uint64_t Offset,
-                               uint64_t Size, int TagType, void *TagBuf) {
+                               uint64_t OpSize, uint64_t InstSize, int TagType,
+                               void *TagBuf) {
   struct DisassembleInfo *info = (struct DisassembleInfo *)DisInfo;
   struct LLVMOpInfo1 *op_info = (struct LLVMOpInfo1 *)TagBuf;
   uint64_t value = op_info->Value;
 
   // Make sure all fields returned are zero if we don't set them.
   memset((void *)op_info, '\0', sizeof(struct LLVMOpInfo1));
   op_info->Value = value;
 
   // If the TagType is not the value 1 which it code knows about or if no
   // verbose symbolic information is wanted then just return 0, indicating no
   // information is being returned.
   if (TagType != 1 || !info->verbose)
     return 0;
 
   unsigned int Arch = info->O->getArch();
   if (Arch == Triple::x86) {
-    if (Size != 1 && Size != 2 && Size != 4 && Size != 0)
+    if (OpSize != 1 && OpSize != 2 && OpSize != 4 && OpSize != 0)
       return 0;
     if (info->O->getHeader().filetype != MachO::MH_OBJECT) {
       // TODO:
       // Search the external relocation entries of a fully linked image
       // (if any) for an entry that matches this segment offset.
       // uint32_t seg_offset = (Pc + Offset);
       return 0;
     }
if (reloc_found && (r_type == MachO::GENERIC_RELOC_SECTDIFF ||
       else
         op_info->SubtractSymbol.Value = pair_r_value;
       op_info->Value = offset;
       return 1;
     }
     return 0;
   }
   if (Arch == Triple::x86_64) {
-    if (Size != 1 && Size != 2 && Size != 4 && Size != 0)
+    if (OpSize != 1 && OpSize != 2 && OpSize != 4 && OpSize != 0)
       return 0;
     // For non MH_OBJECT types, like MH_KEXT_BUNDLE, Search the external
     // relocation entries of a linked image (if any) for an entry that matches
     // this segment offset.
     if (info->O->getHeader().filetype != MachO::MH_OBJECT) {
       uint64_t seg_offset = Pc + Offset;
       bool reloc_found = false;
       DataRefImpl Rel;
if (info->O->getHeader().filetype != MachO::MH_OBJECT) {
           break;
         }
       }
       if (reloc_found && isExtern) {
         // The Value passed in will be adjusted by the Pc if the instruction
         // adds the Pc.  But for x86_64 external relocation entries the Value
         // is the offset from the external symbol.
         if (info->O->getAnyRelocationPCRel(RE))
-          op_info->Value -= Pc + Offset + Size;
+          op_info->Value -= Pc + InstSize;
         const char *name =
             unwrapOrError(Symbol.getName(), info->O->getFileName()).data();
         op_info->AddSymbol.Present = 1;
         op_info->AddSymbol.Name = name;
         return 1;
       }
       return 0;
     }
for (const RelocationRef &Reloc : info->S.relocations()) {
         break;
       }
     }
     if (reloc_found && isExtern) {
       // The Value passed in will be adjusted by the Pc if the instruction
       // adds the Pc.  But for x86_64 external relocation entries the Value
       // is the offset from the external symbol.
       if (info->O->getAnyRelocationPCRel(RE))
-        op_info->Value -= Pc + Offset + Size;
+        op_info->Value -= Pc + InstSize;
       const char *name =
           unwrapOrError(Symbol.getName(), info->O->getFileName()).data();
       unsigned Type = info->O->getAnyRelocationType(RE);
       if (Type == MachO::X86_64_RELOC_SUBTRACTOR) {
         DataRefImpl RelNext = Rel;
         info->O->moveRelocationNext(RelNext);
         MachO::any_relocation_info RENext = info->O->getRelocation(RelNext);
         unsigned TypeNext = info->O->getAnyRelocationType(RENext);
if (reloc_found && isExtern) {
       // like: X86_64_RELOC_TLV, X86_64_RELOC_GOT_LOAD and X86_64_RELOC_GOT.
       op_info->AddSymbol.Present = 1;
       op_info->AddSymbol.Name = name;
       return 1;
     }
     return 0;
   }
   if (Arch == Triple::arm) {
-    if (Offset != 0 || (Size != 4 && Size != 2))
+    if (Offset != 0 || (InstSize != 4 && InstSize != 2))
       return 0;
     if (info->O->getHeader().filetype != MachO::MH_OBJECT) {
       // TODO:
       // Search the external relocation entries of a fully linked image
       // (if any) for an entry that matches this segment offset.
       // uint32_t seg_offset = (Pc + Offset);
       return 0;
     }
if (Arch == Triple::arm) {
     if (add != nullptr) {
       op_info->AddSymbol.Name = add;
       return 1;
     }
     op_info->AddSymbol.Value = value;
     return 1;
   }
   if (Arch == Triple::aarch64) {
-    if (Offset != 0 || Size != 4)
+    if (Offset != 0 || InstSize != 4)
       return 0;
     if (info->O->getHeader().filetype != MachO::MH_OBJECT) {
       // TODO:
       // Search the external relocation entries of a fully linked image
       // (if any) for an entry that matches this segment offset.
       // uint64_t seg_offset = (Pc + Offset);
       return 0;
     }

llvm/unittests/MC/X86/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS
+  MC
+  MCDisassembler
+  Target
+  X86Desc
+  X86Disassembler
+  X86Info
+  )
+
+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-unknown-elf";
+  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 MCContext &() { return *Ctx; };
+};
+
+Context &getContext() {
+  static Context Ctxt;
+  return Ctxt;
+}
+
+class X86MCSymbolizerTest : public MCSymbolizer {
+public:
+  X86MCSymbolizerTest(MCContext &MC) : MCSymbolizer(MC, nullptr) {}
+  ~X86MCSymbolizerTest() {}
+
+  struct OpInfo {
+    int64_t Value = 0;
+    uint64_t Offset = 0;
+    uint64_t Size;
+  };
+  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 OpSize,
+                                uint64_t InstSize) override {
+    Operands.push_back({Value, Offset, OpSize});
+    InstructionSize = InstSize;
+    return false;
+  }
+
+  void tryAddingPcLoadReferenceComment(raw_ostream &cStream, int64_t Value,
+                                       uint64_t Address) override {}
+};
+
+} // namespace
+
+TEST(X86Disassembler, X86MCSymbolizerTest) {
+  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,
+                          uint64_t Size) {
+    ASSERT_TRUE(TestSymbolizer->Operands.size() > OpNo);
+    EXPECT_EQ(TestSymbolizer->Operands[OpNo].Value, Value);
+    EXPECT_EQ(TestSymbolizer->Operands[OpNo].Offset, Offset);
+    EXPECT_EQ(TestSymbolizer->Operands[OpNo].Size, Size);
+  };
+
+  // movq    $0x80000, 0x80000
+  checkBytes(
+      {0x48, 0xc7, 0x04, 0x25, 0x00, 0x00, 0x08, 0x00, 0x00, 0x00, 0x08, 0x00});
+  checkOperand(0, 0x80000, 4, 4);
+  checkOperand(1, 0x80000, 8, 4);
+
+  // movq   $0x2a, 0x123(%rax,%r14,8)
+  checkBytes(
+      {0x4a, 0xc7, 0x84, 0xf0, 0x23, 0x01, 0x00, 0x00, 0x2a, 0x00, 0x00, 0x00});
+  checkOperand(0, 291, 4, 4);
+  checkOperand(1, 42, 8, 4);
+
+  // 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, 4);
+  checkOperand(1, 0xffffffffffffefe8, 7, 4);
+
+  // movq   $0xfffffffffffffef5, (%r12)
+  // Test that the displacement operand has a size of 0, since it is not
+  // explicitly specified in the instruction.
+  checkBytes({0x49, 0xc7, 0x04, 0x24, 0xf5, 0xfe, 0xff, 0xff});
+  checkOperand(0, 0, 4, 0);
+  checkOperand(1, 0xfffffffffffffef5, 4, 4);
+}