[llvm] r222965 - Make MultiClass::DefPrototypes own their Records to fix memory leaks.

[David Blaikie]

On Wed, Dec 3, 2014 at 1:20 PM, Craig Topper <craig.topper at gmail.com> wrote:

>
>
> On Wed, Dec 3, 2014 at 1:04 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Sat, Nov 29, 2014 at 4:19 PM, Craig Topper <craig.topper at gmail.com>
>> wrote:
>>
>>> Author: ctopper
>>> Date: Sat Nov 29 18:19:28 2014
>>> New Revision: 222965
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=222965&view=rev
>>> Log:
>>> Make MultiClass::DefPrototypes own their Records to fix memory leaks.
>>>
>>> Modified:
>>>     llvm/trunk/include/llvm/TableGen/Record.h
>>>     llvm/trunk/lib/TableGen/TGParser.cpp
>>>
>>> Modified: llvm/trunk/include/llvm/TableGen/Record.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/TableGen/Record.h?rev=222965&r1=222964&r2=222965&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/TableGen/Record.h (original)
>>> +++ llvm/trunk/include/llvm/TableGen/Record.h Sat Nov 29 18:19:28 2014
>>> @@ -1663,7 +1663,7 @@ raw_ostream &operator<<(raw_ostream &OS,
>>>
>>>  struct MultiClass {
>>>    Record Rec;  // Placeholder for template args and Name.
>>> -  typedef std::vector<Record*> RecordVector;
>>> +  typedef std::vector<std::unique_ptr<Record>> RecordVector;
>>>
>>
>> Any chance of deque<Record> (or forward_list<Record> or list<Record>)
>> here?
>>
>
> We don't always know at the pointer of record creation whether the record
> will go in this list or to addDef. And the Record copy constructor alters
> the record id. So it seemed weird to make a copy when we figure out where
> it goes.
>

Perhaps the move constructor could not alter the record id? (having the
copy ctor do that is pretty dodgy & we should perhaps add a named ctor for
that special operation, but that's somewhat orthogonal) Then we could just
move the Record into its final destination?


>
>
>>
>>
>>>    RecordVector DefPrototypes;
>>>
>>>    void dump() const;
>>>
>>> Modified: llvm/trunk/lib/TableGen/TGParser.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/TableGen/TGParser.cpp?rev=222965&r1=222964&r2=222965&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/lib/TableGen/TGParser.cpp (original)
>>> +++ llvm/trunk/lib/TableGen/TGParser.cpp Sat Nov 29 18:19:28 2014
>>> @@ -239,7 +239,7 @@ bool TGParser::AddSubMultiClass(MultiCla
>>>        if (AddValue(NewDef.get(), SubMultiClass.RefRange.Start,
>>> MCVals[i]))
>>>          return true;
>>>
>>> -    CurMC->DefPrototypes.push_back(NewDef.release());
>>> +    CurMC->DefPrototypes.push_back(std::move(NewDef));
>>>    }
>>>
>>>    const std::vector<Init *> &SMCTArgs = SMC->Rec.getTemplateArgs();
>>> @@ -274,7 +274,7 @@ bool TGParser::AddSubMultiClass(MultiCla
>>>               jend = CurMC->DefPrototypes.end();
>>>             j != jend;
>>>             ++j) {
>>> -        Record *Def = *j;
>>> +        Record *Def = j->get();
>>>
>>>          if (SetValue(Def, SubMultiClass.RefRange.Start, SMCTArgs[i],
>>>                       std::vector<unsigned>(),
>>> @@ -1258,7 +1258,7 @@ Init *TGParser::ParseSimpleValue(Record
>>>        // known before any use.
>>>        NewRec->setResolveFirst(true);
>>>        // Otherwise, we're inside a multiclass, add it to the multiclass.
>>> -      CurMultiClass->DefPrototypes.push_back(NewRecOwner.release());
>>> +      CurMultiClass->DefPrototypes.push_back(std::move(NewRecOwner));
>>>
>>>        // Copy the template arguments for the multiclass into the def.
>>>        const std::vector<Init *> &TArgs =
>>> @@ -2063,7 +2063,7 @@ bool TGParser::ParseDef(MultiClass *CurM
>>>            == CurRec->getNameInit())
>>>          return Error(DefLoc, "def '" + CurRec->getNameInitAsString() +
>>>                       "' already defined in this multiclass!");
>>> -    CurMultiClass->DefPrototypes.push_back(CurRecOwner.release());
>>> +    CurMultiClass->DefPrototypes.push_back(std::move(CurRecOwner));
>>>    } else if (ParseObjectBody(CurRec)) {
>>>      return true;
>>>    }
>>> @@ -2507,7 +2507,7 @@ bool TGParser::ResolveMulticlassDef(Mult
>>>          == CurRec->getNameInit())
>>>        return Error(DefmPrefixLoc, "defm '" +
>>> CurRec->getNameInitAsString() +
>>>                     "' already defined in this multiclass!");
>>> -  CurMultiClass->DefPrototypes.push_back(CurRec);
>>> +
>>> CurMultiClass->DefPrototypes.push_back(std::unique_ptr<Record>(CurRec));
>>>
>>>    // Copy the template arguments for the multiclass into the new def.
>>>    const std::vector<Init *> &TA =
>>> @@ -2570,7 +2570,7 @@ bool TGParser::ParseDefm(MultiClass *Cur
>>>
>>>      // Loop over all the def's in the multiclass, instantiating each
>>> one.
>>>      for (unsigned i = 0, e = MC->DefPrototypes.size(); i != e; ++i) {
>>> -      Record *DefProto = MC->DefPrototypes[i];
>>> +      Record *DefProto = MC->DefPrototypes[i].get();
>>>
>>>        Record *CurRec = InstantiateMulticlassDef(*MC, DefProto,
>>> DefmPrefix,
>>>                                                  SMRange(DefmLoc,
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>
>>
>
>
> --
> ~Craig
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141203/f89552a8/attachment.html>