Fix error handling in the libcxx std::string formatter

[jingham]

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.

[JDevlieghere]

LGTM module formatting.

[shafik]

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]

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]

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]

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.

[dblaikie]

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

102

Ah, that's a pity. Oh well.

[jingham]

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]

In D108228#2961132, @jingham wrote:

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.

lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp

enum LibcxxStringLayoutMode {
   eLibcxxStringLayoutModeInvalid = 0xffff
 };
 
 /// Determine the size in bytes of \p valobj (a libc++ std::string object) and
 /// extract its data payload. Return the size + payload pair.
 // TODO: Support big-endian architectures.
 static llvm::Optional<std::pair<uint64_t, ValueObjectSP>>
 ExtractLibcxxStringInfo(ValueObject &valobj) {
-  ValueObjectSP D(valobj.GetChildAtIndexPath({0, 0, 0, 0}));
-  if (!D)
+  ValueObjectSP dataval_sp(valobj.GetChildAtIndexPath({0, 0, 0, 0}));
+  if (!dataval_sp)
+    return {};
+  if (!dataval_sp->GetError().Success())
     return {};
 
   ValueObjectSP layout_decider(
-      D->GetChildAtIndexPath(llvm::ArrayRef<size_t>({0, 0})));
+      dataval_sp->GetChildAtIndexPath(llvm::ArrayRef<size_t>({0, 0})));
 
   // this child should exist
   if (!layout_decider)
     return {};
 
   ConstString g_data_name("__data_");
   ConstString g_size_name("__size_");
   bool short_mode = false; // this means the string is in short-mode and the
                            // data is stored inline
   bool using_bitmasks = true; // Whether the class uses bitmasks for the mode
                               // flag (pre-D123580).
   uint64_t size;
   LibcxxStringLayoutMode layout = (layout_decider->GetName() == g_data_name)
                                       ? eLibcxxStringLayoutModeDSC
                                       : eLibcxxStringLayoutModeCSD;
   uint64_t size_mode_value = 0;
 
-  if (ValueObjectSP is_long = D->GetChildAtNamePath(
+  if (ValueObjectSP is_long = dataval_sp->GetChildAtNamePath(
           {ConstString("__s"), ConstString("__is_long_")})) {
     using_bitmasks = false;
     short_mode = !is_long->GetValueAsUnsigned(/*fail_value=*/0);
     if (ValueObjectSP size_member =
-            D->GetChildAtNamePath({ConstString("__s"), ConstString("__size_")}))
+            dataval_sp->GetChildAtNamePath({ConstString("__s"), ConstString("__size_")}))
       size = size_member->GetValueAsUnsigned(/*fail_value=*/0);
     else
       return {};
   } else if (layout == eLibcxxStringLayoutModeDSC) {
     llvm::SmallVector<size_t, 3> size_mode_locations[] = {
         {1, 2}, // Post-c3d0205ee771 layout. This was in use for only a brief
                 // period, so we can delete it if it becomes a burden.
         {1, 1, 0},
         {1, 1, 1},
     };
     ValueObjectSP size_mode;
     for (llvm::ArrayRef<size_t> loc : size_mode_locations) {
-      size_mode = D->GetChildAtIndexPath(loc);
+      size_mode = dataval_sp->GetChildAtIndexPath(loc);
       if (size_mode && size_mode->GetName() == g_size_name)
         break;
     }
 
     if (!size_mode)
       return {};
 
     size_mode_value = (size_mode->GetValueAsUnsigned(0));
     short_mode = ((size_mode_value & 0x80) == 0);
   } else {
-    ValueObjectSP size_mode(D->GetChildAtIndexPath({1, 0, 0}));
+    ValueObjectSP size_mode(dataval_sp->GetChildAtIndexPath({1, 0, 0}));
     if (!size_mode)
       return {};
 
     size_mode_value = (size_mode->GetValueAsUnsigned(0));
     short_mode = ((size_mode_value & 1) == 0);
   }
 
   if (short_mode) {
-    ValueObjectSP s(D->GetChildAtIndex(1, true));
-    if (!s)
+    ValueObjectSP short_sp(dataval_sp->GetChildAtIndex(1, true));
+    if (!short_sp)
       return {};
-    ValueObjectSP location_sp = s->GetChildAtIndex(
+    ValueObjectSP location_sp = short_sp->GetChildAtIndex(
         (layout == eLibcxxStringLayoutModeDSC) ? 0 : 1, true);
     if (using_bitmasks)
       size = (layout == eLibcxxStringLayoutModeDSC)
                  ? size_mode_value
                  : ((size_mode_value >> 1) % 256);
 
     // When the small-string optimization takes place, the data must fit in the
     // inline string buffer (23 bytes on x86_64/Darwin). If it doesn't, it's
     // likely that the string isn't initialized and we're reading garbage.
     ExecutionContext exe_ctx(location_sp->GetExecutionContextRef());
     const llvm::Optional<uint64_t> max_bytes =
         location_sp->GetCompilerType().GetByteSize(
             exe_ctx.GetBestExecutionContextScope());
     if (!max_bytes || size > *max_bytes || !location_sp)
       return {};
 
     return std::make_pair(size, location_sp);
   }
 
-  ValueObjectSP l(D->GetChildAtIndex(0, true));
+  ValueObjectSP l(dataval_sp->GetChildAtIndex(0, true));
   if (!l)
     return {};
   // we can use the layout_decider object as the data pointer
   ValueObjectSP location_sp =
       l->GetChildMemberWithName(ConstString("__data_"), /*can_create=*/true);
   ValueObjectSP size_vo =
       l->GetChildMemberWithName(ConstString("__size_"), /*can_create=*/true);
   ValueObjectSP capacity_vo =

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

 class LibcxxStringDataFormatterTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     def setUp(self):
         # Call super's setUp().
         TestBase.setUp(self)
         # Find the line number to break at.
-        self.line = line_number('main.cpp', '// Set break point at this line.')
+        self.main_spec = lldb.SBFileSpec("main.cpp")
         self.namespace = 'std'
 
     @add_test_categories(["libc++"])
     @expectedFailureAll(bugnumber="llvm.org/pr36109", debug_info="gmodules", triple=".*-android")
     # Inline namespace is randomly ignored as Clang due to broken lookup inside
     # the std namespace.
     @expectedFailureAll(debug_info="gmodules")
     def test_with_run_command(self):
         """Test that that file and class static variables display correctly."""
         self.build()
-        self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
 
-        lldbutil.run_break_set_by_file_and_line(
-            self, "main.cpp", self.line, num_expected_locations=-1)
-
-        self.runCmd("run", RUN_SUCCEEDED)
-
-        # The stop reason of the thread should be breakpoint.
-        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
-                    substrs=['stopped',
-                             'stop reason = breakpoint'])
+        (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
+                                                                            "Set break point at this line.",
+                                                                            self.main_spec)
+        frame = thread.frames[0]
 
         # This is the function to remove the custom formats in order to have a
         # clean slate for the next test case.
         def cleanup():
             self.runCmd('type format clear', check=False)
             self.runCmd('type summary clear', check=False)
             self.runCmd('type filter clear', check=False)
             self.runCmd('type synth clear', check=False)
def test_with_run_command(self):
                 '(%s::u32string) u32_string = U"🍄🍅🍆🍌"'%ns,
                 # FIXME: This should have a 'U' prefix.
                 '(%s::u32string) u32_empty = ""'%ns,
                 '(%s::basic_string<unsigned char, %s::char_traits<unsigned char>, '
                 '%s::allocator<unsigned char> >) uchar = "aaaaa"'%(ns,ns,ns),
                 '(%s::string *) null_str = nullptr'%ns,
         ])
 
