Improve optional formatter

[wallace]

Summary

As suggested by @labath in https://reviews.llvm.org/D114403, we should
make the formatter more resilient to corrupted data. The Libcxx version
explicitly checks for engaged = 1, so we can do that as well for safety.

[labath]

I didn't realize that _M_engaged has type bool. That means the max value this could have is 255, which isn't nearly as bad as 0xffffffff. Nonetheless, this is a good change.

That said, I'm more worried about the strange interactions between libc++ and libstdc++ formatters I reported.

lldb/examples/synthetic/gnu_libstdcpp.py

24

I'd use != 0, as matches what the actual implementation would do in this case.

[wallace]

That said, I'm more worried about the strange interactions between libc++ and libstdc++ formatters I reported.

yes, I'm trying to install libc++ on my machine now

lldb/examples/synthetic/gnu_libstdcpp.py

24

If that's true, then I'd need to update the libcxx formatter. My understanding is that setting something to true is always compiled as set to 1, or am I wrong?

[labath]

In D114450#3149281, @wallace wrote:

That said, I'm more worried about the strange interactions between libc++ and libstdc++ formatters I reported.

yes, I'm trying to install libc++ on my machine now

Cool, thanks.

lldb/examples/synthetic/gnu_libstdcpp.py

24

You are correct. However, _checking_ whether something is true is normally implemented as != 0.
This doesn't make a difference for the normal case, but in the "corrupted" case it might be a bit confusing that the program will report the optional object as "engaged", but we will say it's empty. It's not a big deal though.

I also wouldn't say you need to change the libc++ formatter too (though I won't stop you from doing it). If you really care about keeping the two in sync, I'd recommend making a unified implementation for the two (that might also probably fix the ordering problem).

[wallace]

fix the issue when both formatters run together and apply labath's suggestion

[labath]

thanks

lldb/examples/synthetic/gnu_libstdcpp.py

 from __future__ import division
-import re
 import lldb.formatters.Logger
 
 # C++ STL formatters for LLDB
 # These formatters are based upon the version of the GNU libstdc++
 # as it ships with Mac OS X 10.6.8 thru 10.8.0
 # You are encouraged to look at the STL implementation for your platform
 # before relying on these formatters to do the right thing for your setup
 
 def StdOptionalSummaryProvider(valobj, dict):
     has_value = valobj.GetNumChildren() > 0
     # We add wrapping spaces for consistency with the libcxx formatter
     return " Has Value=" + ("true" if has_value else "false") + " "
 
 
 class StdOptionalSynthProvider:
     def __init__(self, valobj, dict):
         self.valobj = valobj
 
     def update(self):
         try:
             self.payload = self.valobj.GetChildMemberWithName('_M_payload')
             self.value = self.payload.GetChildMemberWithName('_M_payload')
-            self.count = self.payload.GetChildMemberWithName('_M_engaged').GetValueAsUnsigned(0)
+            self.has_value = self.payload.GetChildMemberWithName('_M_engaged').GetValueAsUnsigned(0) != 0
         except:
-            self.count = 0
+            self.has_value = False
         return False
 
 
     def num_children(self):
-        return self.count
+        return 1 if self.has_value else 0
 
     def get_child_index(self, name):
         return 0
 
     def get_child_at_index(self, index):
         # some versions of libstdcpp have an additional _M_value child with the actual value
         possible_value = self.value.GetChildMemberWithName('_M_value')
         if possible_value.IsValid():

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

ValueObjectSP engaged_sp(
       m_backend.GetChildMemberWithName(ConstString("__engaged_"), true));
 
   if (!engaged_sp)
     return false;
 
   // __engaged_ is a bool flag and is true if the optional contains a value.
   // Converting it to unsigned gives us a size of 1 if it contains a value
   // and 0 if not.
-  m_has_value = engaged_sp->GetValueAsUnsigned(0) == 1;
+  m_has_value = engaged_sp->GetValueAsUnsigned(0) != 0;
 
   return false;
 }
 
 ValueObjectSP OptionalFrontEnd::GetChildAtIndex(size_t idx) {
   if (!m_has_value)
     return ValueObjectSP();
 

lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/optional/TestDataFormatterGenericOptional.py

 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 USE_LIBSTDCPP = "USE_LIBSTDCPP"
 USE_LIBCPP = "USE_LIBCPP"
 
 class GenericOptionalDataFormatterTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     def do_test_with_run_command(self, stdlib_type):
         """Test that that file and class static variables display correctly."""
+        # 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)
+        self.addTearDownHook(cleanup)
+
         self.build(dictionary={stdlib_type: "1"})
         self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
 
         bkpt = self.target().FindBreakpointByID(
             lldbutil.run_break_set_by_source_regexp(
                 self, "break here"))
 
         self.runCmd("run", RUN_SUCCEEDED)