[PATCH] D61277: [PDB] Fix hash function used to write /src/headerblock

[Nico Weber via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/DebugInfo/PDB/Native/PDBStringTableBuilder.cpp:29-35
+  // The reference implementation doesn't include code for /src/headerblock
+  // handling, but it can only read natvis entries lld's PDB files if
+  // this hash function truncates the hash to 16 bit.
+  // PDB/include/misc.h in the reference implementation has a hashSz() function
+  // that returns an unsigned short, that seems what's being used for
+  // /src/headerblock.
+  return static_cast<uint16_t>(Table->getIdForString(S));
----------------
zturner wrote:
> IIUC, this shouldn't be specific to `/src/headerblock` right?  Because this `StringTableHashTraits` is used for the entire string table.  This means the comment is overly specific as it's referring to `/src/headerblock` from code that ultimately doesn't really have anything to do with `/src/headerblock`.  
> 
> It also means that changing this will probably affect lots of other things that are subtle and hard to test.  So be on the lookout for other weird failures.
>From what I understand, InjectedSourceTable is the only thing using this StringTableHashTraits: http://llvm-cs.pcc.me.uk/include/llvm/DebugInfo/PDB/Native/PDBStringTableBuilder.h/rStringTableHashTraits

Is that not the case?


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

https://reviews.llvm.org/D61277