labath added inline comments.
================
Comment at: libcxx/utils/gdb/libcxx/printers.py:192
class StdStringPrinter(object):
"""Print a std::string."""
----------------
Mordante wrote:
> 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?
I'm sorry about the delay. The lldb-libc++ integration is broken in several
ways (different ways on different platforms), so I'm not all that surprised
that it's not working for you. I don't really consider the data formatters my
responsibility so I only go near them when I really have to. Still, I agree
that this is not a good situation to be in, and the offer of offloading the
data formatter work to the libc++ team is definitely appealing. So, I'll try to
find some time to make that happen. I'm sorry I can't promise anything specific.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123580/new/
https://reviews.llvm.org/D123580
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
