[PATCH] D61277: [PDB] Fix hash function used to write /src/headerblock
Nico Weber via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 29 13:08:17 PDT 2019
thakis created this revision.
thakis added a reviewer: zturner.
Herald added a subscriber: hiraditya.
Herald added a project: LLVM.
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.
https://reviews.llvm.org/D61277
Files:
llvm/include/llvm/DebugInfo/PDB/Native/HashTable.h
llvm/lib/DebugInfo/PDB/Native/NamedStreamMap.cpp
llvm/lib/DebugInfo/PDB/Native/PDBStringTableBuilder.cpp
llvm/unittests/DebugInfo/PDB/StringTableBuilderTest.cpp
Index: llvm/unittests/DebugInfo/PDB/StringTableBuilderTest.cpp
===================================================================
--- llvm/unittests/DebugInfo/PDB/StringTableBuilderTest.cpp
+++ llvm/unittests/DebugInfo/PDB/StringTableBuilderTest.cpp
@@ -19,11 +19,7 @@
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 @@
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);
+}
Index: llvm/lib/DebugInfo/PDB/Native/PDBStringTableBuilder.cpp
===================================================================
--- llvm/lib/DebugInfo/PDB/Native/PDBStringTableBuilder.cpp
+++ llvm/lib/DebugInfo/PDB/Native/PDBStringTableBuilder.cpp
@@ -26,7 +26,13 @@
: 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<uint16_t>(Table->getIdForString(S));
}
StringRef StringTableHashTraits::storageKeyToLookupKey(uint32_t Offset) const {
Index: llvm/lib/DebugInfo/PDB/Native/NamedStreamMap.cpp
===================================================================
--- llvm/lib/DebugInfo/PDB/Native/NamedStreamMap.cpp
+++ llvm/lib/DebugInfo/PDB/Native/NamedStreamMap.cpp
@@ -34,6 +34,7 @@
// 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<uint16_t>(hashStringV1(S));
}
Index: llvm/include/llvm/DebugInfo/PDB/Native/HashTable.h
===================================================================
--- llvm/include/llvm/DebugInfo/PDB/Native/HashTable.h
+++ llvm/include/llvm/DebugInfo/PDB/Native/HashTable.h
@@ -143,7 +143,7 @@
return EC;
if (Present.intersects(Deleted))
return make_error<RawError>(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))
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D61277.197173.patch
Type: text/x-patch
Size: 3279 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190429/69378ecf/attachment.bin>
More information about the llvm-commits
mailing list