This is an archive of the discontinued LLVM Phabricator instance.

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

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



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

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?)


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;

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


I'll clean this up after this patch lands


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

Can we add a docstring?


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.

Michael137 added inline comments.Dec 2 2022, 5:07 AM

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

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


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


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

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

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