This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Dataformatter] Add support to CF{Dictionary,Set}Ref types
ClosedPublic

Authored by mib on May 6 2020, 10:35 PM.

Details

Summary

This patch improves data formatting for CFDictionaryRef and CFSetRef.
It uses the same data-formatter as NSCFDictionaries and NSCFSets introduced
previously but did require some adjustments in Core::ValueObject.

Since the "Ref" types are opaque pointers to the actual CF containers, if the
value object has a synthetic value, lldb will use the opaque pointer's pointee
type to create the new ValueObjectChild needed to dereference the ValueObject.
This allows the "Ref" types to behaves the same as CF containers when used with
the frame variable command, the SBAPI or in Xcode's variable inspector.

rdar://53104287

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

mib created this revision.May 6 2020, 10:35 PM
friss added inline comments.May 6 2020, 11:26 PM
lldb/source/Core/ValueObject.cpp
52

You don't want to introduce a dependency between Core and a plugin. What you need here might need to be introduced as a new abstract functionality on the TypeSystem base class.

2837

I am understanding correctly that this condition will only trigger when there's a synthetic child provider for the type? In other words, if we have an opaque pointer that we don't know how to display, it will still cause an error, right?

lldb/test/API/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSContainer.py
59–60

I must be missing something obvious, but it seems like that patch doesn't register a formatter for CFDictionaryRef. Was it already there but non-functional?

labath added a subscriber: labath.

The ValueObject change looks very worrying to me. The ability to pretty-print incomplete types seems pretty useful, but the implementation of that looks a bit... scary. I mean you're not even checking whether the incomplete type is a struct or an enum. And the effect of forcefully creating a empty struct has impact outside of the pretty-printer. Maybe it would be better if we thought the valueobject/pretty-printer machinery to work with incomplete types?

Raphael, what do you make of all of this?

Also would it be possible to test the ValueObject functionality on a c++ test (an incomplete c++ type with a custom pretty printer), so that test can run every where?

I don't really know that ValueObject code, but I wish we could just pretend that the dereferenced type of X is a type named Y or something like that and then just map *Ref classes to their non-opaque bridged types when dereferenced. No idea if that's possible though.

Anyway, if we go with the fake empty struct approach then I wish we could just use the (fixed?) list of these special bridged structs and hard-code them into some ObjC plugin or something like that. If those few selected classes are manually completed as empty structs when we find them in the debug info then I think that's a manageable workaround to get this running.

lldb/source/Core/ValueObject.cpp
663

This change (and the one below) don't seem to be some NFC refactoring? Not sure why this was refactored as compiler_type is only ever used once where we previously called GetCompilerType()?

2844

This is the name of the typedef, not the underlying incomplete struct. So this creates a new empty record with the name of the typedef which seems weird. If we fake that some type is actually empty instead of incomplete, then I think the underlying struct makes more sense to emulate. Also this variable name is kinda weird with it's g____ prefix even though it's not a global or a static.

lldb/test/API/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSContainer.py
58

