[lldb] Document the behaviour of IsValid for SBError (#170862)

This reverts commit d20d84f.

Fixes #169788, but in a different way.

In which I changed an SBError use so that when the function succeeded,
IsValid on the SBError would be true.

This seemed to make sense but SBError acts differently to other SB
classes in this respect. For something like SBMemoryRegionInfo, if
IsValid() is false, you can't do anything with it.

However for SBError, IsValid() true only means there's some underlying
error object in there. If the SBError represents a success, there's no
need to put anything in there.

You can see this intent from a lot of its methods, many have handling
for IsValid() false.

This is not a bug but a misunderstanding of what IsValid means. Yes it
does function the way I expected for most classes, but it does not for
SBError and though that's not intuitive, it is consistent with how we
describe IsValid in the documentation.

So instead of changing that method's use of SBError I'm documenting this
initially counterintuitive behaviour in the SBError header and on the
website (https://lldb.llvm.org/resources/sbapi.html).

Author: DavidSpickett

lldb/docs/resources/sbapi.rst

@@ -46,6 +46,17 @@ prepared to handle their opaque implementation pointer being empty, and doing
 something reasonable. We also always have an "IsValid" method on all the SB
 classes to report whether the object is empty or not.
 
+.. note::
+  The implication of an object being "empty" can vary by class.
+
+  For most classes, the lack of anything backing the class means that it
+  would not be valid to interact with it by calling any other methods
+  on it.
+
+  One exception to this is ``SBError``, which can provide valid
+  information even when empty. This is because it does not need an
+  underlying object to be able to represent a success state.
+
 Another piece of the SB API infrastructure is the Python (or other script
 interpreter) customization. SWIG allows you to add property access, iterators
 and documentation to classes. We place the property accessors and iterators in

lldb/include/lldb/API/SBError.h

@@ -72,6 +72,15 @@ class LLDB_API SBError {
 
   explicit operator bool() const;
 
+  /// \brief Returns \c true if this object contains an underlying \c Status
+  /// object.
+  ///
+  /// That object may represent a success or a failure. When \c IsValid returns
+  /// \c false, it may be the case that the \c SBError represents a success but
+  /// does not contain a \c Status representing that success.
+  ///
+  /// It is safe to call \c Success or \c Fail in the case where \c IsValid
+  /// returns \c false.
   bool IsValid() const;
 
   bool GetDescription(lldb::SBStream &description);

lldb/source/API/SBDebugger.cpp

@@ -179,7 +179,7 @@ void SBDebugger::Initialize() {
 lldb::SBError SBDebugger::InitializeWithErrorHandling() {
   LLDB_INSTRUMENT();
 
-  SBError error((Status()));
+  SBError error;
   if (auto e = g_debugger_lifetime->Initialize(
           std::make_unique<SystemInitializerFull>())) {
     error.SetError(Status::FromError(std::move(e)));

lldb/unittests/DAP/TestBase.cpp

@@ -73,7 +73,6 @@ void DAPTestBase::TearDown() {
 
 void DAPTestBase::SetUpTestSuite() {
   lldb::SBError error = SBDebugger::InitializeWithErrorHandling();
-  EXPECT_TRUE(error.IsValid());
   EXPECT_TRUE(error.Success());
 }
 void DAPTestBase::TeatUpTestSuite() { SBDebugger::Terminate(); }