[libcxx-commits] [PATCH] D123580: [libc++] Use bit field for checking if string is in long or short mode

Thu Apr 28 08:08:32 PDT 2022

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