From eaef1427b689cfc09a6f508972a5ef7043652cb0 Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Mon, 29 Apr 2019 23:09:35 +0000 Subject: [PATCH] [PDB] Fix hash function used to write /src/headerblock lld-link used to write PDB files that DIA couldn't recover natvis files from if: - The global strings table was > 64kiB - There were at least 3 natvis files The cause was that the hash function for the /src/headerblock stream was incorrect: It needs to be truncated to 16 bit. If the global strings table was <= 64kiB, truncating to 16 bit is a no-op, so this wasn't needed for small programs. If there are only 1 or 2 natvis files, then the growth strategy in HashTable::grow() would mean the hash table would have 2 buckets (for 1 natvis file) or 4 buckets (for 4 natvis files), and since the hash function is used modulo number of buckets, and since 2 and 4 divide 0x10000, the missing `% 0x10000` is a no-op there too. For 3 natvis files, the hash table grows to 6 buckets, which has a factor that's not common with 0x10000 and the difference starts to matter. Fixes PR41626. Differential Revision: https://reviews.llvm.org/D61277 llvm-svn: 359515 --- include/llvm/DebugInfo/PDB/Native/HashTable.h | 2 +- lib/DebugInfo/PDB/Native/NamedStreamMap.cpp | 1 + .../PDB/Native/PDBStringTableBuilder.cpp | 8 ++++++- .../DebugInfo/PDB/StringTableBuilderTest.cpp | 24 +++++++++++++++---- 4 files changed, 28 insertions(+), 7 deletions(-) diff --git a/include/llvm/DebugInfo/PDB/Native/HashTable.h b/include/llvm/DebugInfo/PDB/Native/HashTable.h index e8f08c68cd3..86c43a482b8 100644 --- a/include/llvm/DebugInfo/PDB/Native/HashTable.h +++ b/include/llvm/DebugInfo/PDB/Native/HashTable.h @@ -143,7 +143,7 @@ public: return EC; if (Present.intersects(Deleted)) return make_error(raw_error_code::corrupt_file, - "Present bit vector interesects deleted!"); + "Present bit vector intersects deleted!"); for (uint32_t P : Present) { if (auto EC = Stream.readInteger(Buckets[P].first)) diff --git a/lib/DebugInfo/PDB/Native/NamedStreamMap.cpp b/lib/DebugInfo/PDB/Native/NamedStreamMap.cpp index a9965ff8d06..1c044e0c265 100644 --- a/lib/DebugInfo/PDB/Native/NamedStreamMap.cpp +++ b/lib/DebugInfo/PDB/Native/NamedStreamMap.cpp @@ -34,6 +34,7 @@ uint16_t NamedStreamMapTraits::hashLookupKey(StringRef S) const { // Here, the type HASH is a typedef of unsigned short. // ** It is not a bug that we truncate the result of hashStringV1, in fact // it is a bug if we do not! ** + // See NMTNI::hash() in the reference implementation. return static_cast(hashStringV1(S)); } diff --git a/lib/DebugInfo/PDB/Native/PDBStringTableBuilder.cpp b/lib/DebugInfo/PDB/Native/PDBStringTableBuilder.cpp index 4b69e09a83d..eabb9d44669 100644 --- a/lib/DebugInfo/PDB/Native/PDBStringTableBuilder.cpp +++ b/lib/DebugInfo/PDB/Native/PDBStringTableBuilder.cpp @@ -26,7 +26,13 @@ StringTableHashTraits::StringTableHashTraits(PDBStringTableBuilder &Table) : Table(&Table) {} uint32_t StringTableHashTraits::hashLookupKey(StringRef S) const { - return Table->getIdForString(S); + // 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(Table->getIdForString(S)); } StringRef StringTableHashTraits::storageKeyToLookupKey(uint32_t Offset) const { diff --git a/unittests/DebugInfo/PDB/StringTableBuilderTest.cpp b/unittests/DebugInfo/PDB/StringTableBuilderTest.cpp index 719cb807a2b..2f7c0619440 100644 --- a/unittests/DebugInfo/PDB/StringTableBuilderTest.cpp +++ b/unittests/DebugInfo/PDB/StringTableBuilderTest.cpp @@ -19,11 +19,7 @@ using namespace llvm; using namespace llvm::pdb; using namespace llvm::support; -namespace { -class StringTableBuilderTest : public ::testing::Test {}; -} - -TEST_F(StringTableBuilderTest, Simple) { +TEST(StringTableBuilderTest, Simple) { // Create /names table contents. PDBStringTableBuilder Builder; @@ -78,3 +74,21 @@ TEST_F(StringTableBuilderTest, Simple) { EXPECT_THAT_EXPECTED(Table.getIDForString("bazz"), HasValue(BazzID)); EXPECT_THAT_EXPECTED(Table.getIDForString("barr"), HasValue(BarrID)); } + +TEST(StringTableHashTraitsTest, Simple) { + PDBStringTableBuilder Builder; + + // Create more than 64kiB of dummy entries. + for (int i = 0; i < 320; ++i) { + std::string aaaaa = std::string(220, 'a') + std::to_string(i); + Builder.insert(aaaaa); + } + + std::string S = "foo.natvis"; + uint32_t Pos = Builder.insert(S); + + EXPECT_GT(Pos, 0xFFFFu); + + StringTableHashTraits Traits(Builder); + EXPECT_LE(Traits.hashLookupKey(S), 0xFFFFu); +}