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

[Raphael Isemann via Phabricator via lldb-commits]

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