Page MenuHomePhabricator

[lldb] Make the ClassTemplateDecl merging logic in TypeSystemClang respect template parameters
ClosedPublic

Authored by teemperor on Apr 16 2021, 9:50 AM.

Details

Summary

DWARF doesn't describe templates itself but only actual template instantiations.
Because of that LLDB has to infer the parameters of the class template declarations
from the actual instantiations when creating the internal Clang AST from debug info

Because there is no dedicated DIE for the class template, LLDB also creates the ClassTemplateDecl
implicitly when parsing a template instantiation. To avoid creating one ClassTemplateDecls for every
instantiation, TypeSystemClang::CreateClassTemplateDecl will check if there is already a
ClassTemplateDecl in the requested DeclContext and will reuse a found fitting declaration.

The logic that checks if a found class template fits to an instantiation is currently just comparing
the name of the template. So right now we map template<typename T> struct S; to
an instantiation with the values S<1, 2, 3> even though they clearly don't belong together.

This causes crashes later on when for example the Itanium mangler's
TemplateArgManglingInfo::needExactType method tries to find fitting the class template parameter
that fits to an instantiation value. In the example above it will try to find the parameter for
the value 2 but will just trigger a boundary check when retrieving the parameter with index 1
from the class template.

There are two ways we can end up with an instantiation that doesn't fit to a class template with
the same name:

  1. We have two TUs with two templates that have the same name and internal linkage.
  2. A forward declared template instantiation is emitted by GCC and Clang without an empty list of parameter values.

This patch makes the check for whether a class template declaration can be reused more sophisticated by
also comparing whether the parameter values can fit to the found class template. If we can't find a fitting
class template we justcreate a second class template with the fitting parameters.

Fixes rdar://76592821

Diff Detail

Event Timeline

teemperor created this revision.Apr 16 2021, 9:50 AM
teemperor requested review of this revision.Apr 16 2021, 9:50 AM
teemperor edited the summary of this revision. (Show Details)
teemperor updated this revision to Diff 338158.Apr 16 2021, 9:52 AM
  • Make a variable name more expressive.
Harbormaster completed remote builds in B99208: Diff 338158.
JDevlieghere added inline comments.Apr 19 2021, 10:52 AM
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
1360

Is "iff" a typo or intentional (if and only if)?

1494

Would there be value in making this an lldb_assert? I.e. is this something we expect to happen?

teemperor added inline comments.Apr 19 2021, 11:44 AM
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
1360

It's the "if and only if` meaning. I guess the confusion comes from not using iff in the function below. WDYT, "if" here or "iff" below?

1494

I think the decision here is the same as in my comment below. It can't happen in the current LLDB from what I can see. But if someone adds LLDB support for other template parameter types and doesn't update this code, then this will crash on the user side (but not otherwise mess with their debug session). If we keep it as an assert then we will just crash for people that have asserts enabled.

1499

I think we could argue that this should be llvm_unreachable, but we have to backport this patch to fix some test failures on the last stable branch, so I want to backport this with an assert(...); return false (mostly because I'm not sure if we don't run into some weird edge case in the Swift->Clang interop code).

I'm open llvm_unreachable or the current version on llvm.org (or maybe landing this with an assert and making it unreachable as a follow-up). @JDevlieghere thoughts?

JDevlieghere added inline comments.Apr 19 2021, 12:15 PM
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
1360

Yep, that was exactly it. I'm fine with iff iff it's consistent.

1494

Then we probably want an lldb assert, so we find out when that happens even if we don't hit it in the tests (which realistically is the only place we have asserts enabled).

1499

Sounds fair. I agree that we'd want an assert on the release branch.

Ping, I would like to get the bots green :)

teemperor updated this revision to Diff 352166.Jun 15 2021, 9:46 AM
  • assert -> lldbassert
kastiglione accepted this revision.Jun 15 2021, 10:04 AM
This revision is now accepted and ready to land.Jun 15 2021, 10:04 AM
This revision was landed with ongoing or failed builds.Jun 15 2021, 10:25 AM
This revision was automatically updated to reflect the committed changes.