[MC] Fix memory leak when allocating MCInst with bump allocator

[hgreving]

Summary

Adds the function createMCInst() to MCContext that creates a MCInst using
a typed bump alloctor.

MCInst contains a SmallVector<MCOperand, 8>. The SmallVector is POD only
for <= 8 operands. The default untyped bump pointer allocator of MCContext
does not delete the MCInst, so if the SmallVector grows, it's a leak.

This fixes https://bugs.llvm.org/show_bug.cgi?id=46900.

[hgreving]

I have added some people w/o knowing them based on contributions to Hexagon and MC. Please accept or reassign the review if possible.

[kariddi]

llvm/lib/MC/MCContext.cpp

135

There is usually an extra space between the new and the "(" when a placement-like new is used in LLVM.

[MaskRay]

Add createMCInst() using a typed MCInst bump allocator.

For titles most patches don't add a full stop. The important fact (it fixes a memory leak) should be mentioned.

This fixes https://bugs.llvm.org/show_bug.cgi?id=46900.

Add a little detail beside the bug reference about the memory leak.

llvm/include/llvm/MC/MCContext.h

384

I think @{ is used very rarely for new code. You may delete it which does not add any documentation value.

[hgreving]

Done.

[MaskRay]

LGTM.

[majnemer]

LGTM

[hgreving]

Tending to push this rel. small fix w/o an explicit Hexagon sign-off. Objections?

[bcain]

In D84896#2188558, @hgreving wrote:

Tending to push this rel. small fix w/o an explicit Hexagon sign-off. Objections?

Sorry for not giving this attention when it was merged -- LGTM.

llvm/include/llvm/MC/MCContext.h

private:
     /// objects.
     BumpPtrAllocator Allocator;
 
     SpecificBumpPtrAllocator<MCSectionCOFF> COFFAllocator;
     SpecificBumpPtrAllocator<MCSectionELF> ELFAllocator;
     SpecificBumpPtrAllocator<MCSectionMachO> MachOAllocator;
     SpecificBumpPtrAllocator<MCSectionWasm> WasmAllocator;
     SpecificBumpPtrAllocator<MCSectionXCOFF> XCOFFAllocator;
+    SpecificBumpPtrAllocator<MCInst> MCInstAllocator;
 
     /// Bindings of names to symbols.
     SymbolTable Symbols;
 
     /// A mapping from a local label number and an instance count to a symbol.
     /// For example, in the assembly
     ///     1:
     ///     2:
public:
     /// @{
 
     /// reset - return object to right after construction state to prepare
     /// to process a new module
     void reset();
 
     /// @}
 
+    /// \name McInst Management
+    /// @{
+
+    /// Create and return a new MC instruction.
+    MCInst *createMCInst();
+
+    /// @}
+
     /// \name Symbol Management
     /// @{
 
     /// Create and return a new linker temporary symbol with a unique but
     /// unspecified name.
     MCSymbol *createLinkerPrivateTempSymbol();
 
     /// Create and return a new assembler temporary symbol with a unique but

llvm/lib/MC/MCContext.cpp

 //===----------------------------------------------------------------------===//
 
 void MCContext::reset() {
   // Call the destructors so the fragments are freed
   COFFAllocator.DestroyAll();
   ELFAllocator.DestroyAll();
   MachOAllocator.DestroyAll();
   XCOFFAllocator.DestroyAll();
+  MCInstAllocator.DestroyAll();
 
   MCSubtargetAllocator.DestroyAll();
   InlineAsmUsedLabelNames.clear();
   UsedNames.clear();
   Symbols.clear();
   Allocator.Reset();
   Instances.clear();
   CompilationDir.clear();
void MCContext::reset() {
   DwarfLocSeen = false;
   GenDwarfForAssembly = false;
   GenDwarfFileNumber = 0;
 
   HadError = false;
 }
 
 //===----------------------------------------------------------------------===//
+// MCInst Management
+//===----------------------------------------------------------------------===//
+
+MCInst *MCContext::createMCInst() {
+  return new(MCInstAllocator.Allocate()) MCInst;
+}
+
+//===----------------------------------------------------------------------===//
 // Symbol Manipulation
 //===----------------------------------------------------------------------===//
 
 MCSymbol *MCContext::getOrCreateSymbol(const Twine &Name) {
   SmallString<128> NameSV;
   StringRef NameRef = Name.toStringRef(NameSV);
 
   assert(!NameRef.empty() && "Normal symbols cannot be unnamed!");

llvm/lib/Target/Hexagon/AsmParser/HexagonAsmParser.cpp

if (!InBrackets) {
       getParser().Error(IDLoc, "Not in a packet");
       return true;
     }
     InBrackets = false;
     if (matchBundleOptions())
       return true;
     return finishBundle(IDLoc, Out);
   }
-  MCInst *SubInst = new (getParser().getContext()) MCInst;
+  MCInst *SubInst = getParser().getContext().createMCInst();
   if (matchOneInstruction(*SubInst, IDLoc, Operands, ErrorInfo,
                           MatchingInlineAsm)) {
     if (InBrackets)
       eatToEndOfPacket();
     return true;
   }
   HexagonMCInstrInfo::extendIfNeeded(
       getParser().getContext(), MII, MCB, *SubInst);

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

DecodeStatus HexagonDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
   Size = 0;
 
   *CurrentBundle = &MI;
   MI.setOpcode(Hexagon::BUNDLE);
   MI.addOperand(MCOperand::createImm(0));
   while (Result == Success && !Complete) {
     if (Bytes.size() < HEXAGON_INSTR_SIZE)
       return MCDisassembler::Fail;
-    MCInst *Inst = new (getContext()) MCInst;
+    MCInst *Inst = getContext().createMCInst();
     Result = getSingleInstruction(*Inst, MI, Bytes, Address, cs, Complete);
     MI.addOperand(MCOperand::createInst(Inst));
     Size += HEXAGON_INSTR_SIZE;
     Bytes = Bytes.slice(HEXAGON_INSTR_SIZE);
   }
   if (Result == MCDisassembler::Fail)
     return Result;
   if (Size > HEXAGON_MAX_PACKET_SIZE)
case 13:
       DecodeHigh = DecoderTableSUBINSN_L232;
       break;
     case 14:
       DecodeLow = DecoderTableSUBINSN_S232;
       DecodeHigh = DecoderTableSUBINSN_S232;
       break;
     }
     MI.setOpcode(Hexagon::DuplexIClass0 + duplexIClass);
-    MCInst *MILow = new (getContext()) MCInst;
-    MCInst *MIHigh = new (getContext()) MCInst;
+    MCInst *MILow = getContext().createMCInst();
+    MCInst *MIHigh = getContext().createMCInst();
     auto TmpExtender = CurrentExtender;
     CurrentExtender =
         nullptr; // constant extenders in duplex must always be in slot 1
     Result = decodeInstruction(DecodeLow, *MILow, Instruction & 0x1fff, Address,
                                this, STI);
     CurrentExtender = TmpExtender;
     if (Result != DecodeStatus::Success)
       return DecodeStatus::Fail;

llvm/lib/Target/Hexagon/HexagonMCInstLower.cpp

void llvm::HexagonLowerToMC(const MCInstrInfo &MCII, const MachineInstr *MI,
   if (MI->getOpcode() == Hexagon::ENDLOOP0) {
     HexagonMCInstrInfo::setInnerLoop(MCB);
     return;
   }
   if (MI->getOpcode() == Hexagon::ENDLOOP1) {
     HexagonMCInstrInfo::setOuterLoop(MCB);
     return;
   }
-  MCInst *MCI = new (AP.OutContext) MCInst;
+  MCInst *MCI = AP.OutContext.createMCInst();
   MCI->setOpcode(MI->getOpcode());
   assert(MCI->getOpcode() == static_cast<unsigned>(MI->getOpcode()) &&
          "MCI opcode should have been set on construction");
 
   for (unsigned i = 0, e = MI->getNumOperands(); i < e; i++) {
     const MachineOperand &MO = MI->getOperand(i);
     MCOperand MCO;
     bool MustExtend = MO.getTargetFlags() & HexagonII::HMOTF_ConstExtended;

llvm/lib/Target/Hexagon/MCTargetDesc/HexagonAsmBackend.cpp

public:
 
   std::unique_ptr<MCObjectTargetWriter>
   createObjectTargetWriter() const override {
     return createHexagonELFObjectWriter(OSABI, CPU);
   }
 
   void setExtender(MCContext &Context) const {
     if (Extender == nullptr)
-      const_cast<HexagonAsmBackend *>(this)->Extender = new (Context) MCInst;
+      const_cast<HexagonAsmBackend *>(this)->Extender = Context.createMCInst();
   }
 
   MCInst *takeExtender() const {
     assert(Extender != nullptr);
     MCInst * Result = Extender;
     const_cast<HexagonAsmBackend *>(this)->Extender = nullptr;
     return Result;
   }
for (auto I : Layout.getSectionOrder()) {
               break;
             }
             case MCFragment::FT_Relaxable: {
               MCContext &Context = Asm.getContext();
               auto &RF = cast<MCRelaxableFragment>(*K);
               auto &Inst = const_cast<MCInst &>(RF.getInst());
               while (Size > 0 &&
                      HexagonMCInstrInfo::bundleSize(Inst) < MaxPacketSize) {
-                MCInst *Nop = new (Context) MCInst;
+                MCInst *Nop = Context.createMCInst();
                 Nop->setOpcode(Hexagon::A2_nop);
                 Inst.addOperand(MCOperand::createInst(Nop));
                 Size -= 4;
                 if (!HexagonMCChecker(
                          Context, *MCII, *RF.getSubtargetInfo(), Inst,
                          *Context.getRegisterInfo(), false)
                          .check()) {
                   Inst.erase(Inst.end() - 1);

llvm/lib/Target/Hexagon/MCTargetDesc/HexagonMCCompound.cpp

static MCInst *getCompoundInsn(MCContext &Context, MCInst const &L,
   switch (L.getOpcode()) {
   default:
     LLVM_DEBUG(dbgs() << "Possible compound ignored\n");
     return CompoundInsn;
 
   case Hexagon::A2_tfrsi:
     Rt = L.getOperand(0);
     compoundOpcode = J4_jumpseti;
-    CompoundInsn = new (Context) MCInst;
+    CompoundInsn = Context.createMCInst();
     CompoundInsn->setOpcode(compoundOpcode);
 
     CompoundInsn->addOperand(Rt);
     CompoundInsn->addOperand(L.getOperand(1)); // Immediate
     CompoundInsn->addOperand(R.getOperand(0)); // Jump target
     break;
 
   case Hexagon::A2_tfr:
     Rt = L.getOperand(0);
     Rs = L.getOperand(1);
 
     compoundOpcode = J4_jumpsetr;
-    CompoundInsn = new (Context) MCInst;
+    CompoundInsn = Context.createMCInst();
     CompoundInsn->setOpcode(compoundOpcode);
     CompoundInsn->addOperand(Rt);
     CompoundInsn->addOperand(Rs);
     CompoundInsn->addOperand(R.getOperand(0)); // Jump target.
 
     break;
 
   case Hexagon::C2_cmpeq:
     LLVM_DEBUG(dbgs() << "CX: C2_cmpeq\n");
     Rs = L.getOperand(1);
     Rt = L.getOperand(2);
 
     compoundOpcode = cmpeqBitOpcode[getCompoundOp(R)];
-    CompoundInsn = new (Context) MCInst;
+    CompoundInsn = Context.createMCInst();
     CompoundInsn->setOpcode(compoundOpcode);
     CompoundInsn->addOperand(Rs);
     CompoundInsn->addOperand(Rt);
     CompoundInsn->addOperand(R.getOperand(1));
     break;
 
   case Hexagon::C2_cmpgt:
     LLVM_DEBUG(dbgs() << "CX: C2_cmpgt\n");
     Rs = L.getOperand(1);
     Rt = L.getOperand(2);
 
     compoundOpcode = cmpgtBitOpcode[getCompoundOp(R)];
-    CompoundInsn = new (Context) MCInst;
+    CompoundInsn = Context.createMCInst();
     CompoundInsn->setOpcode(compoundOpcode);
     CompoundInsn->addOperand(Rs);
     CompoundInsn->addOperand(Rt);
     CompoundInsn->addOperand(R.getOperand(1));
     break;
 
   case Hexagon::C2_cmpgtu:
     LLVM_DEBUG(dbgs() << "CX: C2_cmpgtu\n");
     Rs = L.getOperand(1);
     Rt = L.getOperand(2);
 
     compoundOpcode = cmpgtuBitOpcode[getCompoundOp(R)];
-    CompoundInsn = new (Context) MCInst;
+    CompoundInsn = Context.createMCInst();
     CompoundInsn->setOpcode(compoundOpcode);
     CompoundInsn->addOperand(Rs);
     CompoundInsn->addOperand(Rt);
     CompoundInsn->addOperand(R.getOperand(1));
     break;
 
   case Hexagon::C2_cmpeqi:
     LLVM_DEBUG(dbgs() << "CX: C2_cmpeqi\n");
     Success = L.getOperand(2).getExpr()->evaluateAsAbsolute(Value);
     (void)Success;
     assert(Success);
     if (Value == -1)
       compoundOpcode = cmpeqn1BitOpcode[getCompoundOp(R)];
     else
       compoundOpcode = cmpeqiBitOpcode[getCompoundOp(R)];
 
     Rs = L.getOperand(1);
-    CompoundInsn = new (Context) MCInst;
+    CompoundInsn = Context.createMCInst();
     CompoundInsn->setOpcode(compoundOpcode);
     CompoundInsn->addOperand(Rs);
     CompoundInsn->addOperand(L.getOperand(2));
     CompoundInsn->addOperand(R.getOperand(1));
     break;
 
   case Hexagon::C2_cmpgti:
     LLVM_DEBUG(dbgs() << "CX: C2_cmpgti\n");
     Success = L.getOperand(2).getExpr()->evaluateAsAbsolute(Value);
     (void)Success;
     assert(Success);
     if (Value == -1)
       compoundOpcode = cmpgtn1BitOpcode[getCompoundOp(R)];
     else
       compoundOpcode = cmpgtiBitOpcode[getCompoundOp(R)];
 
     Rs = L.getOperand(1);
-    CompoundInsn = new (Context) MCInst;
+    CompoundInsn = Context.createMCInst();
     CompoundInsn->setOpcode(compoundOpcode);
     CompoundInsn->addOperand(Rs);
     CompoundInsn->addOperand(L.getOperand(2));
     CompoundInsn->addOperand(R.getOperand(1));
     break;
 
   case Hexagon::C2_cmpgtui:
     LLVM_DEBUG(dbgs() << "CX: C2_cmpgtui\n");
     Rs = L.getOperand(1);
     compoundOpcode = cmpgtuiBitOpcode[getCompoundOp(R)];
-    CompoundInsn = new (Context) MCInst;
+    CompoundInsn = Context.createMCInst();
     CompoundInsn->setOpcode(compoundOpcode);
     CompoundInsn->addOperand(Rs);
     CompoundInsn->addOperand(L.getOperand(2));
     CompoundInsn->addOperand(R.getOperand(1));
     break;
 
   case Hexagon::S2_tstbit_i:
     LLVM_DEBUG(dbgs() << "CX: S2_tstbit_i\n");
     Rs = L.getOperand(1);
     compoundOpcode = tstBitOpcode[getCompoundOp(R)];
-    CompoundInsn = new (Context) MCInst;
+    CompoundInsn = Context.createMCInst();
     CompoundInsn->setOpcode(compoundOpcode);
     CompoundInsn->addOperand(Rs);
     CompoundInsn->addOperand(R.getOperand(1));
     break;
   }
 
   return CompoundInsn;
 }