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

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to