Make parseBitcodeFile use a named StructType, if it exists and matches.

[arnt]

Summary

Until now, parseBitcodeFile() would use an already-existing StructType if
it EITHER had the right structure and no name, OR the right name and no
structure, but not if it had both the right name and the right structure.
This change makes parseBitcodeFile() check the struct and use it if
appropriate.

It still creates a new StructType with a new name if the existing type and
what it needs to read differs in structure.

It also add a new function to retrieve a named StructType by name. The new
function is a lookup-only function; it doesn't change anything at all.

[arnt]

I added lebedev.ri and the last two people to touch the file as reviewers, I hope that's not too much of an imposition.

[dnsampaio]

Sorry, I don't believe I ever touched these files before.

[lebedev.ri]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

1601

Pass by non-const value

1602

ArrayRef

1603

I'm not sure this is clang-formatted

1608–1611

if(!EltTys.equals(Candidate->elements()))
  return nullptr;

llvm/lib/IR/Type.cpp

342

nullptr

348

sporadic newline

[arnt]

more LLVM-y style, and better style too. Modern.

[arnt]

@dnsampaio Sorry about that. I looked at my shell history now, and I think I added you because I got a wildcard wrong and picked the most recent two committers for the wrong set of files.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

1608–1611

Oh, that's neat. I hadn't noticed that being done elsewhere, but I like it better. I'll push as soon as the unit tests have run.

[lebedev.ri]

(not all inline remarks were addressed)

llvm/include/llvm/IR/DerivedTypes.h

272–277

This probably deserves a /// doxygen comment

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

1602–1603

here and elsewhere - not from clang-format
(you can just setup a git pre-commit hook so all your commits will have correct formatting)

[arnt]

I didn't mean to suggest that I'd done all of them, I just ran out of working time on Friday. I attended to the rest now and will push a new revision as soon as the tests have run.

llvm/include/llvm/IR/DerivedTypes.h

272–277

Yes.

There could be a lot more of them in general. Doxygen sucks.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

1602–1603

I added the emacs integration. I'll try the git commit hook before my next pull request, thanks for the suggestion.

[arnt]

Attended to the rest of Lebedev.ri's comments, plus clang-format

[lebedev.ri]

For reference, there is a trivial way to correctly format all the changed code:
https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

llvm/unittests/Bitcode/BitReaderTest.cpp

217

EXPECT_EQ() / EXPECT_ME() ?

[arnt]

Thanks.

FWIW I noticed that the author of the git-commit hook suggested using an emacs-lisp clang-format wrapper, and decided to try that first, until I grow unhappy.

llvm/unittests/Bitcode/BitReaderTest.cpp

217

Yes, OK, will fix, although YAGNI and so on.

[arnt]

This revision uses more appropriate gtest matchers.

[arnt]

This patch overlooks a case and should not be merged for now.

Specifically, forward references in the bitcode file cause BitcodeReader::createIdentifiedStructType(LLVMContext &) to create a type, even though the intended type might already exist in the LLVMContext. I'm not sure how to create a test that exercises this clearly and cleanly, or whether this can really be solved. Will have a coffee and a think.

JFYI, my eventual purpose is to do things like reading a class's superclass's Module into the same LLVMContext as the class's own Module.

[arnt]

This modifies the patch to handle forward type references, and adds
relevant unit testing.

Unfortunately this is a rather large change.

[arnt]

Whitespace changes only.

It appears that the emacs clang-format stuff is too demanding for me; I'll
try to the git precommit hook instead and see if that agrees better with
me.

[lebedev.ri]

This patch seems to make sense but as it is all but guaranteed for LLVM patches, it is stalling and is starting to bitrot.
If you're still interested in it i'd maybe recommend posting to llvm-dev, or llvm-weekly review-wanted section

[arnt]

Rebased on top of today's LLVM.

No functional changes, I just had to fix a couple of merge conflicts.

I'll continue rebasing this (I really need it myself) and I'd be happy to
see it reviewed and merged. But if noone else cares about the
functionality, then I don't think I ought to pester anyone for reviews...
so I won't, at least not in the near future.

[t.p.northover]

I think the clang-format should be restricted to the actual diff. Doing the whole file makes this change really hard to read, and disrupts the blame for the future. git-clang-format in the Clang repo does the right thing by default I think.

[arnt]

