This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Adjust libc++ string formatter for changes in D123580
ClosedPublic

Authored by labath on Apr 20 2022, 11:59 AM.

Details

Summary

The code needs more TLC, but for now I've tried making only the changes
that are necessary to get the tests passing -- postponing the more
invasive changes after I create a more comprehensive test.

In a couple of places I have changed the index-based element accesses to
name-based ones (as these are less sensitive to code perturbations). I'm
not sure why the code was using indexes in the first place, but I've
(manually) tested the change with various libc++ versions, and found no
issues with this approach.

Diff Detail

Event Timeline

labath created this revision.Apr 20 2022, 11:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2022, 11:59 AM
labath requested review of this revision.Apr 20 2022, 11:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2022, 11:59 AM
philnik added inline comments.Apr 20 2022, 2:16 PM
lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
660

This should only be done if the string is in the normal layout and little endian or in the alternate layout and big endian. Why do you care about the capacity at all? Isn't that just another point of failure?

Thank you, the names make a lot more sense to me but I am not sure why this was originally used either.

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

nitpick /*can_create=*/true and same for the next two.

labath updated this revision to Diff 424125.Apr 21 2022, 1:38 AM
labath marked 2 inline comments as done.
  • address review comments
labath added inline comments.Apr 21 2022, 1:41 AM
lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
660

I've added the layout check and made a note that big-endian is not supported. The capacity check comes from https://reviews.llvm.org/D73860, but I'm not sure of the overall purpose. I guess it was trying to screen out corrupt/uninitialized string objects, but I'm not convinced that we're doing anyone a favour by refusing to format those -- after all, the application itself might access such objects without checking the capacity, and operate on the data that we've decided to ignore.

philnik accepted this revision.Apr 21 2022, 4:02 AM

LGTM assuming you checked that it actually works with my patch applied. Do you want to land your patch first, or should we do it the other way around?

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

Maybe it would make sense to say that the string is uninitialized or corrupted when the __invariants() fail? That would probably catch more bugs, but I don't know if you can just run a member function in the pretty printer.

This revision is now accepted and ready to land.Apr 21 2022, 4:02 AM

LGTM assuming you checked that it actually works with my patch applied. Do you want to land your patch first, or should we do it the other way around?

I've confirmed this with the real patch and with my magic string simulator. For me, it would be better if this patch went in first (so I guess I'll land it now).

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

Technically, we can run functions from pretty-printers, but we try hard to avoid that. If calling functions was ok, we could just call data() and get rid of this mess. :)