This is an archive of the discontinued LLVM Phabricator instance.

[PDB] Emit index/offset pairs for TPI and IPI streams
ClosedPublic

Authored by rnk on Apr 3 2017, 6:34 PM.

Details

Summary

This lets PDB readers lookup type record data by type index in O(log n)
time. It also enables makes cvdump -t work on PDBs produced by LLD.
cvdump will not dump a PDB that doesn't have an index-to-offset table.

The table is sorted by type index, and has an entry every 8KB. Looking
up a type record by index is a binary search of this table, followed by
a scan of at most 8KB.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Apr 3 2017, 6:34 PM
zturner edited edge metadata.Apr 3 2017, 7:49 PM

The 8KB thing is interesting, because sometimes it's not actually every 8KB. I don't know how important it is though, since it should just change the efficiency of the log(N) search. Run llvm-pdbdump analyze on a large PDB and then paste the results into Spreadsheets and compute differences to see what I mean.

Anyway, thanks for doing this. I'll review in more detail tomorrow when I'm back.

zturner added inline comments.Apr 6 2017, 5:22 PM
lld/test/COFF/pdb.test
194 ↗(On Diff #93982)

I would remove all these values, otherwise it's going to be high maintenance as we add more and more stuff to the PDB and stuff shifts around.

355–356 ↗(On Diff #93982)

Was this intentional? Seems like we should be able to be reasonably certain that 0 source files contribute to the Linker-generated module.

396 ↗(On Diff #93982)

Same here and with other numbers that you changed.

500–501 ↗(On Diff #93982)

Intentional?

565 ↗(On Diff #93982)

Was this intentionally deleted?

llvm/lib/DebugInfo/PDB/Native/TpiStream.cpp
100 ↗(On Diff #93982)

s/hash tables/hash values/

llvm/lib/DebugInfo/PDB/Native/TpiStreamBuilder.cpp
52–55 ↗(On Diff #93982)

emplace_back? This way you don't have to use the brace initializer syntax. I think TypeIndex constructor is implicit too, so you might even be able to just write TypeIndexOffsets.emplace_back(codeview::TypeIndex::FirstNonSimpleIndex + TypeRecords.size(), ulittle32_t(TypeRecordBytes));

106–107 ↗(On Diff #93982)

This looks bogus.

llvm/test/DebugInfo/PDB/pdb-yaml-types.test
29–31 ↗(On Diff #93982)

Maybe I just never knew you could do this, but can you put random text in the middle of a test file with no comment marker?

rnk marked 2 inline comments as done.Apr 7 2017, 11:50 AM
rnk added inline comments.
lld/test/COFF/pdb.test
10 ↗(On Diff #93982)

What actually happened with this test is that @ruiu removed the llvm-pdbdump raw FileCheck line in rL291739, and these RAW checks became stale. They were essentially a "golden output" style test that looks at the complete PDB output before I touched it, and that's where all the diffs other than the TypeIndexOffsets diff come from.

I've re-added the RUN line and regenerated the output in this change because I want to look for the TypeIndexOffset goo at the end of the type stream.

Here's what I'm going to do: I'll land this change and update the RAW checks and we can figure out what we want to do about them separately. This change will just add the TypeIndexOffset checks.

llvm/lib/DebugInfo/PDB/Native/TpiStreamBuilder.cpp
52–55 ↗(On Diff #93982)

That doesn't work because TypeIndexOffset is POD. The braced init syntax allows you to make something of that type but you can't construct one by calling the constructor like emplace_back does. At least, that's how I interpret this spew:

[1/4] Building CXX object lib\DebugInfo\PDB\CMakeFiles\LLVMDebugInfoPDB.dir\Native\TpiStreamBuilder.cpp.obj
FAILED: lib/DebugInfo/PDB/CMakeFiles/LLVMDebugInfoPDB.dir/Native/TpiStreamBuilder.cpp.obj
C:\src\llvm-project\build_stage2\bin\clang-cl.exe   /nologo -DGTEST_HAS_RTTI=0 -DLLVM_BUILD_GLOBAL_ISEL -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_DEBUG_POINTER_IMPL="" -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib\DebugInfo\PDB -IC:\src\llvm-project\llvm\lib\DebugInfo\PDB -Iinclude -IC:\src\llvm-project\llvm\include -I"C:\PROGRA~2\MICROS~1.0\DIA SDK\include" /DWIN32 /D_WINDOWS /W3   /Zc:inline /Zc:strictStrings /Oi /Zc:rvalueCast /Brepro -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wcovered-switch-default -Wdelete-non-virtual-dtor -Wstring-conversion /MD /O2 /Ob2   -UNDEBUG  /EHs-c- /GR- /showIncludes /Folib\DebugInfo\PDB\CMakeFiles\LLVMDebugInfoPDB.dir\Native\TpiStreamBuilder.cpp.obj /Fdlib\DebugInfo\PDB\CMakeFiles\LLVMDebugInfoPDB.dir\ -c C:\src\llvm-project\llvm\lib\DebugInfo\PDB\Native\TpiStreamBuilder.cpp
In file included from C:\src\llvm-project\llvm\lib\DebugInfo\PDB\Native\TpiStreamBuilder.cpp:10:
In file included from C:\src\llvm-project\llvm\include\llvm/DebugInfo/PDB/Native/TpiStreamBuilder.h:14:
In file included from C:\src\llvm-project\llvm\include\llvm/DebugInfo/CodeView/TypeRecord.h:13:
In file included from C:\src\llvm-project\llvm\include\llvm/ADT/APSInt.h:18:
In file included from C:\src\llvm-project\llvm\include\llvm/ADT/APInt.h:20:
In file included from C:\src\llvm-project\llvm\include\llvm/Support/MathExtras.h:19:
In file included from C:/PROGRA~2/MICROS~1.0/VC/include\algorithm:6:
In file included from C:/PROGRA~2/MICROS~1.0/VC/include\xmemory:6:
C:/PROGRA~2/MICROS~1.0/VC/include\xmemory0(737,24):  error: no matching constructor for initialization of 'llvm::pdb::TypeIndexOffset'
                ::new ((void *)_Ptr) _Objty(_STD forward<_Types>(_Args)...);
                                     ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:/PROGRA~2/MICROS~1.0/VC/include\xmemory0(857,7):  note: in instantiation of function template specialization 'std::allocator<llvm::pdb::TypeIndexOffset>::construct<llvm::pdb::TypeIndexOffset, llvm::codeview::TypeIndex, llvm::support::detail::packed_endian_specific_integral<unsigned int, llvm::support::endianness::little, 1> >' requested here
                _Al.construct(_Ptr, _STD forward<_Types>(_Args)...);
                    ^
C:/PROGRA~2/MICROS~1.0/VC/include\xmemory0(995,14):  note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<llvm::pdb::TypeIndexOffset> >::construct<llvm::pdb::TypeIndexOffset, llvm::codeview::TypeIndex, llvm::support::detail::packed_endian_specific_integral<unsigned int, llvm::support::endianness::little, 1> >' requested here
                _Mytraits::construct(*this, _Ptr,
                           ^
C:/PROGRA~2/MICROS~1.0/VC/include\vector(929,18):  note: in instantiation of function template specialization 'std::_Wrap_alloc<std::allocator<llvm::pdb::TypeIndexOffset> >::construct<llvm::pdb::TypeIndexOffset, llvm::codeview::TypeIndex, llvm::support::detail::packed_endian_specific_integral<unsigned int, llvm::support::endianness::little, 1> >' requested here
                this->_Getal().construct(_Unfancy(this->_Mylast()),
                               ^
C:\src\llvm-project\llvm\lib\DebugInfo\PDB\Native\TpiStreamBuilder.cpp(52,22):  note: in instantiation of function template specialization 'std::vector<llvm::pdb::TypeIndexOffset, std::allocator<llvm::pdb::TypeIndexOffset> >::emplace_back<llvm::codeview::TypeIndex, llvm::support::detail::packed_endian_specific_integral<unsigned int, llvm::support::endianness::little, 1> >' requested here
    TypeIndexOffsets.emplace_back(
                     ^
C:\src\llvm-project\llvm\include\llvm/DebugInfo/PDB/Native/RawTypes.h(78,8):  note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 2 were provided
struct TypeIndexOffset {
       ^
C:\src\llvm-project\llvm\include\llvm/DebugInfo/PDB/Native/RawTypes.h(78,8):  note: candidate constructor (the implicit move constructor) not viable: requires 1 argument, but 2 were provided
C:\src\llvm-project\llvm\include\llvm/DebugInfo/PDB/Native/RawTypes.h(78,8):  note: candidate constructor (the implicit default constructor) not viable: requires 0 arguments, but 2 were provided
llvm/test/DebugInfo/PDB/pdb-yaml-types.test
29–31 ↗(On Diff #93982)

Yeah, .test isn't really a file type, it's just an extension that identifies a text file that should get run by lit. lit looks for RUN:, and FileCheck looks for CHECK:, and if you don't use the test as a source file, you don't need comment markers. I could get rid of ; before RUN, I guess.

rnk updated this revision to Diff 94561.Apr 7 2017, 1:13 PM

comments

zturner accepted this revision.Apr 11 2017, 9:07 AM
This revision is now accepted and ready to land.Apr 11 2017, 9:07 AM
This revision was automatically updated to reflect the committed changes.