This is an archive of the discontinued LLVM Phabricator instance.

Fix error handling in the libcxx std::string formatter
ClosedPublic

Authored by jingham on Aug 17 2021, 11:39 AM.

Details

Summary

The formatter tries to get the data field of the std::string, and to check whether that fails it just checks that the ValueObjectSP returned is not empty. But we never return empty ValueObjectSP's to indicate failure, since doing so would lose the Error object that tells you why fetching the ValueObject failed.

This patch adds a check for ValueObject::GetError().Success().

I also added a test case for this failure, and reworked the test case a bit (to use run_to_source_breakpoint). I also renamed a couple of single letter locals which don't follow the lldb coding conventions.

Diff Detail

Event Timeline

jingham requested review of this revision.Aug 17 2021, 11:39 AM
jingham created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2021, 11:39 AM
JDevlieghere accepted this revision.Aug 17 2021, 8:08 PM

LGTM module formatting.

This revision is now accepted and ready to land.Aug 17 2021, 8:08 PM
shafik added a subscriber: shafik.Aug 17 2021, 10:06 PM
shafik added inline comments.
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp
102

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

JDevlieghere added inline comments.Aug 17 2021, 10:24 PM
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp
102

Definitely UB, but the sanitized bot builds LLDB with the sanitizers, not the test cases, so this should be "fine".

dblaikie added inline comments.
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp
101

This cast isn't needed, right? This could be rewritten more traditionally as:

std::string &not_a_string = nullptr;

?

102

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?

teemperor requested changes to this revision.Aug 18 2021, 4:00 AM
teemperor added a subscriber: teemperor.

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.

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py
134

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.

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp
102

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.

This revision now requires changes to proceed.Aug 18 2021, 4:00 AM
dblaikie added inline comments.Aug 18 2021, 2:48 PM
lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp
102

Ah, that's a pity. Oh well.

jingham added a comment.EditedAug 23 2021, 2:29 PM

Raphael's analysis of what the test needs is right. We always check pointers for validity before we do operations on them, so we wouldn't have tried to get the summary, and the expression evaluation will just crash if we do * of a null ptr, so there wouldn't be anything to format.

The reason I passed the string reference to a function and check it there is because from many years of experience, I don't trust compilers even at -O0 not to elide references to silly unused variables. But at -O0 a function argument is never going away.

But if folks think this is over paranoid on my part, I can simplify the test.

teemperor accepted this revision.Oct 4 2021, 5:20 AM

Raphael's analysis of what the test needs is right. We always check pointers for validity before we do operations on them, so we wouldn't have tried to get the summary, and the expression evaluation will just crash if we do * of a null ptr, so there wouldn't be anything to format.

The reason I passed the string reference to a function and check it there is because from many years of experience, I don't trust compilers even at -O0 not to elide references to silly unused variables. But at -O0 a function argument is never going away.

But if folks think this is over paranoid on my part, I can simplify the test.

I don't have a strong objection against the extra step and so on, so LGTM.

This revision is now accepted and ready to land.Oct 4 2021, 5:20 AM
This revision was landed with ongoing or failed builds.May 17 2022, 8:22 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 8:22 AM