Page MenuHomePhabricator

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

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

Details

Reviewers
jingham
labath
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.