RFC: SBValue Metadata Provider

Subprojects LLDB
1

SBValue summaries are a great way to show some condensed info about a variable but summaries are meant to be a relatively short (so the text will fits in a reasonable field in the GUI Local values widget), and human readable, which limits the algorithmic utility of the data they hold. It would be nice to have a more computation friendly API for providing metadata about a SBValue. That’s particularly useful in the interaction between lldb & GUI programs that want to drive lldb.

So I propose adding a parallel structure to the Summaries and SyntheticChild Providers, a “SBValue metadata provider”. It would be bound to SBValues using the same Type lookup mechanism that Summaries and Synthetic Children use.

Since we don’t want lldb to necessarily have to comprehend the data that’s output, we want the result to be self-descriptive. The current way to do something like that in lldb is to return an SBStructuredData which providers can fill out as a dictionary of keys & values.

The next observation is that in this case there might be more than one provider of metadata per type. For instance, you could imagine asking a heap variable for it’s “interesting malloc information” and getting back the history stack for it’s allocation, or maybe even the entire history of the memory it is currently living in. You might also have some type that you can do “sanity checks” on. If the variable fails the sanity check you want the GUI to flag this specially. So you might also want to ask the same variable for its “sanity-check” status, which might just be “bad” and the GUI would draw the value in red or might have “why bad” which the UI could show in a tool tip. And at any given time, you might want the data from one or the other provider.

So I propose the Metadata Providers should also have a “provider name” along with the standard Type matcher. Then the SBValue.GetMetaData call would take a provider, and only return the metadata for the providers that match both the provider name and the Type Matcher.

Note, the addition of providers means that we can’t piggyback this feature on the extant Summary Providers, it needs to be a parallel structure. After all, there might be two types that match the Type matcher. The Summary lookup will return the first matching summary provider, but if only the second Type match is the provider name requested, then we’d have to use the second one for the Metadata.

So then in the end, on the provider writer side, you would produce a Python class that had:

class Provider:    
    def GetMetaData(valobj: SBValue):
        """Returns an SBStructuredData with metadata in key value form"""

   static def GetProviderName():
   static def GetMetaDataSchematic():
      """Returns a SBStructuredData with keys & doc strings"""

The last two calls would make it possible for lldb and the UI to help users understand this data even if it didn’t know about a particular provider. When a provider like this is registered, lldb will get the provider name(s) so we can tell you the legal providers, and the last call would allow us to present definitions for the meanings of the fields.

And on the client side, you would call:

SBStructuredData SBValue::GetMetadata(char *provider_name);

//Returns the global list of providers registered.
static SBStringList SBValue::GetProviderNames();

// This on seems optional to me, but might be useful:
// Returns the list of providers that match the type of the SBValue.
SBStringList SBValue::GetProviderNames();

// Returns a dictionary with the keys as returned from GetMetadata but the value is a definition string for the key.
static SBStructuredData SBValue::GetProviderDescription(char *provider_name);

And on the command line we’d have:

(lldb) type metadata add --classname <PythonClass> <Type Matcher options copied from type summary>

I think it makes sense for the provider class to know its own provider name, but if we don’t want to do it that way, we can do:

(lldb) type metadata add --classname <PythonClass> --providername sanity-check <Type Matcher Options>

and then we’d poke that name into the provider when we registered it.

Then to help users know what is available here:

(lldb) type metadata providers list
    sanity-check
    malloc

(lldb) type  metadata provider show sanity-check
    Goodness - a bool if True the variable passed the sanity-check
    Checker - Tells which of the sanity-checks this variable failed 
    Reason - The detailed string from the failed sanity checker

(lldb) type metadata info --metadata-provider sanity-check <Value Specification>
    sanity-check provider is MyPythonModule.MySanityCheckProvider

this last one is mostly for us, but the equivalent type summary info and type synthetic info have proved very handy to have.

and to see the actual metadata:

(lldb) frame variable -metadata-provider sanity-check
  (int) Foo = 10
      Metadata:
        Goodness: False
        Checker - The one true variable inspector
        Reason: You didn't do it right

and the equivalent for target variable and expr.

We should also consider installing a set of standard do-nothing providers for common operations, like allocation history, sanity-check, related-source-location. That would provide more predictability in these common cases, making it easier to use. We could even check, when a new provider for one of the build-in provider types is registered, that it has the required keys for that provider type.