Trailing spaces (that Phabricator somehow doesn't properly highlight :/)

mib marked 3 inline comments as done.May 7 2020, 8:35 AM
mib added inline comments.
lldb/source/Core/ValueObject.cpp
663

This is a remaining from a previous change that I ended up not removing. I'll change it back the way it was.

lldb/test/API/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSContainer.py
59–60

The string summary worked but not the synthetic child provider.

shafik added a subscriber: shafik.EditedMay 7 2020, 9:06 AM

I have to agree w/ Pavel here that I am not crazy about creating creating an empty struct here, it is not clear why there is no other way.

mib marked 4 inline comments as done.May 7 2020, 11:07 AM

The way the ValueObject code works w.r.t. Synthetic child providers is that you first look up and make a ValueObject for the actual value the user requested, (for instance a ValueObjectVariable or a ValueObjectChild or a ValueObjectConstResult for expressions, etc.), then you hand that to the printer. The printer will look at whether the current settings prefer Synthetic Values, and if they do they ask the actual value if it has any Synthetic children. So when you do something like:

(lldb) frame var *opaque_ptr

we first have to make a ValueObject for *opaque_ptr (in this case the dereference is a ValueObjectChild of the ValueObjectVariable representing the variable opaque_ptr and return that to the printer.

But you can't currently make a ValueObject that doesn't have a type, and the problem here is that we don't have a type for * of the opaque pointer. Making an empty struct definition and returning a ValueObject with that struct definition is one fairly straightforward way of doing that. And we already inject custom types in other places to manage synthetic children (look for __lldb_autogen_nspair for instance). So this seemed the most straightforward choice.

Note, you could also make a new ValueObject subclass for these purposes: ValueObjectOpaqueDereference to forward the Synthetic Children. The ValueObject base class doesn't require a type so you could make such a thing. But it would somehow fake handing out the type so you'd have to be careful how you did that. Then its job would be to hand out the synthetic children. If people really hate making the fake structure we could go this way instead, but it seems better to use an extant facility if we can, and "fake handing out the type" seems to me at least as dangerous as injecting synthesized types into the TypeSystem. The latter we already do, the former we don't.

I'm not happy with the notion of just hard-coding this to CF types or making it about bridged types at all. It is not uncommon to have a client library that vends opaque pointers, but you've figured out what some of the fields are. One solution to debugging this scenario is to introduce another shadow type, either in lldb or actually in your code, and cast types to that when printing. But that means you can't use "frame var" since it doesn't support casting, and you have to remember to cast every time in the expression parser which is annoying. if you made a synthetic child provider, and if what Ismail is trying to get working here works generally, then once you've made the provider, you can just print * of the pointer, and it will work. So I think this should be a general facility for opaque pointer types with synthetic child providers. So I'd much prefer to keep this a general facility.

mib marked 4 inline comments as done.May 7 2020, 1:52 PM
mib added inline comments.
lldb/source/Core/ValueObject.cpp
52

I'll change this before updating the differential with the fixes.

2837

Yes, from my understanding, if the opaque pointer doesn't have any synthetic child provider, it won't have a synthetic value and lldb should error saying error: dereference failed.

mib marked 2 inline comments as done.May 7 2020, 1:58 PM
jingham added inline comments.May 7 2020, 2:49 PM
lldb/source/Core/ValueObject.cpp
2837

We should add a test to make sure this stays true.

mib updated this revision to Diff 262823.May 8 2020, 12:28 AM
mib edited the summary of this revision. (Show Details)
  • Added a virtual method to create an empty struct type TypeSystem::GetEmptyStructType (if someone can think of a better name, I'm open to suggestions)
  • Refactored ValueObject::Dereference
  • Reverted the NFC changes in ValueObject::CreateChildAtIndex
  • Added test for C++ incomplete type with data-formatter (WIP)
teemperor requested changes to this revision.May 8 2020, 8:05 AM

I don't see a huge problem with things like __lldb_autogen_nspair as it's a single obviously generated type hardcoded into LLDB. But this really generic approach here can generate all kind of structs whenever some formatter decides that some random incomplete record has a synthetic value.

But in any case, I think we're kind of talking about different problems here. We do have the following type:

TypedefType 0x7fc9cd04f440 'CFDictionaryRef' sugar
|-Typedef 0x7fc9cd04f3e0 'CFDictionaryRef'
`-PointerType 0x7fc9cd04f3a0 'const struct __CFDictionary *'
  `-QualType 0x7fc9cd04f381 'const struct __CFDictionary' const
    `-RecordType 0x7fc9cd04f380 'struct __CFDictionary'
      `-CXXRecord 0x7fc9cd04f2d0 '__CFDictionary'

So we do have an underlying type for *CFDictionaryRef which is just the __CFDictionary record. The problem is just that this is an incomplete type. So we could make a ValueObject that has an incomplete struct type which seems to work for me just fine? We probably need to adjust the formatter for this prints an actual empty value: (const __CFDictionary) *dict = {}

lldb/source/Core/ValueObject.cpp
2844

This is still the name of the typedef:

child_compiler_type =
    type_system->GetEmptyStructType(compiler_type.GetTypeName());

So we end up with an AST like we get from here:

typedef NSDictionary* CFDictionaryRef;
struct CFDictionaryRef {}; // conflict

This should at least have some kind of __lldb_made_up_type_unique_stringy_ prefix in the generated name to make this not ambiguous.

Also a log statement would be useful that we know when these types are created when someone reports a bug.

This revision now requires changes to proceed.May 8 2020, 8:05 AM

I don't see a huge problem with things like __lldb_autogen_nspair as it's a single obviously generated type hardcoded into LLDB. But this really generic approach here can generate all kind of structs whenever some formatter decides that some random incomplete record has a synthetic value.

But in any case, I think we're kind of talking about different problems here. We do have the following type:

TypedefType 0x7fc9cd04f440 'CFDictionaryRef' sugar
|-Typedef 0x7fc9cd04f3e0 'CFDictionaryRef'
`-PointerType 0x7fc9cd04f3a0 'const struct __CFDictionary *'
  `-QualType 0x7fc9cd04f381 'const struct __CFDictionary' const
    `-RecordType 0x7fc9cd04f380 'struct __CFDictionary'
      `-CXXRecord 0x7fc9cd04f2d0 '__CFDictionary'

So we do have an underlying type for *CFDictionaryRef which is just the __CFDictionary record. The problem is just that this is an incomplete type. So we could make a ValueObject that has an incomplete struct type which seems to work for me just fine? We probably need to adjust the formatter for this prints an actual empty value: (const __CFDictionary) *dict = {}

That's exactly what I was thinking. As the CompilerType abstraction is already capable of representing incomplete types, and our goal is to construct synthetic children ( == ValueObjects) for these types, then it seems reasonable for ValueObjects to operate on these incomplete types. I mean, one won't be able to do much with them, but it should be possible to take their address and then grovel in that memory, or cast it to something else (and I assume that's what these pretty printers will be doing).

mib updated this revision to Diff 264038.May 14 2020, 11:17 AM
mib edited the summary of this revision. (Show Details)

Instead of creating a new empty struct type, this new implementation will use the opaque pointer's pointee type to create the new ValueObjectChild.
This makes the previously introduced helper function in TypeSystem and TypeSystemClang useless, so I got rid of them.

teemperor accepted this revision.May 14 2020, 11:47 PM

IMHO this looks good now. Maybe add a comment to the commit that this adds support for incomplete types in ValueObjects.

I think @jingham should also take a look at this to make sure this makes sense.

lldb/source/Core/ValueObjectSyntheticFilter.cpp
56–58

Maybe comment this line that copying the data of an incomplete type doesn't make sense as it has no (byte) size. (Previously this worked as our made-up type was empty and had a size of 0 bytes.)

This revision is now accepted and ready to land.May 14 2020, 11:47 PM
labath added inline comments.May 15 2020, 2:34 AM
lldb/source/Core/ValueObject.cpp
2833–2855

Can this be merged with the code above? It seems like the only difference is the way it which we get the CompilerType.

2836

What's the reason for limiting this to the C family? It seems like one could want to have a pretty printer for incomplete types in any language...

This revision was automatically updated to reflect the committed changes.
mib marked 4 inline comments as done.
labath added inline comments.Jul 7 2020, 6:03 AM
lldb/source/Core/ValueObject.cpp
690–694

I'm getting a crash here when attempting to dereference incomplete c(++) types. The simplest (albeit contrived) repro is:

$ cat /tmp/a.c 
struct incomplete;

struct incomplete *var = (struct incomplete *)0xdead;

int main() {}
$ clang /tmp/a.c -o /tmp/a.out -g
$ lldb /tmp/a.out -o "target variable var[0]"
(lldb) target create "/tmp/a.out"
Current executable set to '/tmp/a.out' (x86_64).
(lldb) target variable var[0]
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.	Program arguments: lldb /tmp/a.out -o target variable var[0] 
 #0 0x0000561dcd331cea llvm::sys::PrintStackTrace(llvm::raw_ostream&) (lldb+0x29cea)
 #1 0x0000561dcd32ffc4 llvm::sys::RunSignalHandlers() (lldb+0x27fc4)
 #2 0x0000561dcd330108 SignalHandler(int) (lldb+0x28108)
 #3 0x00007f62b35175b0 __restore_rt (/lib64/libpthread.so.0+0x135b0)
 #4 0x00007f62ae9235fc lldb_private::ValueObject::CreateChildAtIndex(unsigned long, bool, int) (../lib/liblldb.so.11git+0xb985fc)
 #5 0x00007f62ae923e2b lldb_private::ValueObject::GetSyntheticArrayMember(unsigned long, bool) (../lib/liblldb.so.11git+0xb98e2b)
...
mib added a comment.Jul 7 2020, 10:44 AM

@labath Thanks for reporting that crash. D83327 should fix the issue.