Sorry about that; I'll revisit. But I won't have time to do this today. FWIW the patch looked right (ie. only my lines were touched by clang-format) immediately before I ran arc diff.

Sigh.

[arnt]

This should avoid clang-formatting the entire files. Should.

Awfully sorry about that.

[t.p.northover]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

1824–1826

Really? I thought loading multiple bitcode files into the same LLVMContext was fine and conflicting types were automatically renamed.

Am I misreading or will this turn that situation into an error?

[t.p.northover]

Sorry for the double reply, Phab ate my comment.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

1604–1605

This looks like it owns the SmallVector so it would be a lot simpler to make it a simple value type and std::move when emplacing into the std::vector. Reduce the size of the diff substantially too, I think.

[arnt]

I have to type this in order to publish the comments above?

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

1604–1605

Sure, I can do that... if this is going to be merged at all. But I have a feeling that this isn't going to be merged anywway, for substantive reasons. It rocks an important boat and only I have a reason for wanting it. So I'll wait with this.

1824–1826

The test for "conflicting type" was not terribly sensible. It assumed that if all modules contained opaque references to the same opaque type, then the the same name meant the same type. However, if one module had a defined type and the others had opaque types, then it was a naming conflict, which was silently resolved by renaming.

The case that blew up for me involved loading the superclasses for a class into the same context. Suppose that C inherits B, which inherits A, and each has its own type and module. A's module contains one or more A-related struct types, B's and C's module contain opaque references. B's module contains, etc. Loading B and C into the same context would break, because C's references to B would be renamed, while those to A would be preserved. I struggled to find a rationale for this.

I chose to make it an error because there are cases where renaming isn't safe, and I can't see a way to detect it. Suppose modules D and E both contain defined struct types T, and module F contains an opaque T. What is intended? The existing code would behave differently depending on load order. If you load only D and E, renaming is safe. But when loading the second of D/E, the reader doesn't know whether F will appear.

[t.p.northover]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

1824–1826

Loading B and C into the same context would break, because C's references to B would be renamed, while those to A would be preserved. I struggled to find a rationale for this.

It sounds like it'd result in weird names and inconsistent usage of each type, but not fundamentally change the semantics of the IR. I believe this is how LTO actually works.

Suppose modules D and E both contain defined struct types T, and module F contains an opaque T. What is intended?

D and E would continue to use a type structurally equivalent to their version of T, but one of them would have to give up the name (and get something like %T.1).

I don't believe there are any constraints on what happens to F: it could use yet another (still) opaque %T.2 or either of the versions from D/E. Because the type was opaque it can't be doing any operations that actually care.

llvm/include/llvm/IR/DerivedTypes.h

public:
   template <class... Tys>
   static typename std::enable_if<are_base_of<Type, Tys...>::value,
                                  StructType *>::type
   create(StringRef Name, Type *elt1, Tys *... elts) {
     assert(elt1 && "Cannot create a struct type with no elements with this");
     SmallVector<llvm::Type *, 8> StructFields({elt1, elts...});
     return create(StructFields, Name);
   }
 
+  /// This static method returns a StructType by that name if one exists, and a
+  /// null pointer otherwise.
+  static StructType *getIfExists(LLVMContext &Context, StringRef Name);
+
   /// This static method is the primary way to create a literal StructType.
   static StructType *get(LLVMContext &Context, ArrayRef<Type*> Elements,
                          bool isPacked = false);
 
   /// Create an empty structure type.
   static StructType *get(LLVMContext &Context, bool isPacked = false);
 
   /// This static method is a convenience method for creating structure types by
   /// specifying the elements as arguments. Note that this method always returns

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

public:
 
   /// Materialize any deferred Metadata block.
   Error materializeMetadata() override;
 
   void setStripDebugInfo() override;
 
 private:
   std::vector<StructType *> IdentifiedStructTypes;
-  StructType *createIdentifiedStructType(LLVMContext &Context, StringRef Name);
-  StructType *createIdentifiedStructType(LLVMContext &Context);
 
   Type *getTypeByID(unsigned ID);
 
   Value *getFnValueByID(unsigned ID, Type *Ty) {
     if (Ty && Ty->isMetadataTy())
       return MetadataAsValue::get(Ty->getContext(), getFnMetadataByID(ID));
     return ValueList.getValueFwdRef(ID, Ty);
   }