3 Likes
2

We could use this as well for the “give me the source location for the function in std::function” proposal as well. That would be a “source-code-reference” provider. For instance, to get the source locations for the functions in std::function, you would make a Metadata Provider whose type match is for std::function objects, and whose provider is extra-source-locations. If you wanted there to be just one extra location, the return dictionary would be something like:

{Location: “foo.cpp:12.10”
Description:
}

Then the UI would get the SBValue, and call:

extra_location = my_value.GetMetadata(“extra-source-locations”)

if extra_location was not null the UI would put up a tool tip using the Description to indicate what the extra location was, and if the user clicked on the “Go There” button, you could take them to the source location.

Internally, StructuredData can hold objects as well as strings and booleans. I don’t know how hard it would be to use that to allow values to be SB Objects, but if you could do that, you could even make the location be a pair of SBFileSpec, SBLineEntry so you wouldn’t have to re-look up the file & line.

If you wanted to support multiple “extra source locations” you could easily extend this to have:

{Locations: [{Location: … ; Description:}, …]}

And the UI could put up a picker list with the Descriptions for each element.

3

I like this idea :slight_smile: ! So IIUC, a variable could have extra metadata coming from multiple providers, right ?

On the scripting side, I guess we could come up with a base implementation of the Provider class that the user’s provider class would be derived from (similarly to what we’re doing with Scripted Processes).

Are you expecting all the providers to share the same fields for the metadata to keep it consistent or should the keys be arbitrary fields ?

+1 :slight_smile:

4

What I’m suggesting is a mix of “free form” and “suggested”. So for instance, sanity-check seems like a pretty standard kind of provider, and probably needs the fields I showed in the example. So I was suggesting we have a sanity-check provider built in with the suggested keys, and if you add a sanity-check provider for some type we would ensure that it has the required keys. It could have extra keys, I don’t think we need to restrict that, but if you provide them you are taking the risk that some UI’s might ignore the extra info.

But I think it’s also worthwhile to allow arbitrary providers whose results make sense mostly to the code that accompanies the provider. Presumably you wouldn’t ask for such a provider if you didn’t know what its keys meant, and you wouldn’t ask for random provider’s metadata if you weren’t ready to not know what its keys meant…

5

Thanks a lot for that write-up! I like this idea a lot :slight_smile:

I think general-purpose metadata providers for SBValue have a lot of value. Besides the three use cases already mentioned (malloc annotations, validators, "give me the source location for the function in std::function”), I could think also of:

  • asan annotations: annotate each pointer with it’s status according to address sanitizer. Display if it can be dereferenced or not.
  • for std::mutex/std::atomic/std::condition_variable: a list of all the threads which are currently waiting on it

That being said, I see two short-comings with the current proposal:

For validators: I think that multiple different validators might apply to the same variable, and the overall variable is only valid if it passes all validators. E.g., for a const char16_t* pointer, it should only be marked as “valid”, if (a) the pointer is correctly aligned, (b) the pointer can be dereference according to ASAN, and (c) the memory it points to contains only valid UTF-16 characters.

As such I would have 3 providers (pointer-aligned-validator, asan-pointer-validator, string-encoding-validator). All 3 providers could apply independently to different sets of variables: the pointer-aligned-validator applies for all pointers, the asan-pointer-validator applies to all pointers, but only if running under ASAN, the string-encoding-validator only applies to char8_t and char16_t. But they all provide meta-data which follows the same schema (“validation state”: “error” or “valid”; “validation messages”: arbitrary text messages) and the different providers should probably be combined into one single validation state which is then displayed in the UI.

For the specific “give me the source location for the function in std::function/std::source_location” use case: I would like the mapping configured by target.source-map to also apply to those source locations. If LLDB is not aware that a certain SBStructuredData should represent a source code location, it cannot apply this mapping automatically, and that mapping would need to be done directly inside each of the MetadataProvider implementations.

I don’t see those two issues as blockers, but I wonder if there is a simple way to extend the proposal to cover those two use cases better :thinking:

6

It shouldn’t be hard to support multiple validators for a given provider name, since we can return a SBStructuredData that is an array of SBStructuredData’s. So collecting and returning the results should be straightforward.

One way to do this would be to allow a provider to state that it wants to be a “generate all matches” rather than a “first match wins” by giving itself a “sub-kind” name. So you would do:

(lldb) type metadata add --python-class --providername sanity-checker --sub-kind ThisKind

Then when we see a provider that has subkinds, we’ll know to run all the matching providers of that name, and then produce an array of results:

{[sub-kind : ThisKind; value : {IsGood: True ; Description: “I see no evil”; ExtraKeyForThisKind : “mysterious, huh?”},
sub-kind: AnotherKind : value {IsGood: False ; Description: “How could you have missed the smell”}]

Something like that. That way lldb doesn’t have to get involved in the logic of what it means to get various different results when presenting them.

7

It might even be good in this case to start with some meta information:

{ResultType: Array ; Result: {what I had before}}

So you can easily tell whether you are getting direct result keys or a list of sub-kind matches.

8

How we construct the paths for the extra sources we present is internally tricky, but poses no problems for this design, I think. After all, the actual work of figuring out what to present as the path is done independently of the container we’re presenting them in. If anything, this dictionary approach might make this job easier, since you could present the result in a more complex way if you needed to, like one key for “debug info path” and one for “remapped path” or whatever was needed to find your way to the actual source file.

9

Makes sense to me

Bike shedding: Personally, I would prefer to just always return an array, i.e.:

struct SBMetaData {
    ConstString providerSubKind;
    SBStructuredData metadata;
};
std::vector<SBMetaData> SBValue::GetMetadata(char *provider_name);

This would make the interface simpler/more strictly typed

10

I’m not sure this should get exposed through the SB API since it’s a component that would be specific to SBValue objects. To add to the bike shedding, SBMetadata name is too vague/generic. That’s why I think having a user-provided class makes more sense here.

11

So I like the general idea here.

I think there are some things on the client side we might need to consider.

  1. Metadata providers can be loaded dynamically. With the proposed API clients will have to discover newly loaded providers by calling SBValue::GetProviderNames() multiple times. To ease the burden on clients perhaps we should provide a way to register a callback so that LLDB will tell the client the name of any newly loaded provider?

  2. Whose responsibility is it check returned metadata matches the schema?

It’s not clear if this interface provides a machine readable schema or if it is for human only consumption. If it’s the former LLDB could actually verify the data returned by SBValue::GetMetadata(...) matches the schema. If it’s the latter then its really only for documentation purposes.

I’m not convinced we would want LLDB always checking that metadata returned matches a schema due to potential perf cost (imagine an IDE that calls SBValue::GetMetadata(...) for every in scope variable) and the fact we’d have to implement a schema language and verifier inside LLDB.

  1. The current design provides a very large amount of freedom in how meta should be structured (anything a SBStructuredData can store). This is good because opens up this feature to many use cases, many of which we probably haven’t though of yet. However, to actually make it easy for this feature to work with IDEs there does need to be some kind of common interface agreed between them. I think the responsibility is on LLDB to provide some example interfaces so that IDEs will know what to expect.

I see this is mentioned here

This is great. I just wanted to emphasize it some more because I think its very important.

In particular I think the sanity-check schema might need to be a little different to what is proposed. Goodness isn’t necessary a binary property. It’s potentially a spectrum (e.g. good, might-be-bad, bad) so perhaps this should be an integer called severity?

For example, if there could be a sanity-checker for raw pointers when ASan was enabled in the program. When this metadata provider runs it can look at the pointer and determine if it points at poisoned memory. I would say that’s a high-severity problem if that happens. The pointer could also point at nullptr. If its dereferenced the program will crash, so it being a nullptr “might-be-bad”, but it might also be fine. It’s not possible to know without more context so I would say it’s a low-severity problem, but we can’t say its a zero-severity problem because the provider can’t guarantee that.

  1. We may want to suggest a namespace convention for providers to prevent them from accidentally stepping on each other.
1 Like
12

+1

13

The kinds of things held by an SBStructuredData are given by the values of the SBStructuredDataType. So we certainly could have the GetProviderDescription return the key, description and type.

The check I was suggesting happens when you do type metadata add. If you add a provider that claims to be a “sanity-check” provider for instance, we’ll compare your Description against the template and make sure it matches.

However, I don’t think we need to check again every time a result is provided. I think we can assume enough good will that we don’t need to guard against a provider’s lying about the type it returns. We just need to make sure it is well-formed.

14

My understanding of the proposed interface was rather: a UI knows which providers it can handle (e.g., only the sanity-check and related-source-location). The UI would then only ask for those two providers. If the UI received metadata with a schema which the UI doesn’t understand, it wouldn’t know how to render this metadata anyway.

After the clarification in RFC: SBValue Metadata Provider - #6 by jingham, I now think of the original “provider name” more as the “name for the provided kind of meta data” and the “SubKind” as the actual “provider name”

15

I’m not entirely sure the “Provider added” callback is needed (though I’m not opposed to it) for a couple of reasons.

I would imagine in most cases the UI will have a set of standard providers it knows how to display, and it just going to display them. The special purpose ones would mostly get comprehended by some code that accompanies that provider and runs as commands or breakpoint callbacks, etc to present that data.

I also think it’s unlikely that the UI would have a list of metadata providers somewhere in the UI that you needed to keep up to date. Rather, at some point in your code, you are going to format a variable. At that point, if your intention was to display the data from all the providers, you would just add a call to list all providers then. That would be a tiny portion of the work of getting and presenting the provider data, so I don’t think that would cause a problem.

This also makes the notion of provider kinds and subkinds more interesting. That way we could have a bunch of known types of fairly general purpose providers, and then what would get dynamically added would either subkinds of the known ones, or fairly esoteric ones special purpose ones. The UI doesn’t really need to know that there are 10 subkinds of “sanity-checker”.

16

Yes, I think that’s a better way of talking about them.

17

The direction where this proposal is going strongly reminds me of GraphQL. Would it make sense to borrow some design ideas from them?

18

Did you have something particular in mind?

19

I buy both your arguments. It sounds like we probably don’t need to the callback. I don’t think it would be hard to implement so we can do it later if it turns out someone actually needs it.

20

@jingham

Given that we may want to change a few things. Here’s a sketch

class Provider:    
    def GetMetaData(valobj: SBValue) -> SBStructuredData :
        """Returns an SBStructuredData with metadata in key value form"""

   # FIXME: Should this be some kind of enum instead?
   # FIXME: There should probably be a "allow anything" kind.
   @staticmethod
   def GetProviderKind() -> str:
      """ Returns the "kind" of provider. The "kind" describes the interface the class 
          implements. E.g. "sanity-check"
      """

   @staticmethod
   def GetProviderName() -> str:
      """ Returns the name of this provider that uniquely identifies this implementation.
           Namespace convention TBD.

      E.g. `org.llvm.asan_ptr_sanity_check`
      """
    
   @staticmethod
   def GetMetaDataSchematic() -> SBStructuredData:
      """Returns a SBStructuredData with keys & doc strings"""
  • I’ve added GetProviderKind() that says which interface the provider will use. E.g. sanity-check
  • GetProviderName() now returns a unique name for the particular implementation of the provider interface.

Now given that a provider knows the kind it implements we can simplify the original proposal below

to

(lldb) type metadata add --python-class --providername  org.llvm.asan_ptr_sanity_check <Type Matcher options>

i.e. we don’t need to --sub-kind argument anymore.

I’m not entirely what the purpose of --python-class from the previous proposal is. Presumably the python implementation file would register the provider with LLDB when its loaded. In which case LLDB would probably already know if the provider was written in Python or C++ so I don’t know what value the flag adds.

In my sketch this would become something like…

The client calls something like this on a SBValue

my_value.GetMetadata(“sanity-check”)

and gets back on array of dictionaries. Each dictionary is the response from a provider that matched the type AND the kind requested (in the above example “sanity-check” is the kind).

[
  {
     providerName : "someProvider",
     value : {IsGood: True, Description: “I see no evil”, ExtraKeyForThisKind : “mysterious, huh?”}
  },
  {
    providerName: "anotherProvider",
    value: {IsGood: False, Description: “How could you have missed the smell”},
  },
]

In addition to this I think providers need a way of saying. “Please don’t use me”. For example, an ASan sanity-check provider has no use in a process not using ASan so the provider would ideally detect this and tell LLDB in some way (e.g. `GetMetaData(…) returns None) to not use the provider.