Page MenuHomePhabricator

[lldb] Add deref support and tests to shared_ptr synthetic
ClosedPublic

Authored by kastiglione on Feb 21 2021, 2:53 PM.

Details

Summary

Add frame variable dereference suppport to libc++ std::shared_ptr.

This change allows for commands like v *thing_sp and v thing_sp->m_id. These commands now work the same way they would with raw pointers, and as they would with expression. This is done by adding an unaccounted for child member named $$dereference$$.

Without this change, the command would have to be written as v *thing_sp.__ptr_ or v thing_sp.__ptr_->m_id which exposes internal structure and is more clumsy to type.

Also, introduces API tests for std::shared_ptr, previously there were none.

Diff Detail

Event Timeline

kastiglione requested review of this revision.Feb 21 2021, 2:53 PM
kastiglione created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2021, 2:53 PM
kastiglione edited the summary of this revision. (Show Details)Feb 21 2021, 3:02 PM
kastiglione added inline comments.Feb 21 2021, 3:06 PM
lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
410–433

Note that these two indexes were not accessed because CalculateNumChildren returns either 0 or 1. So this is actually NFC despite its looks.

472–475

Note that count and weak_count were not accessed because CalculateNumChildren returns either 0 or 1. So this is actually NFC despite its looks.

kastiglione edited the summary of this revision. (Show Details)Feb 21 2021, 7:37 PM

Looks like a great improvement.

teemperor requested changes to this revision.Feb 22 2021, 8:27 AM

You can use expect_var_path to test frame var (LLDB calls the frame var input "expressions paths", hence the _path suffix). That way you can test all of these things for you without having to rely on just finding substrs in the output or having to depend on the way LLDB prints the result:

self.expect_var_path("sp_str", summary="\"hello\" strong=1 weak=1",
                     type="std::shared_ptr<std::basic_string<char, std::char_traits<char>, std::allocator<char> > >",
    children = [
        ValueCheck(name="__ptr_", summary="\"hello\"")
    ]
)
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/TestDataFormatterLibcxxSharedPtr.py
12

Libcx typo. FWIW, you don't have to give these classes a unique name anymore.

18

Comment + function name don't really fit to the test

22

You only need lldbutil.run_to_source_breakpoint(self, '// break here', lldb.SBFileSpec("main.cpp")) (the variable assignments and so on aren't necessary)

26

FWIW, this isn't really testing anything. This just tests that the pointer starts with '0x0', but even non-nullptrs start usually with 0x0 as we pad them with zeroes.

In theory you could test that this is equal to 0x0000000000000000 but then we have the test suite depend on the pointer size.

I think the best solution is to have the test suite figure out what a hex nullptr is on the target and then just provide that as utility variable in lldbtest. But that's out of the scope for this test, so I would say you can just leave it out until someone has time to implement that.

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/main.cpp
2

Unused include?

This revision now requires changes to proceed.Feb 22 2021, 8:27 AM
kastiglione added inline comments.Feb 22 2021, 8:38 AM
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/TestDataFormatterLibcxxSharedPtr.py
12

also copy & paste. What name should I give it, if not unique?

18

ha, this is copypasta from the test for unique_ptr, which also contains this.

22

more copy, thanks

26

I could use a regex for now.

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/main.cpp
2

pasta, thanks

teemperor added inline comments.Feb 22 2021, 9:57 AM
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/TestDataFormatterLibcxxSharedPtr.py
12

You can just call it TestCase, but you can also keep a unique name. It doesn't really matter. I personally use TestCase because repeating the file name inside a file is redundant, but we used to give all tests unique class names, so feel free to carry on that tradition.

26

That would also work in the meantime.

Use expect_var_path and fix other issues with the tests

This looks great. It's a little weird to use "SBValue.value" to get a string version of something that's a pointer and then have to do regex compares. It's more direct to use "unsigned" and compare against 0x0.

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/TestDataFormatterLibcxxSharedPtr.py
32

If you used "unsigned" instead of "value" you'd get a number back and you could check that directly against 0x0 rather than having to do a regex match. If you want to be careful, use GetValueAsUnsigned(fail_value=lldb.LLDB_INVALID_ADDRESS) since the default fail value is 0...

test for null using SBValue.unsigned

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/TestDataFormatterLibcxxSharedPtr.py
32

thanks, this is much nicer, I've fixed up the pointer asserts to check against the unsigned value.

kastiglione marked 8 inline comments as done.Feb 22 2021, 4:00 PM
jingham accepted this revision.Feb 22 2021, 5:36 PM

LGTM at this point.

teemperor accepted this revision.Feb 23 2021, 4:06 AM

This LGTM modulo a missing nullptr check. Thanks for fixing this!

Also we probably should investigate the strong= summary differences. I would have blamed the fake constructors we are creating in LLDB, but as this isn't running any expressions this might us just reading the wrong values?

lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
406

ptr_sp should have a nullptr check as __ptr_ might be missing because we screw up some debug info parsing, or someone renamed the member or idk. In any case, this shouldn't crash.

This revision is now accepted and ready to land.Feb 23 2021, 4:06 AM

check ptr_sp before use

kastiglione marked an inline comment as done.Feb 23 2021, 9:02 AM
This revision was landed with ongoing or failed builds.Feb 23 2021, 9:04 AM
This revision was automatically updated to reflect the committed changes.