[PATCH] D60616: Make parseBitcodeFile use a named StructType, if it exists and matches.

[Arnt Gulbrandsen via Phabricator via llvm-commits]

arnt marked 2 inline comments as done.
arnt added a comment.

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



================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:1604-1605
   SmallString<64> TypeName;
+  std::vector<std::pair<unsigned, std::shared_ptr<SmallVector<uint64_t, 64>>>>
+      TypeRecords;
 
----------------
t.p.northover wrote:
> 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.
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.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:1824-1826
+        // 3. This kind of conflict should not happen.
+        return error(
+            "named struct types match by name and differ by structure");
----------------
t.p.northover wrote:
> 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?
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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60616/new/

https://reviews.llvm.org/D60616