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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
