[lldb] char summary provider

[evgeny777]

Summary

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
characters

[granata.enrico]

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!

[evgeny777]

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.

[granata.enrico]

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.

[evgeny777]

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?

[granata.enrico]

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).

[evgeny777]

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?

[granata.enrico]

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]

Looks like can be done much easier

[granata.enrico]

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.

source/API/SBTypeSummary.cpp

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?

[evgeny777]

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:

http://reviews.llvm.org/D13577

source/API/SBTypeSummary.cpp

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()

[granata.enrico]

source/API/SBTypeSummary.cpp

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]

source/API/SBTypeSummary.cpp

157 ↗

(On Diff #37339)

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

[granata.enrico]

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!

source/API/SBTypeSummary.cpp

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: callback is checked for null + explicit construction

[granata.enrico]

Fine to get it in

[spyffe]

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

Sean

lldb/trunk/include/lldb/API/SBTypeSummary.h

private:
         std::unique_ptr<lldb_private::TypeSummaryOptions> m_opaque_ap;
     };
     
     class SBTypeSummary
     {
     public:
         
         SBTypeSummary();
-        
+        // Native function summary formatter callback
+        typedef bool (*FormatCallback) (SBValue, SBTypeSummaryOptions, SBStream&);
+        
         static SBTypeSummary
         CreateWithSummaryString (const char* data,
                                  uint32_t options = 0); // see lldb::eTypeOption values
         
         static SBTypeSummary
         CreateWithFunctionName (const char* data,
                                 uint32_t options = 0); // see lldb::eTypeOption values
         
         static SBTypeSummary
         CreateWithScriptCode (const char* data,
                               uint32_t options = 0); // see lldb::eTypeOption values
-        
+        static SBTypeSummary
+        CreateWithCallback (FormatCallback cb, 
+                            uint32_t options = 0);
+        
         SBTypeSummary (const lldb::SBTypeSummary &rhs);
         
         ~SBTypeSummary ();
         
         bool
         IsValid() const;
         
         bool

lldb/trunk/source/API/SBTypeSummary.cpp

 SBTypeSummary::CreateWithScriptCode (const char* data, uint32_t options)
 {
     if (!data || data[0] == 0)
         return SBTypeSummary();
     
     return SBTypeSummary(TypeSummaryImplSP(new ScriptSummaryFormat(options, "", data)));
 }
 
+SBTypeSummary
+SBTypeSummary::CreateWithCallback (FormatCallback cb, uint32_t options)
+{
+    return SBTypeSummary(
+               TypeSummaryImplSP(
+                   cb ? new CXXFunctionSummaryFormat(options,
+                       [cb] (ValueObject& valobj, Stream& stm, const TypeSummaryOptions& opt) -> bool {
+                            BStream stream;
+                            if (!cb(SBValue(valobj.GetSP()), SBTypeSummaryOptions(&opt), stream))
+                                return false;
+                            stm.Write(stream.GetData(), stream.GetSize());
+                            return true;
+                       },
+                       "SBTypeSummary formatter callback"
+                   ) : nullptr
+                )
+            );
+}
+
 SBTypeSummary::SBTypeSummary (const lldb::SBTypeSummary &rhs) :
 m_opaque_sp(rhs.m_opaque_sp)
 {
 }
 
 SBTypeSummary::~SBTypeSummary ()
 {
 }