Mordante added inline comments.
================
Comment at: libcxx/utils/gdb/libcxx/printers.py:192
class StdStringPrinter(object):
"""Print a std::string."""
----------------
ldionne wrote:
> philnik wrote:
> > labath wrote:
> > > philnik wrote:
> > > > dblaikie wrote:
> > > > > philnik wrote:
> > > > > > jgorbe wrote:
> > > > > > > Mordante wrote:
> > > > > > > > philnik wrote:
> > > > > > > > > Mordante wrote:
> > > > > > > > > > philnik wrote:
> > > > > > > > > > > Mordante wrote:
> > > > > > > > > > > > Does this also break the LLDB pretty printer?
> > > > > > > > > > > Probably. Would be nice to have a test runner for that.
> > > > > > > > > > I already planned to look into that, D97044#3440904 ;-)
> > > > > > > > > Do you know where I would have to look to know what the LLDB
> > > > > > > > > pretty printers do?
> > > > > > > > Unfortunately no. @jingham seems to be the Data formatter code
> > > > > > > > owner.
> > > > > > > There was a recent lldb change fixing prettyprinters after a
> > > > > > > similar change to string:
> > > > > > > https://github.com/llvm/llvm-project/commit/45428412fd7c9900d3d6ac9803aa7dcf6adfa6fe
> > > > > > >
> > > > > > > If the gdb prettyprinter needed fixing for this change, chances
> > > > > > > are that lldb will need a similar update too.
> > > > > > Could someone from #lldb help me figure out what to change in the
> > > > > > pretty printers? I looked at the file, but I don't really
> > > > > > understand how it works and TBH I don't really feel like spending a
> > > > > > lot of time figuring it out. If nobody says anything I'll land this
> > > > > > in a week.
> > > > > >
> > > > > > As a side note: it would be really nice if there were a few more
> > > > > > comments inside `LibCxx.cpp` to explain what happens there. That
> > > > > > would make fixing the pretty printer a lot easier. The code is
> > > > > > probably not very hard (at least it doesn't look like it), but I am
> > > > > > looking for a few things that I can't find and I have no idea what
> > > > > > some of the things mean.
> > > > > Looks like something around
> > > > > https://github.com/llvm/llvm-project/blob/2e6ac54cf48aa04f7b05c382c33135b16d3f01ea/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp#L597
> > > > > (& the similar masking in the `else` block a few lines down) - I
> > > > > guess a specific lookup for the new field would be needed, rather
> > > > > than the bitmasking.
> > > > Yes, but what do the numbers in `size_mode_locations` mean? Why is
> > > > there no checking if it's big or little endian? Is it unsupported
> > > > maybe? Does it work because of something else? Is there a reason that
> > > > `g_data_name` exists instead of comparing directly? Should I add
> > > > another layout style or should I just update the code for the new
> > > > layout?
> > > > I don't know anything about the LLDB codebase, so I don't understand
> > > > the code and I don't know how I should change it.
> > > I don't think there's been any official policy decision either way, but
> > > historically we haven't been asking libc++ authors to update lldb pretty
> > > printers -- we would just fix them up on the lldb side when we noticed
> > > the change. The thing that has changed recently is that google started
> > > relying (and testing) more on lldb, which considerably shortened the time
> > > it takes to notice this change, and also makes it difficult for some
> > > people to make progress while we are in this state. But I don't think
> > > that means that updating the pretty printer is suddenly your
> > > responsibility.
> > >
> > > As for your questions, I'll try to answer them as best as I can:
> > > > what do the numbers in size_mode_locations mean?
> > > These are the indexes of fields in the string object. For some reason
> > > (unknown to me), the pretty printer uses indexes rather than field names
> > > for its work. Prompted by the previous patch, I've been trying to change
> > > that, but I haven't done it yet, as I was trying to improve the testing
> > > story (more on that later).
> > > > Why is there no checking if it's big or little endian? Is it
> > > > unsupported maybe?
> > > Most likely yes. Although most parts of lldb support big endian, I am not
> > > aware of anyone testing it on a regular basis, so it's quite likely that
> > > a lot of things are in fact broken.
> > > > Is there a reason that g_data_name exists instead of comparing directly?
> > > LLDB uses a global string pool, so this is an attempt to reduce the
> > > number of string pool queries. The pattern is not consistently used
> > > everywhere, and overall, I wouldn't be too worried about it.
> > > > Should I add another layout style or should I just update the code for
> > > > the new layout?
> > > As the pretty printers ship with lldb, they are expected to support not
> > > just the current format, but also the past ones (within reason). This is
> > > what makes adding a new format (or just refactoring the existing code)
> > > difficult, and it's why I was trying to come up with better tests for
> > > this (it remains to be seen if I am successful).
> > >
> > > Anyway, I think I should be able to make that pretty printer work with
> > > this patch. I should have something today or tomorrow, if you're ok with
> > > waiting that long.
> > Thanks for the answers! I think that it wouldn't be that hard for us to
> > update the pretty printers if we have some test coverage and documentation
> > for it. For now, is there any person/group we should ping if we suspect
> > that we break the pretty printers? I'll wait a few days. It's not that
> > important to land this patch soon.
> The situation with pretty printers has been a source of frustration for the 4
> years I've worked on libc++. I have been reaching out to various LLDB folks
> to get help setting up pre-commit CI for the LLDB pretty-printers in libc++'s
> own pipeline so that we can detect breakages in advance, but this has not
> been conclusive so far.
>
> @labath @jgorbe Would you be willing to help us set up a CI job that runs the
> LLDB pretty printers (and only that) in our pre-commit CI infrastructure? We
> have the machines and all the infrastructure in place. We just need the right
> CMake + `lit` invocations. Also CC @Mordante , since he had been
> investigating that IIRC.
>
> If we could notice breakages in advance, we could fix the pretty printers in
> the same patch where we make a change to libc++. We could call out for help
> from LLDB folks when needed. This would be a much smoother experience for
> everyone -- we would not need to revert our patches ever, and the LLDB folks
> would not be broken by changes that come out of the blue (as far as they are
> concerned).
I've indeed been working on that, but not managed yet. I can build lldb, but
the dataformatter tests return `UNSUPPORTED`. I haven't had time to investigate
this further. I hope to find some time soon. But if @labath or @jgorbe have
hints how to do this I would be interested to know. Alternatively what's the
best way to contact you Discourse or Discord?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123580/new/
https://reviews.llvm.org/D123580
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
