[PATCH] D55228: [PDB] Emit S_UDT records in LLD

[Zachary Turner via Phabricator via llvm-commits]

zturner marked an inline comment as done.
zturner added inline comments.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/GSIStreamBuilder.cpp:52-59
+    if (Symbol.kind() == S_UDT) {
+      uint64_t Hash = xxHash64(Symbol.RecordData);
+      auto Iter = UdtHashes.insert(Hash);
+      if (!Iter.second)
+        return;
+    }
+
----------------
rnk wrote:
> I don't think we should assume xxHash64 doesn't have collisions. The space of 64-bit hashes is big, but it's not a content hash like SHA1, so it should be possible to construct some collisions. Someone has code to do it here: https://github.com/Cyan4973/xxHash/issues/54
> 
> If you don't want to add the collision resolution code because it's slow, at least try linking browser_tests with extra instrumentation code to detect collisions. If you find a collision, then we should keep the resolution code and add that as a test case. If you don't find a collision, then at least put a large comment here documenting our decision to sometimes drop typedefs with matching xxHash64 values.
> 
> Also, you might want to structure this code in such a way that it can be extended for S_CONSTANT, which also needs deduplication: https://bugs.llvm.org/show_bug.cgi?id=37933
The thing about collisions is that the consequences of a collision are that we end up with a few extra bytes in the PDB.  So it seems unimportant to worry about a collision, even in the case that we do have collisions.


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

https://reviews.llvm.org/D55228