-        self.runCmd("n")
+        thread.StepOver()
 
-        TheVeryLongOne = self.frame().FindVariable("TheVeryLongOne")
+        TheVeryLongOne = frame.FindVariable("TheVeryLongOne")
         summaryOptions = lldb.SBTypeSummaryOptions()
         summaryOptions.SetCapping(lldb.eTypeSummaryUncapped)
         uncappedSummaryStream = lldb.SBStream()
         TheVeryLongOne.GetSummary(uncappedSummaryStream, summaryOptions)
         uncappedSummary = uncappedSummaryStream.GetData()
         self.assertTrue(uncappedSummary.find("someText") > 0,
                         "uncappedSummary does not include the full string")
         summaryOptions.SetCapping(lldb.eTypeSummaryCapped)
def test_with_run_command(self):
         # The test assumes that std::string is in its cap-size-data layout.
         is_alternate_layout = ('arm' in self.getArchitecture()) and self.platformIsDarwin()
         if is_64_bit and not is_alternate_layout:
             self.expect("frame variable garbage1", substrs=['garbage1 = Summary Unavailable'])
             self.expect("frame variable garbage2", substrs=[r'garbage2 = "\xfa\xfa\xfa\xfa"'])
             self.expect("frame variable garbage3", substrs=[r'garbage3 = "\xf0\xf0"'])
             self.expect("frame variable garbage4", substrs=['garbage4 = Summary Unavailable'])
             self.expect("frame variable garbage5", substrs=['garbage5 = Summary Unavailable'])
+
+        # Finally, make sure that if the string is not readable, we give an error:
+        bkpt_2 = target.BreakpointCreateBySourceRegex("Break here to look at bad string", self.main_spec)
+        self.assertEqual(bkpt_2.GetNumLocations(), 1, "Got one location")
+        threads = lldbutil.continue_to_breakpoint(process, bkpt_2)
+        self.assertEqual(len(threads), 1, "Stopped at second breakpoint")
+        frame = threads[0].frames[0]
+        var = frame.FindVariable("in_str")
+        self.assertTrue(var.GetError().Success(), "Made variable")
+        summary = var.GetSummary()
+        self.assertEqual(summary, "Summary Unavailable", "No summary for bad value")
+        

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

 // A corrupt libcxx string in long mode with a payload that would trigger a
 // buffer overflow.
 static struct {
   uint64_t cap = 5;
   uint64_t size = 2;
   uint64_t data = 0xfffffffffffffffeULL;
 } garbage_string_long_mode4;
 
+size_t touch_string(std::string &in_str)
+{
+  return in_str.size(); // Break here to look at bad string
+}
+
 int main()
 {
     std::wstring wempty(L"");
     std::wstring s(L"hello world! מזל טוב!");
     std::wstring S(L"!!!!");
     const wchar_t *mazeltov = L"מזל טוב";
     std::string empty("");
     std::string q("hello world");
if (sizeof(std::string) == sizeof(garbage_string_long_mode3))
       memcpy((void *)&garbage4, &garbage_string_long_mode3, sizeof(std::string));
     if (sizeof(std::string) == sizeof(garbage_string_long_mode4))
       memcpy((void *)&garbage5, &garbage_string_long_mode4, sizeof(std::string));
 #else
 #error "Test potentially needs to be updated for a new std::string ABI."
 #endif
 
     S.assign(L"!!!!!"); // Set break point at this line.
+    std::string *not_a_string = (std::string *) 0x0;
+    touch_string(*not_a_string);
     return 0;
 }