Page MenuHomePhabricator

[lldb/DataFormatters] Display null C++ pointers as nullptr
ClosedPublic

Authored by JDevlieghere on Mar 31 2020, 10:50 AM.

Details

Summary

The original motivation for this patch was to display a null
std::string pointer as nullptr instead of "", but the fix
seemed generic enough to be done for all C++ summary providers.

Diff Detail

Event Timeline

friss created this revision.Mar 31 2020, 10:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2020, 10:50 AM

I haven't tested the libstdc++ part, would be great if someone could confirm this works.

shafik added a subscriber: shafik.Mar 31 2020, 12:05 PM
shafik added inline comments.
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/string/main.cpp
13 ↗(On Diff #253927)

How about string_ptr_null

I haven't tested the libstdc++ part, would be great if someone could confirm this works.

This doesn't work work for libstdc++ (I get <parent failed to evaluate: parent is NULL>), presumably because the code uses the simple StringSummaryFormat (new StringSummaryFormat(stl_summary_flags, "${var._M_dataplus._M_p}")) instead of CXXFunctionSummaryFormat However, it doesn't make the situation any worse either, so you can just drop the libstdc++ test changes and carry on.

It might be worth mentioning though that the title of the patch is somewhat misleading -- I believe the CXX in CXXFunctionSummaryFormat refers to the language the formatter is implemented in, not the language of the type itself. So this patch affects all types whose formatters are implemented as c++ functions, not all c++ pointers in the target program.

friss added a comment.Apr 1 2020, 3:03 PM

It might be worth mentioning though that the title of the patch is somewhat misleading -- I believe the CXX in CXXFunctionSummaryFormat refers to the language the formatter is implemented in, not the language of the type itself. So this patch affects all types whose formatters are implemented as c++ functions, not all c++ pointers in the target program.

Wow, thanks for pointing this out. It should have clicked when I saw ScriptSummaryFormat besides it, but I was indeed completely confused. Let me rework this

friss added a comment.Apr 1 2020, 3:33 PM

I was checking whether there is a way to catch null pointer before a type summary is even chosen. And found out that such logic exists, but only for Objective-C. Languages have a IsNilReference() virtual method that can tell whether a ValueObject is a nil reference. It's only implemented for ObjectiveC and value objects that test positive for this method are displayed as "nil" is the generic code of ValueObjectPrinter.cpp. I can see a few options:

  • I can rework the patch to affect only the libc++ string formatter which was the issue I was trying to solve in the first place
  • I can implement IsNilReference() for C++ and change ValueObjectPrinter to print C++ "nil references" as "nullptr".
  • I do the above and add another customization point in the Language plugins which tells ValueObjectPrinter out to display "nil" references, so that there's no Language specific code in ValueObjectPrinter.

A few additional questions come to mind though:

  • Was the intent if IsNilReference() to be Objective-C specific in the first place?
  • Should we do the same thing for C and NULL?
  • C, C++ and Objective-C(++) being highly intermingled, would something like this even do the right thing in all cases?
labath added a comment.Apr 2 2020, 1:54 AM

I was checking whether there is a way to catch null pointer before a type summary is even chosen. And found out that such logic exists, but only for Objective-C. Languages have a IsNilReference() virtual method that can tell whether a ValueObject is a nil reference. It's only implemented for ObjectiveC and value objects that test positive for this method are displayed as "nil" is the generic code of ValueObjectPrinter.cpp. I can see a few options:

  • I can rework the patch to affect only the libc++ string formatter which was the issue I was trying to solve in the first place
  • I can implement IsNilReference() for C++ and change ValueObjectPrinter to print C++ "nil references" as "nullptr".
  • I do the above and add another customization point in the Language plugins which tells ValueObjectPrinter out to display "nil" references, so that there's no Language specific code in ValueObjectPrinter.

All of those options sound fine to me. Making that generic is obviously preferable, but I don't know how much work that would be.

The thing that I found surprising when looking at this is that std::string has the exact same summary as a std::string * which points to it. I would have expected at least some subtle hint (perhaps by prepending the summary with a hex pointer value?) to indicate that we're looking at a pointer. This would be doable if the generic code had some way to understand pointers (and their nullness).

A few additional questions come to mind though:

  • Was the intent if IsNilReference() to be Objective-C specific in the first place?
  • Should we do the same thing for C and NULL?
  • C, C++ and Objective-C(++) being highly intermingled, would something like this even do the right thing in all cases?

Good questions, and I don't really have an answer for that. I don't think that IsNilReference should be c++-specific. Printing C pointers as NULL would be nice, but i don't know if we have any summaries for C types anyway.

friss updated this revision to Diff 255571.Apr 6 2020, 7:56 PM

Implement null C++ printing at the ValueObjectPrinter level

friss updated this revision to Diff 255579.Apr 6 2020, 9:00 PM

Remove the now useless code in TypeSummary.cpp, and change the C++
implementation of IsNilReference() to return true soemtimes. This
breaks other tests though...

friss added a comment.Apr 6 2020, 9:01 PM

This looks like what I'd like to implement, but unfortunately it breaks other tests. Some C tests start printing null pointers as nullptr too. I suppose this is because the expression evaluator is always in C++ mode. Is there a way to get the origin type/language of a variable through a ValueObject?

ValueObjects know their execution context, and so they can get to their frame, if they have one. The language of the frame seems like the best thing to clue off here.

JDevlieghere commandeered this revision.Nov 10 2020, 11:41 AM
JDevlieghere updated this revision to Diff 304282.
JDevlieghere added a reviewer: friss.
  • Add fallback for finding the language
  • Fix tests

So this work for primitive types like int* etc?

So this work for primitive types like int* etc?

No, Jim probably know the details, but this only applies to composite types.

Fix the C test

JDevlieghere added inline comments.Nov 11 2020, 12:41 AM
lldb/source/DataFormatters/ValueObjectPrinter.cpp
365

If we had a C runtime we could print "NULL", but I don't think it's worth adding that just for this. Another alternative would be to just have a switch based on the LanguageType, but that seems like a pretty bad idea from a layering perspective.

teemperor requested changes to this revision.Nov 11 2020, 1:52 AM
teemperor added a subscriber: teemperor.

A few comments, but otherwise this seems good.

lldb/include/lldb/Target/Language.h
214

/// Returns the summary string that should be used for a ValueObject for which IsNilReference() is true or something like that.

Also I think this probably should have a Get prefix like the other methods in this class.

lldb/source/DataFormatters/ValueObjectPrinter.cpp
365

Not sure what to think about hardcoding 0x0 here. You can specify all kind of formatters that would format the value here differently and this seems kinda inconsistent.

(lldb) expr --format d -- nullptr
(nullptr_t) $2 = 0
(lldb) expr --format b -- nullptr
(nullptr_t) $3 = 0b0000000000000000000000000000000000000000000000000000000000000000

Could we just fall to the default formatter if we don't have a special summary from the plugin?

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
1138

Nit: !Language::LanguageIsCPlusPlus(valobj.GetCompilerType().GetMinimumLanguage()) ? (as we might return a specific C++ standard in the future from GetMinimumLanguage).

This revision now requires changes to proceed.Nov 11 2020, 1:52 AM
JDevlieghere marked 3 inline comments as done.Nov 11 2020, 11:55 AM
jingham accepted this revision.EditedNov 11 2020, 5:36 PM

It seems weird that even if you had a summary formatter for some pointer type that was trying to print "DANGER WILL ROBINSON" when the pointer value was 0x0, we will override that and print "nullptr" in a C++ context or "nil" in an ObjC context. Seems like even if the value is Nil we should first consult the ValueObject's summary value and only do the nullptr/nil computation if it was empty.

But that bad behavior was already present for ObjC objects before this patch, you're just extending it to C++ or anything else that implements IsNilReference. So fixing that doesn't seem required for this patch.

lldb/source/DataFormatters/ValueObjectPrinter.cpp
368

If you wanted to you could check here if the languages hadn't assigned anything to Summary, and default to "NULL"? That would be consistent with our treating C as the fallback language rather than as a separate Language plugin. Not required...

teemperor accepted this revision.Nov 12 2020, 10:43 AM
This revision is now accepted and ready to land.Nov 12 2020, 10:43 AM