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

[Alexandre Ganea via Phabricator via llvm-commits]

aganea 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;
+    }
+
----------------
zturner wrote:
> 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.
Unless I'm missing something, @rnk is right: the PDB will not receive a colliding record. If you're inserting a S_UDT 'typedef char B' with hash **0x2**, and a previous 'typedef int A' with same hash **0x2** was already there in `UdtHashes`, the new record 'B' will not make it to `Records.push_back()`.


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

https://reviews.llvm.org/D55228