This adds inline function support to NativePDB by parsing S_INLINESITE records
to retrieve inlinee line info and add them into line table at ParseLineTable.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I don't know much about PDBs, but the code seems ok. I particularly like the test case.
The one part that bothers me is the "two-phased" setting of line tables. IIUC, the problem is that the inline line info is in the block descriptor, so you need to parse block info in order to create a line table, but in order to actually create an lldb_private::Block, you need the call site info from the caller, which is in the pdb line table. This creates a sort of a dependency loop, is that right?
Although, at the moment, I can't think of anything that would break in this setup, this two-phase design (where you temporarily install an incomplete line table) definitely seems like a code smell, and a possible future maintenance burden. Is there any way around that? AFAICT, the "temporary" line table is accessed only from SymbolFileNativePDB::ParseInlineSite. Could we maybe change that function so that it reads the line table from some temporary location, so that we only install a "real" line table once it is final? The CompileUnit objects have a [GS]etUserData methods which can be used to store opaque pointers. Maybe you could store it there? Or maybe inside the CompileUnitIndex ?
lldb/source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.cpp | ||
---|---|---|
116 | We usually use ec for variables of type std::error_code. This is doubly confusing since you have also used auto (whose usage does not fit the current guidelines -- "use when the type is obvious from the context"). | |
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp | ||
332–390 | Use a switch and put the assert on the default branch? | |
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h | ||
165–166 | Would you be able to use LineTable::Entry out-of-the box if it was made public? I don't see any strong reason for why it would have to remain hidden... | |
275 |
The one part that bothers me is the "two-phased" setting of line tables. IIUC, the problem is that the inline line info is in the block descriptor, so you need to parse block info in order to create a line table, but in order to actually create an lldb_private::Block, you need the call site info from the caller, which is in the pdb line table. This creates a sort of a dependency loop, is that right?
Yes. The temporary line table is in CompileUnitIndex now.
This looks better. Thanks.
lldb/include/lldb/Symbol/LineTable.h | ||
---|---|---|
306–336 | The rest of this can stay protected, right? |
The rest of this can stay protected, right?