teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.
You could land all the formatting changes as its own commit just to make it
clear that this is adding one `if (error)` and the other line changes are
formatting updates. But I don't have a problem with keeping the formatting
changes in this commit.
Could we minimize the test changes to:
std::string &null_str_ref = *null_str; // (null_str is already a variable in
the test).
# No summary for null reference
self.expect_var_path("null_str_ref", summary='"Summary Unavailable"')
There is no `expect_var` yet, so we have to stick to the expr/var path variant
at the moment. But this would avoid all the breakpoint setting, continuing,
variable fetching and manual error handling (it would also print the error when
fetching the variable if we run into one),
Beside that this LGTM.
================
Comment at:
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py:134
+ var = frame.FindVariable("in_str")
+ self.assertTrue(var.GetError().Success(), "Made variable")
+ summary = var.GetSummary()
----------------
Side note as this could all be replaced with the one line check below, but
Pavel added is `assertSuccess` that checks the same thing but also prints out
the actual error message.
================
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;
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D108228/new/
https://reviews.llvm.org/D108228
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
