[llvm] r303576 - Make TypeSerializer's StringMap use the same allocator.

Richard Trieu via llvm-commits llvm-commits at lists.llvm.org
Mon May 22 20:53:16 PDT 2017


Zach,

I think this patch is causing a memory leak.  Removing the function
TypeSerializer::insertRecordBytesWithCopy and not making the copy will
cause the memory backing the array to be deleted before it is used.  This
has been detected by the sanitizer bot.

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/5121

Richard

On Mon, May 22, 2017 at 2:07 PM, Zachary Turner via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> Author: zturner
> Date: Mon May 22 16:07:14 2017
> New Revision: 303576
>
> URL: http://llvm.org/viewvc/llvm-project?rev=303576&view=rev
> Log:
> Make TypeSerializer's StringMap use the same allocator.
>
> Modified:
>     llvm/trunk/include/llvm/DebugInfo/CodeView/TypeSerializer.h
>     llvm/trunk/include/llvm/DebugInfo/CodeView/TypeTableBuilder.h
>     llvm/trunk/include/llvm/DebugInfo/CodeView/TypeTableCollection.h
>     llvm/trunk/lib/DebugInfo/CodeView/TypeSerializer.cpp
>     llvm/trunk/lib/DebugInfo/CodeView/TypeTableCollection.cpp
>     llvm/trunk/tools/llvm-pdbdump/llvm-pdbdump.cpp
>
> Modified: llvm/trunk/include/llvm/DebugInfo/CodeView/TypeSerializer.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/
> llvm/DebugInfo/CodeView/TypeSerializer.h?rev=303576&
> r1=303575&r2=303576&view=diff
> ============================================================
> ==================
> --- llvm/trunk/include/llvm/DebugInfo/CodeView/TypeSerializer.h (original)
> +++ llvm/trunk/include/llvm/DebugInfo/CodeView/TypeSerializer.h Mon May
> 22 16:07:14 2017
> @@ -45,12 +45,13 @@ class TypeSerializer : public TypeVisito
>      }
>    };
>
> -  typedef SmallVector<MutableArrayRef<uint8_t>, 2> RecordList;
> +  typedef SmallVector<MutableArrayRef<uint8_t>, 2> MutableRecordList;
> +  typedef SmallVector<ArrayRef<uint8_t>, 2> RecordList;
>
>    static constexpr uint8_t ContinuationLength = 8;
>    BumpPtrAllocator &RecordStorage;
>    RecordSegment CurrentSegment;
> -  RecordList FieldListSegments;
> +  MutableRecordList FieldListSegments;
>
>    TypeIndex LastTypeIndex;
>    Optional<TypeLeafKind> TypeKind;
> @@ -61,7 +62,7 @@ class TypeSerializer : public TypeVisito
>    TypeRecordMapping Mapping;
>
>    RecordList SeenRecords;
> -  StringMap<TypeIndex> HashedRecords;
> +  StringMap<TypeIndex, BumpPtrAllocator&> HashedRecords;
>
>    bool isInFieldList() const;
>    TypeIndex calcNextTypeIndex() const;
> @@ -69,9 +70,7 @@ class TypeSerializer : public TypeVisito
>    MutableArrayRef<uint8_t> getCurrentSubRecordData();
>    MutableArrayRef<uint8_t> getCurrentRecordData();
>    Error writeRecordPrefix(TypeLeafKind Kind);
> -  TypeIndex insertRecordBytesPrivate(MutableArrayRef<uint8_t> Record);
> -  TypeIndex insertRecordBytesWithCopy(CVType &Record,
> -                                      MutableArrayRef<uint8_t> Data);
> +  TypeIndex insertRecordBytesPrivate(ArrayRef<uint8_t> &Record);
>
>    Expected<MutableArrayRef<uint8_t>>
>    addPadding(MutableArrayRef<uint8_t> Record);
> @@ -79,9 +78,9 @@ class TypeSerializer : public TypeVisito
>  public:
>    explicit TypeSerializer(BumpPtrAllocator &Storage);
>
> -  ArrayRef<MutableArrayRef<uint8_t>> records() const;
> +  ArrayRef<ArrayRef<uint8_t>> records() const;
>    TypeIndex getLastTypeIndex() const;
> -  TypeIndex insertRecordBytes(MutableArrayRef<uint8_t> Record);
> +  TypeIndex insertRecordBytes(ArrayRef<uint8_t> Record);
>    Expected<TypeIndex> visitTypeEndGetIndex(CVType &Record);
>
>    Error visitTypeBegin(CVType &Record) override;
>
> Modified: llvm/trunk/include/llvm/DebugInfo/CodeView/TypeTableBuilder.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/
> llvm/DebugInfo/CodeView/TypeTableBuilder.h?rev=303576&
> r1=303575&r2=303576&view=diff
> ============================================================
> ==================
> --- llvm/trunk/include/llvm/DebugInfo/CodeView/TypeTableBuilder.h
> (original)
> +++ llvm/trunk/include/llvm/DebugInfo/CodeView/TypeTableBuilder.h Mon May
> 22 16:07:14 2017
> @@ -64,7 +64,7 @@ public:
>      return *ExpectedIndex;
>    }
>
> -  TypeIndex writeSerializedRecord(MutableArrayRef<uint8_t> Record) {
> +  TypeIndex writeSerializedRecord(ArrayRef<uint8_t> Record) {
>      return Serializer.insertRecordBytes(Record);
>    }
>
> @@ -77,7 +77,7 @@ public:
>      }
>    }
>
> -  ArrayRef<MutableArrayRef<uint8_t>> records() const {
> +  ArrayRef<ArrayRef<uint8_t>> records() const {
>      return Serializer.records();
>    }
>  };
>
> Modified: llvm/trunk/include/llvm/DebugInfo/CodeView/TypeTableCollection.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/
> llvm/DebugInfo/CodeView/TypeTableCollection.h?rev=
> 303576&r1=303575&r2=303576&view=diff
> ============================================================
> ==================
> --- llvm/trunk/include/llvm/DebugInfo/CodeView/TypeTableCollection.h
> (original)
> +++ llvm/trunk/include/llvm/DebugInfo/CodeView/TypeTableCollection.h Mon
> May 22 16:07:14 2017
> @@ -18,7 +18,7 @@ namespace codeview {
>
>  class TypeTableCollection : public TypeCollection {
>  public:
> -  explicit TypeTableCollection(ArrayRef<MutableArrayRef<uint8_t>>
> Records);
> +  explicit TypeTableCollection(ArrayRef<ArrayRef<uint8_t>> Records);
>
>    Optional<TypeIndex> getFirst() override;
>    Optional<TypeIndex> getNext(TypeIndex Prev) override;
> @@ -33,7 +33,7 @@ private:
>    bool hasCapacityFor(TypeIndex Index) const;
>    void ensureTypeExists(TypeIndex Index);
>
> -  ArrayRef<MutableArrayRef<uint8_t>> Records;
> +  ArrayRef<ArrayRef<uint8_t>> Records;
>    TypeDatabase Database;
>  };
>  }
>
> Modified: llvm/trunk/lib/DebugInfo/CodeView/TypeSerializer.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> DebugInfo/CodeView/TypeSerializer.cpp?rev=303576&
> r1=303575&r2=303576&view=diff
> ============================================================
> ==================
> --- llvm/trunk/lib/DebugInfo/CodeView/TypeSerializer.cpp (original)
> +++ llvm/trunk/lib/DebugInfo/CodeView/TypeSerializer.cpp Mon May 22
> 16:07:14 2017
> @@ -52,43 +52,24 @@ Error TypeSerializer::writeRecordPrefix(
>  }
>
>  TypeIndex
> -TypeSerializer::insertRecordBytesPrivate(MutableArrayRef<uint8_t>
> Record) {
> +TypeSerializer::insertRecordBytesPrivate(ArrayRef<uint8_t> &Record) {
>    assert(Record.size() % 4 == 0 && "Record is not aligned to 4 bytes!");
>
>    StringRef S(reinterpret_cast<const char *>(Record.data()),
> Record.size());
>
>    TypeIndex NextTypeIndex = calcNextTypeIndex();
>    auto Result = HashedRecords.try_emplace(S, NextTypeIndex);
> +
> +  StringRef NewData = Result.first->getKey();
> +  Record = ArrayRef<uint8_t>(NewData.bytes_begin(), NewData.bytes_end());
> +
>    if (Result.second) {
> +    // If this triggered an insert into the map, store the bytes.
>      LastTypeIndex = NextTypeIndex;
>      SeenRecords.push_back(Record);
>    }
> -  return Result.first->getValue();
> -}
>
> -TypeIndex
> -TypeSerializer::insertRecordBytesWithCopy(CVType &Record,
> -                                          MutableArrayRef<uint8_t> Data) {
> -  assert(Data.size() % 4 == 0 && "Record is not aligned to 4 bytes!");
> -
> -  StringRef S(reinterpret_cast<const char *>(Data.data()), Data.size());
> -
> -  // Do a two state lookup / insert so that we don't have to allocate
> unless
> -  // we're going
> -  // to do an insert.  This is a big memory savings.
> -  auto Iter = HashedRecords.find(S);
> -  if (Iter != HashedRecords.end())
> -    return Iter->second;
> -
> -  LastTypeIndex = calcNextTypeIndex();
> -  uint8_t *Copy = RecordStorage.Allocate<uint8_t>(Data.size());
> -  ::memcpy(Copy, Data.data(), Data.size());
> -  Data = MutableArrayRef<uint8_t>(Copy, Data.size());
> -  S = StringRef(reinterpret_cast<const char *>(Data.data()),
> Data.size());
> -  HashedRecords.insert(std::make_pair(S, LastTypeIndex));
> -  SeenRecords.push_back(Data);
> -  Record.RecordData = Data;
> -  return LastTypeIndex;
> +  return Result.first->getValue();
>  }
>
>  Expected<MutableArrayRef<uint8_t>>
> @@ -112,19 +93,19 @@ TypeSerializer::TypeSerializer(BumpPtrAl
>      : RecordStorage(Storage), LastTypeIndex(),
>        RecordBuffer(MaxRecordLength * 2),
>        Stream(RecordBuffer, llvm::support::little), Writer(Stream),
> -      Mapping(Writer) {
> +      Mapping(Writer), HashedRecords(Storage) {
>    // RecordBuffer needs to be able to hold enough data so that if we are 1
>    // byte short of MaxRecordLen, and then we try to write MaxRecordLen
> bytes,
>    // we won't overflow.
>  }
>
> -ArrayRef<MutableArrayRef<uint8_t>> TypeSerializer::records() const {
> +ArrayRef<ArrayRef<uint8_t>> TypeSerializer::records() const {
>    return SeenRecords;
>  }
>
>  TypeIndex TypeSerializer::getLastTypeIndex() const { return
> LastTypeIndex; }
>
> -TypeIndex TypeSerializer::insertRecordBytes(MutableArrayRef<uint8_t>
> Record) {
> +TypeIndex TypeSerializer::insertRecordBytes(ArrayRef<uint8_t> Record) {
>    assert(!TypeKind.hasValue() && "Already in a type mapping!");
>    assert(Writer.getOffset() == 0 && "Stream has data already!");
>
> @@ -163,8 +144,8 @@ Expected<TypeIndex> TypeSerializer::visi
>    Prefix->RecordLen = ThisRecordData.size() - sizeof(uint16_t);
>
>    Record.Type = *TypeKind;
> -  TypeIndex InsertedTypeIndex =
> -      insertRecordBytesWithCopy(Record, ThisRecordData);
> +  Record.RecordData = ThisRecordData;
> +  TypeIndex InsertedTypeIndex = insertRecordBytesPrivate(
> Record.RecordData);
>
>    // Write out each additional segment in reverse order, and update each
>    // record's continuation index to point to the previous one.
>
> Modified: llvm/trunk/lib/DebugInfo/CodeView/TypeTableCollection.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> DebugInfo/CodeView/TypeTableCollection.cpp?rev=303576&r1=303575&r2=303576&
> view=diff
> ============================================================
> ==================
> --- llvm/trunk/lib/DebugInfo/CodeView/TypeTableCollection.cpp (original)
> +++ llvm/trunk/lib/DebugInfo/CodeView/TypeTableCollection.cpp Mon May 22
> 16:07:14 2017
> @@ -25,7 +25,7 @@ static void error(Error &&EC) {
>  }
>
>  TypeTableCollection::TypeTableCollection(
> -    ArrayRef<MutableArrayRef<uint8_t>> Records)
> +  ArrayRef<ArrayRef<uint8_t>> Records)
>      : Records(Records), Database(Records.size()) {}
>
>  Optional<TypeIndex> TypeTableCollection::getFirst() {
>
> Modified: llvm/trunk/tools/llvm-pdbdump/llvm-pdbdump.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-
> pdbdump/llvm-pdbdump.cpp?rev=303576&r1=303575&r2=303576&view=diff
> ============================================================
> ==================
> --- llvm/trunk/tools/llvm-pdbdump/llvm-pdbdump.cpp (original)
> +++ llvm/trunk/tools/llvm-pdbdump/llvm-pdbdump.cpp Mon May 22 16:07:14
> 2017
> @@ -878,11 +878,11 @@ static void mergePdbs() {
>    auto &DestTpi = Builder.getTpiBuilder();
>    auto &DestIpi = Builder.getIpiBuilder();
>    MergedTpi.ForEachRecord(
> -      [&DestTpi](TypeIndex TI, MutableArrayRef<uint8_t> Data) {
> +      [&DestTpi](TypeIndex TI, ArrayRef<uint8_t> Data) {
>          DestTpi.addTypeRecord(Data, None);
>        });
>    MergedIpi.ForEachRecord(
> -      [&DestIpi](TypeIndex TI, MutableArrayRef<uint8_t> Data) {
> +      [&DestIpi](TypeIndex TI, ArrayRef<uint8_t> Data) {
>          DestIpi.addTypeRecord(Data, None);
>        });
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170522/0bee7ed4/attachment-0001.html>


More information about the llvm-commits mailing list