static void upgradeDLLImportExportLinkage(GlobalValue *GV, unsigned Val) {
   }
 }
 
 Type *BitcodeReader::getTypeByID(unsigned ID) {
   // The type table size is always specified correctly.
   if (ID >= TypeList.size())
     return nullptr;
 
-  if (Type *Ty = TypeList[ID])
-    return Ty;
-
-  // If we have a forward reference, the only possible case is when it is to a
-  // named struct.  Just create a placeholder for now.
-  return TypeList[ID] = createIdentifiedStructType(Context);
-}
-
-StructType *BitcodeReader::createIdentifiedStructType(LLVMContext &Context,
-                                                      StringRef Name) {
-  auto *Ret = StructType::create(Context, Name);
-  IdentifiedStructTypes.push_back(Ret);
-  return Ret;
-}
-
-StructType *BitcodeReader::createIdentifiedStructType(LLVMContext &Context) {
-  auto *Ret = StructType::create(Context);
-  IdentifiedStructTypes.push_back(Ret);
-  return Ret;
+  return TypeList[ID];
 }
 
 //===----------------------------------------------------------------------===//
 //  Functions for parsing blocks from the bitcode file
 //===----------------------------------------------------------------------===//
 
 static uint64_t getRawAttributeMask(Attribute::AttrKind Val) {
   switch (Val) {
Error BitcodeReader::parseTypeTable() {
   if (Error Err = Stream.EnterSubBlock(bitc::TYPE_BLOCK_ID_NEW))
     return Err;
 
   return parseTypeTableBody();
 }
 
 Error BitcodeReader::parseTypeTableBody() {
   if (!TypeList.empty())
     return error("Invalid multiple blocks");
 
-  SmallVector<uint64_t, 64> Record;
-  unsigned NumRecords = 0;
-
   SmallString<64> TypeName;
+  std::vector<std::pair<unsigned, std::shared_ptr<SmallVector<uint64_t, 64>>>>
+      TypeRecords;
 
   // Read all the records for this type table.
-  while (true) {
+  bool Done = false;
+  while (!Done) {
     Expected<BitstreamEntry> MaybeEntry = Stream.advanceSkippingSubblocks();
     if (!MaybeEntry)
       return MaybeEntry.takeError();
     BitstreamEntry Entry = MaybeEntry.get();
 
     switch (Entry.Kind) {
     case BitstreamEntry::SubBlock: // Handled for us already.
     case BitstreamEntry::Error:
       return error("Malformed block");
     case BitstreamEntry::EndBlock:
-      if (NumRecords != TypeList.size())
+      if (TypeRecords.size() != TypeList.size())
         return error("Malformed block");
-      return Error::success();
+      Done = true;
+      break;
     case BitstreamEntry::Record:
-      // The interesting case.
+      std::shared_ptr<SmallVector<uint64_t, 64>> Record(
+          new SmallVector<uint64_t, 64>());
+      Expected<unsigned> MaybeCode = Stream.readRecord(Entry.ID, *Record);
+      if(!MaybeCode)
+        return MaybeCode.takeError();
+      unsigned Code = MaybeCode.get();
+      switch (Code) {
+      case bitc::TYPE_CODE_NUMENTRY: // TYPE_CODE_NUMENTRY: [numentries]
+        // TYPE_CODE_NUMENTRY contains a count of the number of types in the
+        // type list.  This allows us to reserve space.
+        if (Record->size() < 1)
+          return error("Invalid record");
+        TypeList.resize((*Record)[0]);
+        break;
+      case bitc::TYPE_CODE_STRUCT_NAME: // STRUCT_NAME: [strchr x N]
+        // TYPE_CODE_STRUCT_NAME provides a name for the next struct type. We
+        // provide that type already, so forward references can reach it. If
+        // necessary the type is left opaque for now.
+        if (convertToString(*Record, 0, TypeName))
+          return error("Invalid record");
+        break;
+      case bitc::TYPE_CODE_STRUCT_NAMED:
+      case bitc::TYPE_CODE_OPAQUE: {
+        StructType *NamedStructType = nullptr;
+        if (!TypeName.empty())
+          NamedStructType = StructType::getIfExists(Context, TypeName);
+        if (!NamedStructType)
+          NamedStructType = StructType::create(Context, TypeName);
+        TypeList[TypeRecords.size()] = NamedStructType;
+        IdentifiedStructTypes.push_back(NamedStructType);
+        TypeName.clear();
+      }
+        LLVM_FALLTHROUGH;
+      default:
+        // All other records are left to process once the forward references are
+        // resolvable.
+        TypeRecords.push_back(make_pair(Code, Record));
         break;
       }
+      break;
+    }
+  }
 
-    // Read a record.
-    Record.clear();
+  unsigned NumRecords = 0;
+  // Process the deferred type records.
+  for (auto TypeRecord : TypeRecords) {
+    auto Record = TypeRecord.second;
     Type *ResultTy = nullptr;
-    Expected<unsigned> MaybeRecord = Stream.readRecord(Entry.ID, Record);
-    if (!MaybeRecord)
-      return MaybeRecord.takeError();
-    switch (MaybeRecord.get()) {
+    switch (TypeRecord.first) {
     default:
       return error("Invalid value");
-    case bitc::TYPE_CODE_NUMENTRY: // TYPE_CODE_NUMENTRY: [numentries]
-      // TYPE_CODE_NUMENTRY contains a count of the number of types in the
-      // type list.  This allows us to reserve space.
-      if (Record.size() < 1)
-        return error("Invalid record");
-      TypeList.resize(Record[0]);
-      continue;
-    case bitc::TYPE_CODE_VOID:      // VOID
+    case bitc::TYPE_CODE_VOID: // VOID
       ResultTy = Type::getVoidTy(Context);
       break;
-    case bitc::TYPE_CODE_HALF:     // HALF
+    case bitc::TYPE_CODE_HALF: // HALF
       ResultTy = Type::getHalfTy(Context);
       break;
-    case bitc::TYPE_CODE_FLOAT:     // FLOAT
+    case bitc::TYPE_CODE_FLOAT: // FLOAT
       ResultTy = Type::getFloatTy(Context);
       break;
-    case bitc::TYPE_CODE_DOUBLE:    // DOUBLE
+    case bitc::TYPE_CODE_DOUBLE: // DOUBLE
       ResultTy = Type::getDoubleTy(Context);
       break;
-    case bitc::TYPE_CODE_X86_FP80:  // X86_FP80
+    case bitc::TYPE_CODE_X86_FP80: // X86_FP80
       ResultTy = Type::getX86_FP80Ty(Context);
       break;
-    case bitc::TYPE_CODE_FP128:     // FP128
+    case bitc::TYPE_CODE_FP128: // FP128
       ResultTy = Type::getFP128Ty(Context);
       break;
     case bitc::TYPE_CODE_PPC_FP128: // PPC_FP128
       ResultTy = Type::getPPC_FP128Ty(Context);
       break;
-    case bitc::TYPE_CODE_LABEL:     // LABEL
+    case bitc::TYPE_CODE_LABEL: // LABEL
       ResultTy = Type::getLabelTy(Context);
       break;
-    case bitc::TYPE_CODE_METADATA:  // METADATA
+    case bitc::TYPE_CODE_METADATA: // METADATA
       ResultTy = Type::getMetadataTy(Context);
       break;
-    case bitc::TYPE_CODE_X86_MMX:   // X86_MMX
+    case bitc::TYPE_CODE_X86_MMX: // X86_MMX
       ResultTy = Type::getX86_MMXTy(Context);
       break;
-    case bitc::TYPE_CODE_TOKEN:     // TOKEN
+    case bitc::TYPE_CODE_TOKEN: // TOKEN
       ResultTy = Type::getTokenTy(Context);
       break;
     case bitc::TYPE_CODE_INTEGER: { // INTEGER: [width]
-      if (Record.size() < 1)
+      if (Record->size() < 1)
         return error("Invalid record");
 
-      uint64_t NumBits = Record[0];
+      uint64_t NumBits = (*Record)[0];
       if (NumBits < IntegerType::MIN_INT_BITS ||
           NumBits > IntegerType::MAX_INT_BITS)
         return error("Bitwidth for integer type out of range");
       ResultTy = IntegerType::get(Context, NumBits);
       break;
     }
     case bitc::TYPE_CODE_POINTER: { // POINTER: [pointee type] or
                                     //          [pointee type, address space]
-      if (Record.size() < 1)
+      if (Record->size() < 1)
         return error("Invalid record");
       unsigned AddressSpace = 0;
-      if (Record.size() == 2)
-        AddressSpace = Record[1];
-      ResultTy = getTypeByID(Record[0]);
-      if (!ResultTy ||
-          !PointerType::isValidElementType(ResultTy))
+      if (Record->size() == 2)
+        AddressSpace = (*Record)[1];
+      ResultTy = getTypeByID((*Record)[0]);
+      if (!ResultTy || !PointerType::isValidElementType(ResultTy))
         return error("Invalid type");
       ResultTy = PointerType::get(ResultTy, AddressSpace);
       break;
     }
     case bitc::TYPE_CODE_FUNCTION_OLD: {
       // FIXME: attrid is dead, remove it in LLVM 4.0
       // FUNCTION: [vararg, attrid, retty, paramty x N]
-      if (Record.size() < 3)
+      if (Record->size() < 3)
         return error("Invalid record");
-      SmallVector<Type*, 8> ArgTys;
-      for (unsigned i = 3, e = Record.size(); i != e; ++i) {
-        if (Type *T = getTypeByID(Record[i]))
+      SmallVector<Type *, 8> ArgTys;
+      for (unsigned i = 3, e = Record->size(); i != e; ++i) {
+        if (Type *T = getTypeByID((*Record)[i]))
           ArgTys.push_back(T);
         else
           break;
       }
 
-      ResultTy = getTypeByID(Record[2]);
-      if (!ResultTy || ArgTys.size() < Record.size()-3)
+      ResultTy = getTypeByID((*Record)[2]);
+      if (!ResultTy || ArgTys.size() < Record->size() - 3)
         return error("Invalid type");
 
-      ResultTy = FunctionType::get(ResultTy, ArgTys, Record[0]);
+      ResultTy = FunctionType::get(ResultTy, ArgTys, (*Record)[0]);
       break;
     }
     case bitc::TYPE_CODE_FUNCTION: {
       // FUNCTION: [vararg, retty, paramty x N]
-      if (Record.size() < 2)
+      if (Record->size() < 2)
         return error("Invalid record");
-      SmallVector<Type*, 8> ArgTys;
-      for (unsigned i = 2, e = Record.size(); i != e; ++i) {
-        if (Type *T = getTypeByID(Record[i])) {
+      SmallVector<Type *, 8> ArgTys;
+      for (unsigned i = 2, e = Record->size(); i != e; ++i) {
+        if (Type *T = getTypeByID((*Record)[i])) {
           if (!FunctionType::isValidArgumentType(T))
             return error("Invalid function argument type");
           ArgTys.push_back(T);
-        }
-        else
+        } else
           break;
       }
 
-      ResultTy = getTypeByID(Record[1]);
-      if (!ResultTy || ArgTys.size() < Record.size()-2)
+      ResultTy = getTypeByID((*Record)[1]);
+      if (!ResultTy || ArgTys.size() < Record->size() - 2)
         return error("Invalid type");
 
-      ResultTy = FunctionType::get(ResultTy, ArgTys, Record[0]);
+      ResultTy = FunctionType::get(ResultTy, ArgTys, (*Record)[0]);
       break;
     }
-    case bitc::TYPE_CODE_STRUCT_ANON: {  // STRUCT: [ispacked, eltty x N]
-      if (Record.size() < 1)
+    case bitc::TYPE_CODE_STRUCT_ANON: { // STRUCT: [ispacked, eltty x N]
+      if (Record->size() < 1)
         return error("Invalid record");
-      SmallVector<Type*, 8> EltTys;
-      for (unsigned i = 1, e = Record.size(); i != e; ++i) {
-        if (Type *T = getTypeByID(Record[i]))
+      SmallVector<Type *, 8> EltTys;
+      for (unsigned i = 1, e = Record->size(); i != e; ++i) {
+        if (Type *T = getTypeByID((*Record)[i]))
           EltTys.push_back(T);
         else
           break;
       }
-      if (EltTys.size() != Record.size()-1)
+      if (EltTys.size() != Record->size() - 1)
         return error("Invalid type");
-      ResultTy = StructType::get(Context, EltTys, Record[0]);
+      ResultTy = StructType::get(Context, EltTys, (*Record)[0]);
       break;
     }
-    case bitc::TYPE_CODE_STRUCT_NAME:   // STRUCT_NAME: [strchr x N]
-      if (convertToString(Record, 0, TypeName))
-        return error("Invalid record");
-      continue;
-
     case bitc::TYPE_CODE_STRUCT_NAMED: { // STRUCT: [ispacked, eltty x N]
-      if (Record.size() < 1)
+      if (Record->size() < 1)
         return error("Invalid record");
 
       if (NumRecords >= TypeList.size())
         return error("Invalid TYPE table");
 
-      // Check to see if this was forward referenced, if so fill in the temp.
-      StructType *Res = cast_or_null<StructType>(TypeList[NumRecords]);
-      if (Res) {
-        Res->setName(TypeName);
-        TypeList[NumRecords] = nullptr;
-      } else  // Otherwise, create a new struct.
-        Res = createIdentifiedStructType(Context, TypeName);
-      TypeName.clear();
-
-      SmallVector<Type*, 8> EltTys;
-      for (unsigned i = 1, e = Record.size(); i != e; ++i) {
-        if (Type *T = getTypeByID(Record[i]))
+      SmallVector<Type *, 8> EltTys;
+      for (unsigned i = 1, e = Record->size(); i != e; ++i) {
+        if (Type *T = getTypeByID((*Record)[i]))
           EltTys.push_back(T);
         else
           break;
       }
-      if (EltTys.size() != Record.size()-1)
+      if (EltTys.size() != Record->size() - 1)
         return error("Invalid record");
-      Res->setBody(EltTys, Record[0]);
+
+      StructType *Res = cast<StructType>(TypeList[NumRecords]);
+      TypeList[NumRecords] = nullptr;
+      if (Res->isOpaque()) {
+        Res->setBody(EltTys, (*Record)[0]);
+      } else if (!Res->elements().equals(EltTys)) {
+        // Ouch! The LLVMContext's existing named struct type and the one being
+        // read have different structures. This must mean that the LLVMContext
+        // contains more than one module, and there is disagreement.  Several
+        // possibilities: 1a. There are no opaque references to structs. In this
+        // case renaming either type is safe (but perhaps not desirable).
+        // 1b. There are opaque references, but no further Modules will be read.
+        // In this case renaming the type in the new module is safe. 2a. There
+        // may be opaque references, and all are to the type that's already in
+        // the Context. In this case renaming the new type is safe. 2b. There
+        // may be opaque references, and all are to the type that's being
+        // read. In this case renaming the existing type is safe. 2c. There may
+        // be opaque references to either. In this case nothing is safe.
+
+        // 3. This kind of conflict should not happen.
+        return error(
+            "named struct types match by name and differ by structure");
+      }
+
       ResultTy = Res;
       break;
     }
-    case bitc::TYPE_CODE_OPAQUE: {       // OPAQUE: []
-      if (Record.size() != 1)
+    case bitc::TYPE_CODE_OPAQUE: { // OPAQUE: []
+      if (Record->size() != 1)
         return error("Invalid record");
 
       if (NumRecords >= TypeList.size())
         return error("Invalid TYPE table");
 
-      // Check to see if this was forward referenced, if so fill in the temp.
-      StructType *Res = cast_or_null<StructType>(TypeList[NumRecords]);
-      if (Res) {
-        Res->setName(TypeName);
-        TypeList[NumRecords] = nullptr;
-      } else  // Otherwise, create a new struct with no body.
-        Res = createIdentifiedStructType(Context, TypeName);
-      TypeName.clear();
+      StructType *Res = cast<StructType>(TypeList[NumRecords]);
       ResultTy = Res;
       break;
     }
-    case bitc::TYPE_CODE_ARRAY:     // ARRAY: [numelts, eltty]
-      if (Record.size() < 2)
+    case bitc::TYPE_CODE_ARRAY: // ARRAY: [numelts, eltty]
+      if (Record->size() < 2)
         return error("Invalid record");
-      ResultTy = getTypeByID(Record[1]);
+      ResultTy = getTypeByID((*Record)[1]);
       if (!ResultTy || !ArrayType::isValidElementType(ResultTy))
         return error("Invalid type");
-      ResultTy = ArrayType::get(ResultTy, Record[0]);
+      ResultTy = ArrayType::get(ResultTy, (*Record)[0]);
       break;
     case bitc::TYPE_CODE_VECTOR:    // VECTOR: [numelts, eltty] or
                                     //         [numelts, eltty, scalable]
-      if (Record.size() < 2)
+      if (Record->size() < 2)
         return error("Invalid record");
-      if (Record[0] == 0)
+      if ((*Record)[0] == 0)
         return error("Invalid vector length");
-      ResultTy = getTypeByID(Record[1]);
+      ResultTy = getTypeByID((*Record)[1]);
       if (!ResultTy || !StructType::isValidElementType(ResultTy))
         return error("Invalid type");
-      bool Scalable = Record.size() > 2 ? Record[2] : false;
-      ResultTy = VectorType::get(ResultTy, Record[0], Scalable);
+      bool Scalable = Record->size() > 2 ? (*Record)[2] : false;
+      ResultTy = VectorType::get(ResultTy, (*Record)[0], Scalable);
       break;
     }
 
     if (NumRecords >= TypeList.size())
       return error("Invalid TYPE table");
     if (TypeList[NumRecords])
       return error(
           "Invalid TYPE table: Only named structs can be forward referenced");
     assert(ResultTy && "Didn't read a type?");
     TypeList[NumRecords++] = ResultTy;
   }
+  if (NumRecords < TypeList.size())
+    return error("Invalid TYPE table");
+  return Error::success();
 }
 
 Error BitcodeReader::parseOperandBundleTags() {
   if (Error Err = Stream.EnterSubBlock(bitc::OPERAND_BUNDLE_TAGS_BLOCK_ID))
     return Err;
 
   if (!BundleTags.empty())
     return error("Invalid multiple blocks");

llvm/lib/IR/Type.cpp

 bool FunctionType::isValidArgumentType(Type *ArgTy) {
   return ArgTy->isFirstClassType();
 }
 
 //===----------------------------------------------------------------------===//
 //                       StructType Implementation
 //===----------------------------------------------------------------------===//
 
+StructType *StructType::getIfExists(LLVMContext &Context, StringRef Name) {
+  auto I = Context.pImpl->NamedStructTypes.find(Name);
+  if (I == Context.pImpl->NamedStructTypes.end())
+    return nullptr;
+  return I->getValue();
+}
+
 // Primitive Constructors.
 
+
 StructType *StructType::get(LLVMContext &Context, ArrayRef<Type*> ETypes,
                             bool isPacked) {
   LLVMContextImpl *pImpl = Context.pImpl;
   const AnonStructTypeKeyInfo::KeyTy Key(ETypes, isPacked);
 
   StructType *ST;
   // Since we only want to allocate a fresh struct type in case none is found
   // and we don't want to perform two lookups (one for checking if existent and

llvm/unittests/Bitcode/BitReaderTest.cpp

TEST(BitReaderTest, MaterializeFunctionsForBlockAddrInFunctionAfter) {
 
   // Materialize @after, pulling in @func.
   EXPECT_FALSE(M->getFunction("after")->materialize());
   EXPECT_FALSE(M->getFunction("func")->empty());
   EXPECT_TRUE(M->getFunction("other")->empty());
   EXPECT_FALSE(verifyModule(*M, &dbgs()));
 }
 
+TEST(BitReaderTest, UseExistingNameStructType) {
+  // Make a module using a struct type, then write that.
+  LLVMContext C1;
+  StructType *T1 = StructType::create(C1, "Correct");
+  T1->setBody(Type::getInt32Ty(C1));
+  std::unique_ptr<Module> M1(new Module("M1", C1));
+  M1->getOrInsertFunction("F1", T1);
+  SmallString<1024> Memory;
+  raw_svector_ostream OS(Memory);
+  WriteBitcodeToFile(*M1, OS);
+
+  // Define that struct type in a new context. Read the module into that
+  // context. At this point, the context contains the StructType the module
+  // uses.
+  LLVMContext C2;
+  StructType *T2 = StructType::create(C2, T1->getName());
+  T2->setBody(Type::getInt32Ty(C2));
+  auto Careful = parseBitcodeFile(MemoryBufferRef(Memory.str(), "test"), C2);
+  EXPECT_TRUE((bool)Careful);
+  std::unique_ptr<Module> M2(Careful.get().release());
+
+  // Then one single test: was the already-present struct type used?
+  Function *F2 = M2->getFunction("F1");
+  EXPECT_NE(F2, nullptr);
+  EXPECT_EQ(cast<StructType>(F2->getReturnType())->getName(), T1->getName());
+
+  // Define a different StructType of the same name. Reading the bitcode into
+  // THAT context should fail.
+  LLVMContext C3;
+  StructType *T3 = StructType::create(C3, T1->getName());
+  T3->setBody(Type::getFloatTy(C3));
+  Careful = parseBitcodeFile(MemoryBufferRef(Memory.str(), "test"), C3);
+  EXPECT_FALSE((bool)Careful);
+  handleAllErrors(Careful.takeError(), [&](const ErrorInfoBase &DE) {});
+}
+
+TEST(BitReaderTest, ResolveForwardStructReferences) {
+  // This test is intended to resemble reading a subclass and a superclass from
+  // separate .bc files into the same LLVMContext. Subclasses and superclasses
+  // often contain functions (or methods if you will) with the same signature.
+  // This test approximates that using two functions with the same FunctionType.
+
+  // Make a context using two struct types that reference each other. One of the
+  // types necessarily has to contain a forward reference when written to the
+  // .bc files.
+
+  LLVMContext C1;
+  StructType *T1A = StructType::create(C1, "Struct1");
+  StructType *T2A = StructType::create(C1, "Struct2");
+  T1A->setBody(T2A->getPointerTo());
+  T2A->setBody(T1A->getPointerTo());
+
+  // Make two modules, each containing one function with the same function type.
+
+  std::unique_ptr<Module> M1A(new Module("M1", C1));
+  std::unique_ptr<Module> M2A(new Module("M2", C1));
+  M1A->getOrInsertFunction("F1", Type::getVoidTy(C1), T1A->getPointerTo(),
+                           T2A->getPointerTo());
+  M2A->getOrInsertFunction("F2", Type::getVoidTy(C1), T1A->getPointerTo(),
+                           T2A->getPointerTo());
+  EXPECT_EQ(M1A->getFunction("F1")->getFunctionType(),
+            M2A->getFunction("F2")->getFunctionType());
+
+  // Write both modules to separate .bc files, then read the .bc files into the
+  // same new context.
+
+  SmallString<1024> BC1;
+  raw_svector_ostream OS1(BC1);
+  WriteBitcodeToFile(*M1A, OS1);
+  SmallString<1024> BC2;
+  raw_svector_ostream OS2(BC2);
+  WriteBitcodeToFile(*M2A, OS2);
+
+  LLVMContext C2;
+  auto Careful = parseBitcodeFile(MemoryBufferRef(BC1.str(), "test"), C2);
+  EXPECT_TRUE((bool)Careful);
+  std::unique_ptr<Module> M1B(Careful.get().release());
+  Careful = parseBitcodeFile(MemoryBufferRef(BC2.str(), "test"), C2);
+  EXPECT_TRUE((bool)Careful);
+  std::unique_ptr<Module> M2B(Careful.get().release());
+
+  // The two functions should still have the same signature.
+
+  EXPECT_EQ(M1B->getFunction("F1")->getFunctionType(),
+            M2B->getFunction("F2")->getFunctionType());
+
+  // I wish I could EXPECT_THAT(BC1->containsForwardTypeReference()) but that
+  // seems entirely unreasonable.
+}
+
+TEST(BitReaderTest, CreateUnnamedStructTypes) {
+  // This test checks that two unnamed named struct types aren't inappropriately
+  // merged. I love it when I write a test just to be safe, and it passes the
+  // first time I run it.
+
+  LLVMContext C1;
+  StructType *T1 = StructType::create(C1);
+  StructType *T2 = StructType::create(C1);
+  T1->setBody(T2->getPointerTo());
+  // T2 intentionally opaque
+
+  std::unique_ptr<Module> M(new Module("M", C1));
+  M->getOrInsertFunction("F", Type::getVoidTy(C1), T1->getPointerTo(),
+                         T2->getPointerTo());
+
+  SmallString<1024> BC;
+  raw_svector_ostream OS(BC);
+  WriteBitcodeToFile(*M, OS);
+
+  LLVMContext C2;
+  StructType *T3 = StructType::create(C2);
+  auto Careful = parseBitcodeFile(MemoryBufferRef(BC.str(), "test"), C2);
+  EXPECT_TRUE((bool)Careful);
+
+  // There's no reason to believe that T1 or T2 matches T3, and T1 and T2
+  // definitely do not match each other. Verify all of that.
+
+  Function *F = Careful.get().release()->getFunction("F");
+  EXPECT_NE(nullptr, F);
+  FunctionType *FT = F->getFunctionType();
+  EXPECT_EQ(2U, FT->getNumParams());
+  EXPECT_NE(FT->getParamType(0), FT->getParamType(1));
+  EXPECT_NE(T3, FT->getParamType(0));
+  EXPECT_NE(T3, FT->getParamType(1));
+}
+
 } // end namespace