[PATCH] D47415: [lldb, lldb-mi] Re-implement MI -exec-continue command.

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
Comment at: tools/lldb-mi/MICmdCmdExec.cpp:248
     }
-  } else {
-    // ToDo: Re-evaluate if this is required when application near finished as
-    // this is parsing LLDB error message
-    // which seems a hack and is code brittle
-    const char *pLldbErr = m_lldbResult.GetError();
-    const CMIUtilString strLldbMsg(CMIUtilString(pLldbErr).StripCREndOfLine());
-    if (strLldbMsg == "error: Process must be launched.") {
-      CMIDriver::Instance().SetExitApplicationFlag(true);
-    }
-  }
+  } else m_lldbResult.SetError(error.GetCString());
 
----------------
apolyakov wrote:
> labath wrote:
> > polyakov.alex wrote:
> > > clayborg wrote:
> > > > clayborg wrote:
> > > > > clayborg wrote:
> > > > > > polyakov.alex wrote:
> > > > > > > clayborg wrote:
> > > > > > > > An error can claim to fail but not have an error string set. It might be nice to have a helper function that checks to make sure there is an error string on error cases and if there is no error string when error.Success() is false or error.Fail() is true, then set the error string to "unknown error". 
> > > > > > > This functionality might be useful in all lldb-mi commands. So do you know where to place this function? Maybe inside SBError class?
> > > > > > I would put it somewhere in lldb-mi in a static function that is something like:
> > > > > > ```
> > > > > > static void SetErrorString(lldb::SBError &error, T &lldbResult) {
> > > > > >   const char *error_cstr = error.GetCString();
> > > > > >   if (error_cstr)
> > > > > >     lldbResult.SetError(error.GetCString());
> > > > > >   else
> > > > > >     lldbResult.SetError("unknown error");
> > > > > > }
> > > > > > ```
> > > > > > Where the T is the type of m_lldbResult. 
> > > > > Actually, are we using m_lldbResult now? I didn't realize its type was lldb::SBCommandReturnObject. That was only needed if we were calling:
> > > > > ```
> > > > > rSessionInfo.GetDebugger().GetCommandInterpreter().HandleCommand(pCmd, m_lldbResult);
> > > > > ```
> > > > > 
> > > > > So we can get rid of all "lldb::SBCommandReturnObject m_lldbResult" member variables in any lldb-mi functions where we switch to using the API.
> > > > Since we can get rid of m_lldbResult, this should be:
> > > > 
> > > > ```
> > > > } else return MIstatus::failure;
> > > > ```
> > > It should be:
> > > ```
> > > else {
> > >   SetError(CMIUtilString::Format(..., error.GetCString()));
> > >   return MIstatus::failure;
> > > }
> > > ```
> > > So, we anyway need a C-style error string.
> > AFAICT, `SBError::GetCString` calls `Status::AsCString` which has a default argument `const char *default_error_str = "unknown error"`. So it sounds like this issue is already taken care of down below. If we are still getting null strings for failed SBErrors, maybe we need to fix something else instead of adding another layer here.
> `SBError::GetCString` potentially may return NULL:
> ```
> const char *SBError::GetCString() const {
>   if (m_opaque_ap.get())
>     return m_opaque_ap->AsCString();
>   return NULL;
> }
> ```
Yes, but that can happen only in case `SBError::Fail()` is false, which means you probably shouldn't be calling `GetCString` in the first place.


https://reviews.llvm.org/D47415