Page MenuHomePhabricator

[lldb] char summary provider

Authored by evgeny777 on Oct 12 2015, 8:40 AM.



This patch enables type summary for 'char' type. Let's suppose we have:

char c = 'h';

Current revision evaluates c as:

(char) $0 = 'h'

After this patch (104 is the ASCII code of 'h'):

(char) $0 = 104 'h'

This change primary objective is to conform MI evaluation of character type, so that MI formatting
can be eliminated and entirely replaced with one of lldb. Also this can be useful when evaluating non-printable

Diff Detail


Event Timeline

evgeny777 updated this revision to Diff 37119.Oct 12 2015, 8:40 AM
evgeny777 retitled this revision from to [lldb] char summary provider.
evgeny777 updated this object.
evgeny777 added a reviewer: granata.enrico.
evgeny777 added subscribers: lldb-commits, KLapshin.
granata.enrico requested changes to this revision.Oct 12 2015, 10:34 AM
granata.enrico edited edge metadata.

Truth be told, I find this notation (numeric value + printable character) to be heavy and somewhat redundant

I would be happier with a model where printable characters print as the character, and non-printable ones print as the numeric value

With that said, if the MI really needs to do this for whatever reason, a solution would be for the MI to create its own formatters category and insert this formatter there. Since the char formatter is in the 'system' category, any other category you create should have higher priority and thus win the match and get to be the formatter of choice.
I am not sure whether the SB API allows CXXFormatterFunctions to be created, but this would be easy to add, so please feel free to do so and submit that change for review. I will gladly look at it!

This revision now requires changes to proceed.Oct 12 2015, 10:34 AM

Ok, I see
MI historically has this code + value printing and test cases covering it.
By the way char16_t and char32_t summary providers also print code and value, in case you don't know.

The consistency argument is not entirely unfair.

I would claim that Unicode is a more complex beast than plain ASCII, so it makes sense to go the extra mile in giving you details in that case, compared to the simpler char case.

If the only reason you're pursuing the patch is to appease the test case, I say "no" to the change.
If you think this is actually worthwhile on its own merits, I would rather much have either a separate "MI" formatters category.

I thought this would be a nice feature, as I'm long term gdb user and gdb also evaluates chars as code and value.
MI private category sounds good, but I have concerns on implementing it properly. For instance SBTypeSummary can create string, function and script summaries only. Although string summary ${var%d} ${var} can be used for this case, it's much better to be able to use something like AddCXXSummary(), so that my own C++ formatter function is called. Do you know if this is possible?

Currently I don't think SBTypeSummary allows for defining formatters backed by a CXXFunctionSummaryFormat
It would, however, be a great addition to our API. Is it a change you're interested in working on?

If so, the easy part is going to be introducing LLVM-style RTTI for TypeSummaryImpl such that one can replace the ->IsScripted() calls with a proper switch over all kinds of summaries (there's three, you're gonna want to introduce a fourth - read below).
The slightly more interesting part is that currently CXXFunctionSummaryFormat expects to transact in ValueObjects and std::strings. These are things we cannot expose at the SB layer. So we might need to add yet another kind of summary that instead takes an SBValue, an SBSummaryOptions and fills in an SBStream. Once we have that, the MI could actually use the functionality to register its own summaries (and if we find SBValue to be lackluster in any way as a result, we should fill in those gaps).

Yes, I'm interested.

Imagine I have:

SBTypeSummary::CreateCxxFunctionSummary( ... )

How am I supposed to pass the callback there? Or this should be done by means of introducing some base class, like SBTypeSummaryFormatter and deriving custom formatters from this class?

We are not supposed to be inheriting from SB classes, much less introduce virtual-ness to them (Greg can go in detail about the reasons, the tl;dr is that it has the potential to be ABI-breaking)

The way one would do that is to have a typedef akin to (and after fixing up any syntax errors and choosing a nice name)

typedef bool (*SBSummaryFormatterCallback)(SBValue, SBTypeSummaryOptions, SBStream&)

Unfortunately it has to be a C-style function pointer, and not an std::function, again because the SB API can't use the STL

Then you would have a creating function of the sort of:

SBTypeSummary::CreateCxxFunctionSummary(const char* name, const char* description, SBSummaryFormatterCallback callback)
(CxxFunctionSummary is definitely not great as a name, but I have no better suggestion right now - let's worry about that once we have some code up and running)

You might need to make yourself a subclass of TypeSummaryImpl in the .cpp file to store these formatters. It will essentially be very similar to CXXFunctionSummaryFormat except it would end up using the SBSummaryFormatterCallback typedef - and also it has to live in the SBTypeSummary.cpp because we can't put things that reference the SB API in lldb_private. Notice that, if you're so inclined, your helper class *can* actually use an std::function, and have virtual functions, and all such niceties, because it is an implementation-only detail not exposed to API clients

evgeny777 updated this revision to Diff 37339.Oct 14 2015, 5:44 AM
evgeny777 edited edge metadata.

Looks like can be done much easier

Admittedly way simpler than my original idea. +1

Having the RTTI support so that these SBTypeSummary objects can actually be used for anything other than mere creation would be nice.
However, I can fill that gap myself later.

155 ↗(On Diff #37339)

Should we check for cb != null here?

157 ↗(On Diff #37339)

I assume you are essentially relying on the SBValue constructor that takes a ValueObjectSP here, right?
And similarly for the SummaryOptions?

162 ↗(On Diff #37339)

Any reason not to let people submit their own name/description for the summary formatter here?

One question: CreateWithCallback and CreateWithSummaryString do not require Python support and can be used with LLDB_DISABLE_PYTHON. May be it makes sense to remove conditional compilation, like it is done here:

155 ↗(On Diff #37339)

May be assert(cb) ?

157 ↗(On Diff #37339)

You're right - implicit construction here

162 ↗(On Diff #37339)

Probably it makes sense adding extra parameter to CreateWithCallback()

155 ↗(On Diff #37339)

No, I would rather much us create an hollow SBTypeSummary (with a TypeSummaryImplSP that points to nullptr). Then you would get an invalid one (IsValid() == false) but not cause a crash

157 ↗(On Diff #37339)

Sorry to nitpick, but is there any advantage to not using explicit construction here?

evgeny777 added inline comments.Oct 14 2015, 12:25 PM
157 ↗(On Diff #37339)

None, except more compact code. Would like to use explicit construction here?

So, if you do the explicit constructor change and handle the case of a nullptr Callback I think it should be good to go. Looking forward to it!

157 ↗(On Diff #37339)

Yes, I would prefer that

It saves us a few lines of code, but it is confusing to read, and I want to make sure we don't break sometime in the future due to changes in the constructor (the ones taking SPs are technically private to us)

evgeny777 updated this revision to Diff 37487.Oct 15 2015, 8:10 AM
evgeny777 edited edge metadata.

Updated: callback is checked for null + explicit construction

granata.enrico accepted this revision.Oct 15 2015, 10:41 AM
granata.enrico edited edge metadata.

Fine to get it in

This revision is now accepted and ready to land.Oct 15 2015, 10:41 AM
Closed by commit rL251080: Summary provider for char. (authored by dperchik). · Explain WhyOct 22 2015, 5:05 PM
This revision was automatically updated to reflect the committed changes.
spyffe added a subscriber: spyffe.Oct 22 2015, 5:40 PM

This patch failed to compile. I fixed it with r251083.