Page MenuHomePhabricator

Adding libc++ formattors for std::optional
ClosedPublic

Authored by shafik on Jul 12 2018, 4:04 PM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
davide added a subscriber: davide.Jul 12 2018, 4:38 PM

Some comments. Jonas looked at many formatters recently so he's in a good position to take a look.

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/Makefile
10

commented out code?

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py
22–23

ditto

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/main.cpp
15–39

You don't really need to se all these breakpoints.
You just need set one on the return and print all the types.
Also, an lldbInline python test here should suffice and it's probably more readable (grep for lldbInline).

source/Plugins/Language/CPlusPlus/CMakeLists.txt
15–16

Please sort this.

source/Plugins/Language/CPlusPlus/LibCxx.cpp
48–51

Can you use StringRef?

source/Plugins/Language/CPlusPlus/LibCxx.h
30–33

Please clang-format this patch.

source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp
60

Please remove this commented out code.

64

if you want to log diagnostics, you might consider using the LOG facility and then add these to the data formatters channel.

davide requested changes to this revision.Jul 12 2018, 4:38 PM
davide added a reviewer: JDevlieghere.
This revision now requires changes to proceed.Jul 12 2018, 4:38 PM
jingham requested changes to this revision.EditedJul 12 2018, 4:48 PM

It seems a little odd to choose to represent this as an array with 1 element. I think I would be confused by that. You can maybe choose a better name like "value" for your cloned object. But it looks like it is also possible to make a Synthetic Child Provider that just replaces the value of the object with the value provided by the synthetic child provider. That's done in the Python front end by saying you have no children and then providing a the "get_value" method. It must be possible to do this from C++ as well, but it will take a little thought to figure out how that works. But that would be the most natural way to present this, I think.

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py
27–39

you can do this more cleanly with:

(self.target, _, _, bkpt) = lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.cpp"))

40–49

I don't think you need this cleanup, you aren't adding any formatters, you're just testing built-in ones.

source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp
64

If you want to log this, get the formatters log channel and put the text there. Then somebody will see this when they turn on the log. An fprintf like this won't help anybody.

shafik updated this revision to Diff 155508.Jul 13 2018, 3:05 PM
shafik marked 7 inline comments as done.
shafik edited the summary of this revision. (Show Details)

Address comments

  • Applying clang-formt
  • Refactoring OptionalFrontEnd to produce out that makes the underlying data look like an array
  • Removing commented out code and left in debugging
  • Using StringRef instead of const char *

@davide @jingham Thank you for the review, those were good catches and comments.

I have addressed them except for refactoring the test to use lldbInline which I will be working on now.

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/main.cpp
15–39

This looks like a good idea, I will try it out and let you know.

source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp
64

Thank you for catching, I meant to remove this.

This is getting really close. Please try the lldbInline test format and revert the unrelated bits and I'll take another look.
Thanks!

source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
973–974

this looks unrelated?

source/Plugins/Language/CPlusPlus/LibCxx.cpp
183–211

this looks like .. unrelated?

shafik updated this revision to Diff 155618.Jul 15 2018, 10:57 PM

Removing unrelated changes due to applying clang-format to patch file with context.

Looks good to me, modulo the inline test (or the current comments addressed). Thanks Shafik!

shafik updated this revision to Diff 155758.Jul 16 2018, 2:02 PM

Refactoring test to use lldbinline

shafik marked 5 inline comments as done and 3 inline comments as done.Jul 16 2018, 2:04 PM

@jingham @davide I believe I have addressed all your comments

JDevlieghere accepted this revision.Jul 17 2018, 3:53 AM

LGTM, Thanks!

davide requested changes to this revision.Jul 17 2018, 5:29 PM

Probably last round of comments. Thanks for your patience!

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/Makefile
8

you don't need this, it's implicit (-O0)

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py
9

