This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Fix libstdc++ 11's std::unique_ptr affecting LLDB testsuite TestDataFormatterStdUniquePtr.py
ClosedPublic

Authored by jankratochvil on Jun 15 2021, 12:12 AM.

Details

Summary

This is a part of D101237 to fix LLDB testsuite with GCC 11 installed (and its libstdc++). Any non-trivial handling of std::unique_ptr still asserts LLDB, that is the rest of D101237.

Diff Detail

Event Timeline

jankratochvil created this revision.Jun 15 2021, 12:12 AM
jankratochvil requested review of this revision.Jun 15 2021, 12:12 AM
teemperor requested changes to this revision.Jun 15 2021, 12:44 AM

I think the way the provider is supposed to work is that there is always deleter child as long as it's not default_delete<T>, so I think we have to check for the name to avoid that we also hide an empty user-specified deleter.

ValueObjectSP del_obj = tuple_frontend->GetChildAtIndex(1);
if (del_obj) {
  ConstString del_name = del_obj->GetDisplayTypeName();
  if (!del_name.GetStringRef().startswith("std::default_delete<"))
    m_del_obj = del_obj->Clone(ConstString("deleter")).get();
}

(Technically that would hide the deleter if the user specifies a default-deleter that just happens to be compatible, but that seems like an obscure corner case so this seems "good enough").

This revision now requires changes to proceed.Jun 15 2021, 12:44 AM
jankratochvil abandoned this revision.Jun 15 2021, 12:51 AM

So I think you can check-in your patch. I would just copy-paste it.

(Technically that would hide the deleter if the user specifies a default-deleter that just happens to be compatible, but that seems like an obscure corner case so this seems "good enough").

Personally I think if there is the pointer (and sizeof(std::unique_ptr<XXX, YYY> == 16) there should be the "deleter =" specified even if it is default deleter. So I rather disagree with your patch. But whetever, it would be nice to finally fix it for the last stable Fedora release.

jankratochvil reclaimed this revision.Jun 15 2021, 1:33 AM
This revision now requires changes to proceed.Jun 15 2021, 1:33 AM
jankratochvil requested review of this revision.Jun 15 2021, 1:33 AM
teemperor accepted this revision.Jun 15 2021, 2:13 AM

Personally I think if there is the pointer (and sizeof(std::unique_ptr<XXX, YYY> == 16) there should be the "deleter =" specified even if it is default deleter.

I don't think that it can happen that sizeof(ptr) == 16 with a reasonable unique_ptr/default_delete implementation, but I think in theory it could be possible.

But anyway, after some offline discussion with @jankratochvil the consensus was:

  • We should hide the deleter if it's stateless (default_delete or a user-specified deleter that is empty) as the less verbose output is more useful for command line users.
  • Users that care about the (user-specified) empty deleter (or the default deleter) can always inspect the type via the template argument. That anyway has to happen via the SB API and the ValueObject doesn't provide a great benefit there.

So this patch LGTM, but please update the update comment when landing to something like:

// Add a 'deleter' child if there was a non-empty deleter type specified.
// 
// The object might have size=1 in the TypeSystem but occupies no dedicated storage due
// to no_unique_address, so infer the actual size from the total size of the unique_ptr class.
// If sizeof(unique_ptr) == sizeof(void*) then the deleter is empty and should be hidden.
This revision is now accepted and ready to land.Jun 15 2021, 2:13 AM
  • We should hide the deleter if it's stateless (default_delete or a user-specified deleter that is empty) as the less verbose output is more useful for command line users.

My point of view is to rather more closely show what is physically stored in the unique_ptr.

This revision was landed with ongoing or failed builds.Jun 15 2021, 2:21 AM
This revision was automatically updated to reflect the committed changes.