This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][NativePDB] Fix PdbAstBuilder::GetParentDeclContext when ICF happens.
ClosedPublic

Authored by zequanwu on Sep 2 2022, 7:03 PM.

Details

Summary

Removed GetParentDeclContextForSymbol as this is not necesssary. We can get
the demangled names from CVSymbol and then using it to create tag decl or
namespace decl. This also fixed a bug when icf applied.

Diff Detail

Event Timeline

zequanwu created this revision.Sep 2 2022, 7:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2022, 7:03 PM
zequanwu requested review of this revision.Sep 2 2022, 7:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2022, 7:03 PM
zequanwu updated this revision to Diff 457752.Sep 2 2022, 7:06 PM

Fix comment in test case.

labath added inline comments.Sep 5 2022, 5:02 AM
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
631

What if the two symbols have the same address (same code) and the same base name (but are e.g. in different namespaces)?

634–661

I know you didn't write this, but I still have to express my surprise at the existence of this code. My don't have anything like that -- using demangled names to look up things -- in the DWARF plugin. There, you can find the containing context by just going "up" in the DIE tree. I don't know if that is possible in PDB, but the current implementation seems fairly fragile, and I suspect it could be broken if the user overrides a mangled name of a symbol (void f() asm("my_linkage_name");).
Are you sure it's not possible to do this in some other way?

zequanwu added inline comments.Sep 6 2022, 10:53 AM
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
634–661

For namespace, I don't see any symbol record for it. All members of namespace, functions or variables, just have namespace string as a prefix for their names, like N1::N2::func.
@rnk Do you know any other ways to get the namespace string of a symbol rather than using mangled names?

zequanwu updated this revision to Diff 458299.Sep 6 2022, 3:43 PM
  • Removed GetParentDeclContextForSymbol as this is not necesssary. We can get the demangled names from CVSymbol and then using it to create tag decl or namespace decl.
  • Add test cases with same base names, but with different namespace/class.
labath accepted this revision.Sep 6 2022, 11:59 PM

Well, I still don't claim to understand this code, but I am happy to stamp anything that fixes bugs by deleting code.

Just as an idea for future work, you may want to check that we're correctly handling functions with unusual linkage names (e.g. due to asm labels). In DWARF, we're adding explicit asm labels with the debug-info-provided linkage names to make this work.

This revision is now accepted and ready to land.Sep 6 2022, 11:59 PM
zequanwu retitled this revision from [LLDB][NativePDB] Fix PdbAstBuilder::GetParentDeclContextForSymbol when ICF happens. to [LLDB][NativePDB] Fix PdbAstBuilder::GetParentDeclContext when ICF happens..Sep 7 2022, 11:50 AM
zequanwu edited the summary of this revision. (Show Details)