I do wonder if you need a decorator to indicate that this is a libcxx only test (and skip everywhere the library isn't supported).

source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
496

I'm a little nervous about this regex, but I think it's fine and I'll let Jim take another look in case I missed something.

source/Plugins/Language/CPlusPlus/LibCxx.cpp
42–51

This is really obscure to somebody who doesn't understand the type internally. Can you add a comment explaining the structure? (here or in the synthetic)

This revision now requires changes to proceed.Jul 17 2018, 5:29 PM
jingham accepted this revision.Jul 17 2018, 5:40 PM

This looks fine. I agree with Davide that putting some description of the type you are formatting in comments somewhere would be make things easier for somebody else who might have to fix this (or to you when you've totally forgotten how this worked a year from now...)

I'm okay with this if you fix the things Davide asked about (except the regex, that looks right to me.)

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py
9

Yes, you do need to mark this or you will get spurious failures at least on Windows, which turns off the libcxx category.

source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
496

That looks right to me, it's catching the type or a reference to it.

shafik updated this revision to Diff 156886.Jul 23 2018, 2:50 PM
shafik marked 4 inline comments as done.

Addressing comments

  • -O0 is not needed in Makefile
  • engaged is not friendly terminology so switching to "Has Value"
  • Switching test away from lldbinline style due to bug w/ .categories files which does not work on these tests
shafik marked 6 inline comments as done.Jul 23 2018, 2:52 PM

@davide One more pass

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py
9

Switched back to the old style tests due to the .categories bug we discussed earlier.

source/Plugins/Language/CPlusPlus/LibCxx.cpp
42–51

That is a good point, I am looking at the cppreference page for it and I think maybe has_value might be an improvement.

davide accepted this revision.Jul 23 2018, 3:37 PM

This is good, but please add a comment explaining the type before committing.

This revision is now accepted and ready to land.Jul 23 2018, 3:37 PM
shafik updated this revision to Diff 157337.Jul 25 2018, 12:24 PM

Adding additional comments

This revision was automatically updated to reflect the committed changes.

Your latest update doesn't contain CMakeList.txt files and the Xcode project changes. That's why it failed to apply. Please upload a correct version and I'll commit it for you.

Recommitted:

a11963323fc Recommit [DataFormatters] Add formatter for C++17 std::optional.

Authentication realm: https://llvm.org:443 LLVM Subversion repository
Password for 'davide': ***

Sending lldb/trunk/lldb.xcodeproj/project.pbxproj
Adding lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional
Adding lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/Makefile
Adding lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py
Adding lldb/trunk/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/main.cpp
Sending lldb/trunk/source/Plugins/Language/CPlusPlus/CMakeLists.txt
Sending lldb/trunk/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
Sending lldb/trunk/source/Plugins/Language/CPlusPlus/LibCxx.cpp
Sending lldb/trunk/source/Plugins/Language/CPlusPlus/LibCxx.h
Adding lldb/trunk/source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp
Transmitting file data .........done
Committing transaction...
Committed revision 338156.
Committed a11963323fc to svn.

I'm afraid I had to revert this again. It broke an ubuntu lldb-cmake bot which builds with clang-3.5 (which has no -std=c++17 support flag, as 17 wasn't there yet).

I'd recommend to update this patch with another decorator to skip places where -std=c++17 is not available doesn't work, and then we'll try again.

Bot logs for completeness:

Session logs for test failures/errors/unexpected successes will go into directory 'logs-clang-3.5-i386'
Command invoked: /lldb-buildbot/lldbSlave/buildWorkingDir/scripts/../llvm/tools/lldb/test/dotest.py -A i386 -C clang-3.5 -v -s logs-clang-3.5-i386 -u CXXFLAGS -u CFLAGS --env ARCHIVER=ar --env OBJCOPY=objcopy --executable /lldb-buildbot/lldbSlave/buildWorkingDir/scripts/../build/bin/lldb --channel gdb-remote packets --channel lldb all --skip-category watchpoint --skip-category lldb-mi --results-port 43584 -S fnmac --inferior -p TestDataFormatterLibcxxOptional.py /lldb-buildbot/lldbSlave/buildWorkingDir/llvm/tools/lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional --event-add-entries worker_index=4:int

Configuration: arch=i386 compiler=/usr/bin/clang-3.5
----------------------------------------------------------------------
Collected 4 tests

1: test_with_run_command_dsym (TestDataFormatterLibcxxOptional.LibcxxOptionalDataFormatterTestCase)
   Test that that file and class static variables display correctly. ... skipped 'test case does not fall in any category of interest for this run'
2: test_with_run_command_dwarf (TestDataFormatterLibcxxOptional.LibcxxOptionalDataFormatterTestCase)
   Test that that file and class static variables display correctly. ... 3: test_with_run_command_dwo (TestDataFormatterLibcxxOptional.LibcxxOptionalDataFormatterTestCase)
   Test that that file and class static variables display correctly. ... 4: test_with_run_command_gmodules (TestDataFormatterLibcxxOptional.LibcxxOptionalDataFormatterTestCase)
   Test that that file and class static variables display correctly. ... skipped 'test case does not fall in any category of interest for this run'

======================================================================
ERROR: test_with_run_command_dwarf (TestDataFormatterLibcxxOptional.LibcxxOptionalDataFormatterTestCase)
   Test that that file and class static variables display correctly.
----------------------------------------------------------------------
Error when building test subject.

Build Command:
make VPATH=/lldb-buildbot/lldbSlave/buildWorkingDir/llvm/tools/lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional -C /lldb-buildbot/lldbSlave/buildWorkingDir/scripts/lldb-test-build.noindex/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.test_with_run_command_dwarf -I /lldb-buildbot/lldbSlave/buildWorkingDir/llvm/tools/lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional -f /lldb-buildbot/lldbSlave/buildWorkingDir/llvm/tools/lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/Makefile MAKE_DSYM=NO ARCH=i386 CC="/usr/bin/clang-3.5" 

Build Command Output:
error: invalid value 'c++17' in '-std=c++17'
error: invalid value 'c++17' in '-std=c++17'
make: *** [main.o] Error 1
labath added a subscriber: labath.Jul 30 2018, 12:51 AM

Thanks for looking out for the bots.

I am afraid the compiler check will not be enough here. After that, you'll run into the issue of the system libc++ not being recent enough to contain std::optional. I suppose this could be handled by including some other header first (to make sure __config is evaluated) and then checking the _LIBCPP_VERSION macro.

I am sorry that this is so annoying. It's surprising that this is the first time we're running into this, but I guess this is the first data formatter for something that hasn't been implemented in libc++ since forever.

shafik updated this revision to Diff 158121.Jul 30 2018, 3:58 PM

Adjust test to filter on clang version and libc++ version to precent build bots from failing due to lack of C++17 or lack of optional support

@davide @labath I believe I have addressed both the C++17 filter and the libc++ filter. Please let me know if you have further comments.

labath added inline comments.Jul 31 2018, 2:16 AM
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py
22

Could you add another line for gcc here? The -std=c++17 flag seems to be supported starting with gcc-5.1.

Also a comment that this is due to the -std flag would be helpful to people looking at this in the future.

45

Replace by self.skipTest(...). This way, the test will be properly marked as skipped.

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/main.cpp
16–18

these need to be guarded by the #if too (i'd just move then into the main function).

shafik updated this revision to Diff 158342.Jul 31 2018, 11:22 AM
shafik marked 2 inline comments as done.
  • Adding comments
  • Changing from exit to self.skipTest
  • Adding skip for gcc
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py
22

Adding a comment makes sense.

So @add_test_categories(["libc++"]) won't skip for gcc bots then?

shafik updated this revision to Diff 158362.Jul 31 2018, 12:43 PM

Fixing gcc skipIf to check for version as well

labath added a comment.Aug 1 2018, 4:44 AM

Thanks for the patience. I've tried the patch out on our end. You shouldn't have any problems now.

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py
22

It won't because it doesn't make sense to do that. Gcc is perfectly capable of using libc++. The only tricky thing is that it doesn't have the magic -stdlib argument, so you have to specify the include paths and libraries explicitly (which our makefiles do).

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/main.cpp
18–21

This could be simplified to bool has_optional = HAVE_OPTIONAL;

shafik updated this revision to Diff 158662.Aug 1 2018, 4:31 PM

Simplifying initialization of has_optional in test.

shafik marked 4 inline comments as done.Aug 1 2018, 4:33 PM

@labath Addressed comment, thank you for helping out.

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py
22

Good point I did not realize this before!

packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/main.cpp
18–21

Good catch!

I see this test failing on Linux right now (bots are down, so I can't confirm that the official bots fail as well). The failure is because of the last decorator which was not part of the review:

@skipIf(macos_version=["<", "10.14"])

It looks like this only works correctly when mac_ver returns a meaningful result (a.k.a. on mac platforms), so linux is broken.

Another thing that I noticed is that the name of the test class (LibcxxOptionalDataFormatterTestCase) was re-used from another test. This has caused problems in the past with test overriding each other's results because they share the same class and test name (as these do).

shafik marked 2 inline comments as done.Oct 12 2018, 12:54 PM

@stella.stamenova Thank you for catching this. I fixed the test names, I am looking into the best way to fix the skipif now.

@stella.stamenova Thank you for catching this. I fixed the test names, I am looking into the best way to fix the skipif now.

@shafik I send a change for review this morning that I think should do the trick: https://reviews.llvm.org/D53208. The other option is to change the skipIf to include the os like some of the other ones.