[Lldb-commits] [PATCH] D108228: Fix error handling in the libcxx std::string formatter

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.


================
Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp:102
+    std::string *not_a_string = (std::string *) 0x0;
+    touch_string(*not_a_string);
     return 0;
----------------
teemperor wrote:
> dblaikie wrote:
> > JDevlieghere wrote:
> > > shafik wrote:
> > > > This is undefined behavior and I AFAICT this won't pass a sanitized build, amazingly even if I use `__attribute__((no_sanitize("address", "undefined")))` : https://godbolt.org/z/4TGPbrYhq
> > > Definitely UB, but the sanitized bot builds LLDB with the sanitizers, not the test cases, so this should be "fine". 
> > Seems best avoided if possible though, yeah? What's trying to be demonstrated by this test?
> > 
> > What if the function took a std::string* instead of std::string&, and the caller doesn't need to dereference that pointer - it could call some other, unrelated function to act as a stop-point for the debugger?
> > 
> > & then the "printing a bad string" Could be tested by printing an expression, like "p *str" that dereferences in the expression?
> > 
> > Or is the issue only present through the auto-printing of variables in parameters in a stack trace, and not present when using the user-defined expression evaluator?
> > This is undefined behavior and I AFAICT this won't pass a sanitized build, amazingly even if I use __attribute__((no_sanitize("address", "undefined"))) : https://godbolt.org/z/4TGPbrYhq
> 
> This is just a segfault unrelated to sanitizers.
> 
> > & then the "printing a bad string" Could be tested by printing an expression, like "p *str" that dereferences in the expression?
> > Or is the issue only present through the auto-printing of variables in parameters in a stack trace, and not present when using the user-defined expression evaluator?
> 
> The expression evaluator is already erroring out when printing that variable (or doing the `*str` equivalent) so I believe this is just about directly accessing the variables. The same goes for situations like `frame var *str` which already handle the error that we now also check for here. I *believe* we really need a the null reference as in the current test so that we end up in a situation where the formatter has to handle the "parent is null" error itself in this code path because of a null reference.
Ah, that's a pity. Oh well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108228