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.

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

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

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

396

Same here and with other numbers that you changed.

500–501

Intentional?

565

Was this intentionally deleted?

llvm/lib/DebugInfo/PDB/Native/TpiStream.cpp
100

s/hash tables/hash values/

llvm/lib/DebugInfo/PDB/Native/TpiStreamBuilder.cpp
52–55

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

This looks bogus.

llvm/test/DebugInfo/PDB/pdb-yaml-types.test
29–31

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

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

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

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.