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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: lld/test/COFF/s_udt.test:1
+; RUN: llc -filetype=obj < %s > %t.obj
+; RUN: lld-link /DEBUG:FULL /nodefaultlib /entry:main %t.obj /PDB:%t.pdb /OUT:%t.exe
----------------
Any particular reason for this to be an IR test that uses llc? LTO isn't involved, so you could either do a yaml input or an assembly file input. Which ever is more readable.

If you do want an IR test, can we name it .ll? We should be able to add .ll to the list of test case suffixes if it's not in there already.


================
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;
+    }
+
----------------
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


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

https://reviews.llvm.org/D55228