Page MenuHomePhabricator

[lldb] Desugar template specializations
ClosedPublic

Authored by jarin on Jul 15 2020, 12:48 AM.

Details

Summary

Template specializations are not handled in many of the
TypeSystemClang methods. For example, GetNumChildren does not handle
the TemplateSpecialization type class, so template specializations
always look like empty objects.

This patch just desugars template specialization in the existing
RemoveWrappingTypes desugaring helper.

Diff Detail

Event Timeline

jarin created this revision.Jul 15 2020, 12:48 AM
teemperor accepted this revision.Jul 15 2020, 2:51 AM

This could cause that RemoveWrappingTypes goes into an infinite loop under some situations. Usually this function is reserved for types that are always 'sugar', but TemplateSpecializationTypes are not always sugar (e.g., dependent types are not sugar). And for types that are not sugar, getLocallyUnqualifiedSingleStepDesugaredType will return the type that was passed in. So that will make the loop in that function just keep spinning forever.

However I'm not sure though if there is actually a way to get a dependent type into that logic with the normal LLDB APIs. Also Decltype is already suffering from the same problem so I don't think this patch should be blocked over this.

So beside some minor nits in the inline comments this LGTM, thanks!

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
2506

Nit: this is supposed to be in alphabetical order.

lldb/test/API/lang/cpp/template-specialization/TestTemplateSpecialization.py
1 ↗(On Diff #278097)

I think this test should be just called template-specialization-type as it's mainly testing that we do the right thing this this specific type.

25 ↗(On Diff #278097)

Maybe add a comment that this turns the RecordType into a TemplateSpecializationType.

This revision is now accepted and ready to land.Jul 15 2020, 2:51 AM
jarin updated this revision to Diff 278173.Jul 15 2020, 7:05 AM
jarin marked 3 inline comments as done.

Addressed review comments.

jarin added a comment.EditedJul 15 2020, 7:09 AM

This could cause that RemoveWrappingTypes goes into an infinite loop under some situations. Usually this function is reserved for types that are always 'sugar', but TemplateSpecializationTypes are not always sugar (e.g., dependent types are not sugar). And for types that are not sugar, getLocallyUnqualifiedSingleStepDesugaredType will return the type that was passed in. So that will make the loop in that function just keep spinning forever.

That sounds some what worrisome indeed. I changed the code to special case the TemplateSpecialization case to return if the type cannot be desugared. There are two other options:

  1. Special case TemplateSpecialization to always return type->getLocallyUnqualifiedSingleStepDesugaredType().
  2. No special case, but always early return if the type was unchanged. I am imagining something like
case clang::Type::TemplateSpecialization:
case clang::Type::TypeOf:
case clang::Type::TypeOfExpr: {
  auto original = type;
  type = type->getLocallyUnqualifiedSingleStepDesugaredType();
  if (type == original)
    return type;
}

However I'm not sure though if there is actually a way to get a dependent type into that logic with the normal LLDB APIs. Also Decltype is already suffering from the same problem so I don't think this patch should be blocked over this.

So beside some minor nits in the inline comments this LGTM, thanks!

teemperor accepted this revision.Jul 15 2020, 7:14 AM

I think your comment got broken by Phabricator. I actually think the old code was fine. It's a generic bug that also applies to Decltype (which is already implemented), so someone just has to fix the whole function in a follow up patch (your current code just fixes the TemplateSpecializationType but not Decltype). I can do that, but finding a good testing strategy for this is a bit of a pain (it's a lot of boilerplate to make a dependent type by hand and not sure if we have a good way to reach this from a normal LLDB API).

Just revert the RemoveWrappingTypes to the old implementation and then this is ready to go IMHO.

jarin updated this revision to Diff 278200.Jul 15 2020, 8:29 AM

Undo the infinite loop detection.

This revision was automatically updated to reflect the committed changes.