This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][NativePDB] Add support for inlined functions
ClosedPublic

Authored by zequanwu on Jan 7 2022, 4:41 PM.

Details

Summary

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.

Diff Detail

Event Timeline

zequanwu created this revision.Jan 7 2022, 4:41 PM
zequanwu requested review of this revision.Jan 7 2022, 4:41 PM
Herald added a project: Restricted Project. · View Herald Transcript

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
zequanwu updated this revision to Diff 398772.Jan 10 2022, 4:07 PM
zequanwu marked 4 inline comments as done.

Address comments.
Store "temporary" line table in CompileUnitIndex.

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.

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?

zequanwu updated this revision to Diff 399001.Jan 11 2022, 10:01 AM
zequanwu marked an inline comment as done.

Update.

labath accepted this revision.Jan 11 2022, 10:32 AM
This revision is now accepted and ready to land.Jan 11 2022, 10:32 AM
This revision was automatically updated to reflect the committed changes.