[Lldb-commits] [PATCH] D124113: [lldb] Adjust libc++ string formatter for changes in D123580

Thu Apr 21 05:06:54 PDT 2022

labath added a comment.

In D124113#3464287 <https://reviews.llvm.org/D124113#3464287>, @philnik wrote:

> LGTM assuming you checked that it actually works with my patch applied. Do you want to land your patch first, or should we do it the other way around?

I've confirmed this with the real patch and with my magic string simulator. For me, it would be better if this patch went in first (so I guess I'll land it now).

================
Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp:658
+  if (!using_bitmasks)
+    capacity *= 2;
   if (size == LLDB_INVALID_OFFSET || capacity == LLDB_INVALID_OFFSET ||
----------------
philnik wrote:
> labath wrote:
> > philnik wrote:
> > > This should only be done if the string is in the normal layout and little endian or in the alternate layout and big endian. Why do you care about the capacity at all? Isn't that just another point of failure?
> > I've added the layout check and made a note that big-endian is not supported. The capacity check comes from <https://reviews.llvm.org/D73860>, but I'm not sure of the overall purpose. I guess it was trying to screen out corrupt/uninitialized string objects, but I'm not convinced that we're doing anyone a favour by refusing to format those -- after all, the application itself might access such objects without checking the capacity, and operate on the data that we've decided to ignore.
> Maybe it would make sense to say that the string is uninitialized or corrupted when the `__invariants()` fail? That would probably catch more bugs, but I don't know if you can just run a member function in the pretty printer.
Technically, we can run functions from pretty-printers, but we try hard to avoid that. If calling functions was ok, we could just call `data()` and get rid of this mess. :)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124113/new/

https://reviews.llvm.org/D124113