This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Fix simple template names interaction with debug info declarations
ClosedPublic

Authored by aeubanks on Nov 28 2022, 10:17 AM.

Details

Summary

Without checking template parameters, we would sometimes lookup the
wrong type definition for a type declaration because different
instantiations of the same template class had the same debug info name.

The added GetForwardDeclarationDIETemplateParams() shouldn't need a
cache because we'll cache the results of the declaration -> definition
lookup anyway. (DWARFASTParserClang::ParseStructureLikeDIE()
is_forward_declaration branch)

Diff Detail

Event Timeline

aeubanks created this revision.Nov 28 2022, 10:17 AM
Herald added a project: Restricted Project. · View Herald Transcript
aeubanks requested review of this revision.Nov 28 2022, 10:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 10:17 AM
aeubanks added a comment.EditedNov 28 2022, 10:18 AM

depends on D138612

dblaikie added inline comments.Nov 28 2022, 10:49 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
747–749

Could invert this condition and use an early return ConstString(); to reduce indentation - but I guess this looks more similar to the definition handling case and so might be worth keeping in the same shape. (maybe ParseTemplateParameterInfos should return true only if the result is !empty or has packed args, so that test doesn't need to be repeated at multiple callers?)

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3068–3078

might be worth flipping to reduce indentation:

auto i = test_base_name.find('<');
if (i == llvm::StringRef::npos)
  return true;
llvm::StringRef test_template_params = ...
if (test_template_params != template_params.GetStringRef())
  return true;
lldb/test/API/lang/cpp/unique-types3/main.cpp
2–15

maybe good to simplify this a bit - rather than using big/complex templates like std::atomic and std::vector, instead using test-only, simpler templates to keep the test a bit more focussed (less likely to fail for unrelated reasons/other regressions)?

aeubanks updated this revision to Diff 478323.Nov 28 2022, 12:20 PM
aeubanks marked an inline comment as done.

don't use stl in test
simplify some code a bit

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
747–749

I'll clean this up after this patch lands

lldb/test/API/lang/cpp/unique-types3/main.cpp
2–15

hmm I swear I tried that and it ended up passing, although that might have been before I figured out the Makefile env var vs normal var issue. done

dblaikie accepted this revision.Dec 1 2022, 3:34 PM

I think this is good - welcome to wait for a second opinion if you prefer, or folks can provide feedback post-commit.

This revision is now accepted and ready to land.Dec 1 2022, 3:34 PM
Michael137 added inline comments.Dec 2 2022, 2:47 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
91

Can we add a docstring?

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2988

So after this call we'll have an unnamed ClassTemplateDecl hanging off the AST? Doing this inside FindDefinitionTypeForDWARFDeclContext instead of ParseStructureLikeDIE feels a bit off. But will have to understand this change a bit deeper to know whether there is something we can do about that.

As a side-note, we seem to be accumulating a lot of blocks around LLDB that look like:

// Do X because -gsimple-template-names
if (name.contains('<')) {
   // Do something
}

Still wonder if there isn't some flag that can be set on, e.g., a CU, that would tell us this.

3063
Michael137 added inline comments.Dec 2 2022, 5:07 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
765

Ok so what we're doing is:

  1. Create a ClassTemplateSpecializationDecl with an empty basename
  2. Return the type-name and since the basename is empty we end up with just the template arguments

Why can't we just call GetTemplateParametersString(die) instead of creating this function?

Michael137 added inline comments.Dec 2 2022, 5:14 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
765

Can confirm that this works locally. Though required moving that function out of the DWARFASTParserClang, which seems OK

aeubanks updated this revision to Diff 479759.Dec 2 2022, 3:21 PM
aeubanks marked 2 inline comments as done.

address comments

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
765

GetTemplateParametersString is specifically only used for GetCPlusPlusQualifiedName, which is used below

// For C++, we rely solely upon the one definition rule that says
// only one thing can exist at a given decl context. We ignore the
// file and line that things are declared on.
std::string qualified_name = GetCPlusPlusQualifiedName(die);

so it doesn't have to match clang's printing. but for the simple template name stuff, we are comparing clang-generated names, so everything needs to go through clang.

I've replaced GetTemplateParametersString with this code that goes through clang as well

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2988

the effect is not really per-CU but rather per-DIE because in some cases (where DWARF roundtripping is lossy, @dblaikie would know best) we still put template params in the name. so it needs to be checked per-DIE

I agree that it's weird to create clang AST nodes here and then forget about them, but clang uses a bump allocator and we can't easily delete it. As long as we call this at most once or twice per DIE, I don't think it's worth caching the results, but if we do end up using this more, we do need to cache the created AST nodes. I've added a comment to the docstring.

Michael137 added inline comments.Dec 3 2022, 8:20 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
765

But didn't GetTemplateParametersString go through Clang's type printer too? Probably missing something.
What would be an example of the difference in output that could arise between the new vs. old method?

aeubanks added inline comments.Dec 6 2022, 9:53 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
765

one thing that I was aware of even initially implementing GetTemplateParametersString was spacing between two >, clang will add a space (unless you tell it not to), whereas the naive implementation didn't