This is an archive of the discontinued LLVM Phabricator instance.

Add gdb pretty printers for a wide variety of libc++ data structures.
ClosedPublic

Authored by saugustine on Aug 1 2019, 2:55 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

saugustine created this revision.Aug 1 2019, 2:55 PM
Herald added a project: Restricted Project. · View Herald Transcript
thakis added a subscriber: thakis.Aug 1 2019, 3:01 PM
thakis added inline comments.
libcxx/utils/gdb/libcxx/printers.py
32 ↗(On Diff #212910)

(llvm uses pep8 style)

Please also add installation, since pretty-printers should be shipped with libc++, same as GCC does with libstdc++ and its pretty-printers.

EricWF added a comment.Aug 3 2019, 7:52 PM

wow. This is a fantastic amount of work. Thank you. No longer will libc++ accidentally break the pretty printers!

I'm happy to handle the minutiae related to configuring the build and test suite. I check in some code to detect GDB and LLDB shortly.

More review to come shortly...

Thanks again!

libcxx/test/pretty_printers/gdb_pretty_printer_test.sh.cpp
2 ↗(On Diff #212910)

Please add the LLVM license header to all new files.

wow. This is a fantastic amount of work. Thank you. No longer will libc++ accidentally break the pretty printers!

I want to echo this. This is really cool. I _really_ hope we can get something similar for LLDB.

I'm happy to handle the minutiae related to configuring the build and test suite. I check in some code to detect GDB and LLDB shortly.

We could have a lit feature like has-gdb or something along those lines? And we can then put these pretty-printer tests directly in test/libcxx. WDYT?

wow. This is a fantastic amount of work. Thank you. No longer will libc++ accidentally break the pretty printers!

I want to echo this. This is really cool. I _really_ hope we can get something similar for LLDB.

LLDB has pretty-printers. They are implemented in C++ instead of scripting.

wow. This is a fantastic amount of work. Thank you. No longer will libc++ accidentally break the pretty printers!

I want to echo this. This is really cool. I _really_ hope we can get something similar for LLDB.

LLDB has pretty-printers. They are implemented in C++ instead of scripting.

Sorry, my point is that we don't have the pretty-printer tests checked into libc++ and part of its test suite, which has been a source of pain in the past. Changing that situation is what I was referring to when I said "I hope we can get something similar for LLDB".

wow. This is a fantastic amount of work. Thank you. No longer will libc++ accidentally break the pretty printers!

I want to echo this. This is really cool. I _really_ hope we can get something similar for LLDB.

LLDB has pretty-printers. They are implemented in C++ instead of scripting.

Sorry, my point is that we don't have the pretty-printer tests checked into libc++ and part of its test suite, which has been a source of pain in the past. Changing that situation is what I was referring to when I said "I hope we can get something similar for LLDB".

Sorry for misunderstanding, but may be libc++ developers should run LLDB tests too? Or at least make sure that libc++ changes do force LLDB tests on BuildBot and tests results should be monitored by committer?

Please also add installation, since pretty-printers should be shipped with libc++, same as GCC does with libstdc++ and its pretty-printers.

Did these pretty-printers exist for libc++ before this patch? If so, how were they shipped?

libcxx/test/pretty_printers/gdb_pretty_printer_test.py
99 ↗(On Diff #212910)

I assume this is a remnant of prior testing?

Please also add installation, since pretty-printers should be shipped with libc++, same as GCC does with libstdc++ and its pretty-printers.

Did these pretty-printers exist for libc++ before this patch? If so, how were they shipped?

There is separate project on GitHub. It was mentioned in libcxx/docs/UsingLibcxx.rst.

GCC ships libstdc++ pretty-printers in <install directory>/share/gcc-<version>/python/libstdcxx/.

ldionne requested changes to this revision.Aug 5 2019, 10:20 AM

Please also add installation, since pretty-printers should be shipped with libc++, same as GCC does with libstdc++ and its pretty-printers.

Did these pretty-printers exist for libc++ before this patch? If so, how were they shipped?

There is separate project on GitHub. It was mentioned in libcxx/docs/UsingLibcxx.rst.

I had missed that, thanks!

In that case, this patch should update the documentation.

GCC ships libstdc++ pretty-printers in <install directory>/share/gcc-<version>/python/libstdcxx/.

I guess we could start with doing the same for libc++'s pretty-printers.

This revision now requires changes to proceed.Aug 5 2019, 10:20 AM
saugustine updated this revision to Diff 213467.Aug 5 2019, 2:41 PM
saugustine marked 4 inline comments as done.
  • Run a pep8 formatter.
  • Address other comments from code review.

wow. This is a fantastic amount of work. Thank you. No longer will libc++ accidentally break the pretty printers!

tamur and rdhindsa (cc'd as reviewers) both deserve strong credit for this. It wasn't solely me by a long shot. I will include them in the final commit message.

I'm happy to handle the minutiae related to configuring the build and test suite. I check in some code to detect GDB and LLDB shortly.

Thank you--that will be very helpful.

I've updated the revision to address these comments.

Please also add installation, since pretty-printers should be shipped with libc++, same as GCC does with libstdc++ and its pretty-printers.

I wanted to discuss how to do that, but I think ericwf is thankfully taking that hassle from me.

wow. This is a fantastic amount of work. Thank you. No longer will libc++ accidentally break the pretty printers!

I want to echo this. This is really cool. I _really_ hope we can get something similar for LLDB.

I'm unfamiliar with how lldb handles scripting, but the python script portion of the framework is fairly short and straightforward. The test program itself contains both its inputs and test cases, so lldb could be run against it no problem. Perhaps with an ifdef to use lldb-based comparison strings.

libcxx/test/pretty_printers/gdb_pretty_printer_test.py
99 ↗(On Diff #212910)

Rats. Fixed.

Anyone have any remaining thoughts?

Would someone please accept this patch, or let me know what else there is to be done?

Please also add installation, since pretty-printers should be shipped with libc++, same as GCC does with libstdc++ and its pretty-printers.

Did these pretty-printers exist for libc++ before this patch? If so, how were they shipped?

There is separate project on GitHub. It was mentioned in libcxx/docs/UsingLibcxx.rst.

I had missed that, thanks!

In that case, this patch should update the documentation.

GCC ships libstdc++ pretty-printers in <install directory>/share/gcc-<version>/python/libstdcxx/.

I guess we could start with doing the same for libc++'s pretty-printers.

We could check them in for now and then EricWF can do the moving and updating of documentation around when they're happy?

I'm pretty happy with this as is... any objections?

-eric

echristo accepted this revision.Aug 30 2019, 4:32 PM

The concern that ldionne had has been addressed and I think anything else can be done in post - it'd be nice to have these available for people to use while EricWF works up some of the test harness.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 30 2019, 4:42 PM
This revision was automatically updated to reflect the committed changes.

Thanks. Commited as r370551

Reverted because it broke some buildbots. Looking into why.