philnik added inline comments.
================
Comment at: libcxx/utils/gdb/libcxx/printers.py:192
class StdStringPrinter(object):
"""Print a std::string."""
----------------
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.
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
