Mordante added inline comments.
================
Comment at: libcxx/utils/gdb/libcxx/printers.py:192
class StdStringPrinter(object):
"""Print a std::string."""
----------------
labath wrote:
> 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.
Thanks for the information. The libc++ CI uses a Ubuntu 20.04 Docker image on
amd64.
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
