Implement new methods for handling an error in MI commands.

[apolyakov]

Summary

The new methods take SBError object and call handler, specified by user, depending on SBError status.

[apolyakov]

This approach is quite simple and can be used in -exec-..., -target-... and future commands.

Also, I've been thinking about another approach with having a method in CMICmdBase that takes two parameters: pointers to a functions in which user could specify needed actions. But the main problem is that we don't have a knowledge about these functions, they may have any possible signatures. I don't yet know is it possible to have a pointer to a function with variable number of arguments of unknown types.

[aprantl]

tools/lldb-mi/MICmdBase.cpp

221

It returns a bool, right?

At some point it sure would be nice if we could convert the lldb-mi sources over to the LLVM coding style ...

[aprantl]

Also, I've been thinking about another approach with having a method in CMICmdBase that takes two parameters: pointers to a functions in which user could specify needed actions. But the main problem is that we don't have a knowledge about these functions, they may have any possible signatures. I don't yet know is it possible to have a pointer to a function with variable number of arguments of unknown types.

Could you post an example of two use-cases where you would need different number of arguments?

[apolyakov]

tools/lldb-mi/MICmdBase.cpp

221

Yep, it does. I'll update this.

Is there some code in this commit that doesn't correspond to LLVM coding style?

[aprantl]

tools/lldb-mi/MICmdBase.cpp

221

I was referring to the variable naming and documentation schema, which is very different from what the rest of the code uses. You are doing the right thing here, you're matching the new code to whatever style is used by the file.

[apolyakov]

I got an idea how to deal with different number of parameters: what if we suggest to user to create own lambda functions(where he can specify needed actions) without parameters, but with capturing required variables by reference or by value? auto f = [&x1, &x2...]{do some stuff}. Looks weird, but...

[aprantl]

Can you post a more concrete example? I think this sounds like a good idea.

[apolyakov]

Sure. For example we may look at bool CMICmdCmdExecContinue::Execute(), there we may see following code:

if (error.Success()) {
    // CODETAG_DEBUG_SESSION_RUNNING_PROG_RECEIVED_SIGINT_PAUSE_PROGRAM
    if (!CMIDriver::Instance().SetDriverStateRunningDebugging()) {
      const CMIUtilString &rErrMsg(CMIDriver::Instance().GetErrorDescription());
      SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_SET_NEW_DRIVER_STATE),
                                     m_cmdData.strMiCmd.c_str(),
                                     rErrMsg.c_str()));
      return MIstatus::failure;
    }
    return MIstatus::success;
  }

instead of this we may do something like

auto SetDriverStateRunning = [this] {
    if (!CMIDriver::Instance().SetDriverStateRunningDebugging()) {
      const CMIUtilString &rErrMsg(CMIDriver::Instance().GetErrorDescription());
      this->SetError(CMIUtilString::Format(
          MIRSRC(IDS_CMD_ERR_SET_NEW_DRIVER_STATE),
          this->m_cmdData.strMiCmd.c_str(),
          rErrMsg.c_str())
      );
    }
  };

SuccessHandler = SetDriverStateRunning;

and finally we will have a function CMICmdBase::FinishMICommand (or another name :D) which will be like:

bool CMICmdBase::FinishMICommand(const SBError &error) {
  if (error.Success()) {
    // call SucessHandler
    SucessHandler();
    return MIstatus::success;
  }

  // call ErrorHandler
  ErrorHandler();
  SetError(error.GetCString());
  return MIstatus::failure;
}

Of course, there will be some default values of SuccessHandler and ErrorHandler(some dummy function).

[apolyakov]

SuccessHandler and ErrorHandler will be a member variables of CMICmdBase.

[aprantl]

That sounds like a good overall direction, though I probably wouldn't communicate the function pointers via member variables but rather prefer them to be passed as function arguments or return values. This makes the flow a little more obvious for readers.

[apolyakov]

Attached another realization of CMICmdBase::ReturnMIStatus. It uses lambda functions to get user defined actions.

It works, but, as you may see, I had to define auto error_handler = ... in Execute function since in C++ we can't do something like return ReturnMIStatus(success_handler,, error).

Base on this, I see a few options:

1. keep it in current state;
2. use llvm::Optional instead of default arguments. We still will be needed to pass all three parameters, but in this case user could do following: return ReturnMIStatus(success_handler, llvm::Optional::None, error).
3. go back to simple implementation of ReturnMIStatus:

bool CMICmdBase::ReturnMIStatus(const SBError &error) {
  if (error.Success())
    return MIstatus::success;

  SetError(error.GetCString());
  return MIstatus::failure;
}

4. discard this idea and keep duplicating code;

[aprantl]

In this design the success_handlers return an exit status *and* set a string error. We could unify this by having the handler return an llvm::Error (https://llvm.org/doxygen/classllvm_1_1Error.html). When it is successful, it returns Error::success, otherwise it returns llvm::make_error<llvm::StringError>("message", llvm::inconvertibleErrorCode). In ReturnMIStatus we do something like

auto status = handler(...)
bool exit_status =  MIstatus::success;
handleAllErrors(status.takeError(), [&](const StringError &error) {
  his->SetError(error.getMessage();
  exit_status =  error_handler();
});
return exit_status;

[apolyakov]

In D48295#1136663, @aprantl wrote:

In this design the success_handlers return an exit status *and* set a string error. We could unify this by having the handler return an llvm::Error (https://llvm.org/doxygen/classllvm_1_1Error.html). When it is successful, it returns Error::success, otherwise it returns llvm::make_error<llvm::StringError>("message", llvm::inconvertibleErrorCode). In ReturnMIStatus we do something like

auto status = handler(...)
bool exit_status =  MIstatus::success;
handleAllErrors(status.takeError(), [&](const StringError &error) {
  his->SetError(error.getMessage();
  exit_status =  error_handler();
});
return exit_status;

I don't completely understand what you mean. First of all, what do you mean when talking about success_handlers? 'cause if you mean success_handler from Execute function, then I should say that it doesn't have to set an error, it might be any. Secondly, status in your example is a function, how can it has a takeError method?

[aprantl]

In D48295#1136692, @apolyakov wrote:

I don't completely understand what you mean. First of all, what do you mean when talking about success_handlers? 'cause if you mean success_handler from Execute function, then I should say that it doesn't have to set an error, it might be any. Secondly, status in your example is a function, how can it has a takeError method?

status is meant to be a bool. I think success_handler was a bad name. It should probably be called action or command something similar. It would be the actual implementation of the command, such as lambda that is assigned to success_handler in your patch.

[apolyakov]

Sorry, but I still don't understand you. In my thoughts, user specify actions for success and/or error cases. Success case means that out SBError objects' status is success(sb_error.Success() == true). Error case means similar except SBError object has fail status. So if SBError has success status we should call success_handler, specified by user. I don't see similar logic in your pseudo code. Could you please explain your idea more exactly?

[aprantl]

I think I misread your patch. Now the naming of success_handler makes much more sense, too.

What do you think about defining a function that takes an SBError result, and a function that converts this error into a string? This would allow chaining more than one SBAPI command in one lldb-mi command implementation. I'm not sure how common this is, though.

bool handleSBError(SBError error, std::function<std::string(SBError)> convert) {
  if (error.Success())
    return false;

  SetError(convert(error));
  return error_handler();
}

...
bool CMICmdCmdExecContinue::Execute() {
  if (handleSBError(CMICmnLLDBDebugSessionInfo::Instance().GetProcess().Continue(), [](SBError error){
        return CMIUtilString::Format(
           MIRSRC(IDS_CMD_ERR_SET_NEW_DRIVER_STATE),
           this->m_cmdData.strMiCmd.c_str(),
           rErrMsg.c_str());'
       }) 
    return MIstatus::failure;

  // potentially run other SBAPI commands...
  return MIstatus::success;
};

[apolyakov]

bool handleSBError(SBError error, std::function<std::string(SBError)> convert) {
  if (error.Success())
    return false;

  SetError(convert(error));
  return error_handler();
}

...
bool CMICmdCmdExecContinue::Execute() {
  if (handleSBError(CMICmnLLDBDebugSessionInfo::Instance().GetProcess().Continue(), [](SBError error){
        return CMIUtilString::Format(
           MIRSRC(IDS_CMD_ERR_SET_NEW_DRIVER_STATE),
           this->m_cmdData.strMiCmd.c_str(),
           rErrMsg.c_str());'
       }) 
    return MIstatus::failure;

  // potentially run other SBAPI commands...
  return MIstatus::success;
};

In this case, it's not clear how to pass error_handler to handleSBError(...). Moreover, user may want to specify action for success case which isn't presented in your example.

About using more than one SB API command: it's possible even in my patch, just use ReturnMIStatus like:

if (!ReturnMIStatus(success_handler, error_handler, error))
  return MIstatus::failure;

// do other stuff
...
return MIstatus::success;

[aprantl]

Ok, then let's continue this way.

tools/lldb-mi/MICmdBase.h

89

If you move the error argument to the front, you can use the default error handler in the function below. In fact, I'm not even sure what it would mean to call this function without an error. I guess the error argument should probably not have a default value at all.

tools/lldb-mi/MICmdCmdExec.cpp

242 ↗

(On Diff #151906)

If you add a second variant (or a wrapper) that takes a bool instead of an SBError, you could use that one here, since it's basically the same pattern, just not with an SBError.

[apolyakov]

Changed method's name from ReturnMIStatus to HandleSBError. Also added one more use case(see -exec-step's Execute function).

The only thing that worries me is that if we want to specify handler for error case, we should do something like:

SBError error = ...
auto errorHandler = ...

return HandleSBError(error, [] { return MIstatus::success; }, errorHandler);

since in C++ we can't skip first optional argument and only pass second one.

[aprantl]

In D48295#1137595, @apolyakov wrote:

Changed method's name from ReturnMIStatus to HandleSBError. Also added one more use case(see -exec-step's Execute function).

The only thing that worries me is that if we want to specify handler for error case, we should do something like:

SBError error = ...
auto errorHandler = ...

return HandleSBError(error, [] { return MIstatus::success; }, errorHandler);

since in C++ we can't skip first optional argument and only pass second one.

Does it ever make sense to return something different than MIstatus::failure from the failure handler? Since HandleSBError sets the Error string, probably not. So we could change the error handler to a const std::function<void()>.

Then you can define the following overloads:

bool CMICmdBase::HandleSBError(lldb::SBError error, const std::function<bool()> successHandler = [] { return MIstatus::success; }, const std::function<void()> errorHandler = [] { return MIstatus::failure; });
bool CMICmdBase::HandleSBError(lldb::SBError error, const std::function<void()> errorHandler);
// this one is for the second half of Execute()
bool CMICmdBase::HandleMIstatus(bool status, const std::function<void()> errorHandler = [] { return MIstatus::success; });

By the way, I don't think it makes sense to pass any of the arguments by reference. Both SBError and std::function should be very cheap to pass by value.

[apolyakov]

With such overloads we'll get compile time error. If we have overload:

bool HandleSBError(SBError error, std::function<bool()> successHandler = [] {some func}, std::function<void()> errorHandler = [] {some func});
bool HandleSBError(SBError error, std::function<void()> errorHandler);

and call it like:

auto successCase = [] {
  ...
  return MIstatus::failure;
}

bool status = HandleSBError(error, successCase);

we get compile error: call of HandleSBError is ambiguous.

[apolyakov]

By the way, I do not see a solution better than current patch, but, at the same time, I do not like current patch. Maybe we should discard this idea?

[aprantl]

I guess you *could* use different function names, such as HandleSBError, HandleSBErrorWithSuccess, HandleSBErrorWithSuccessAndFailure, ...

[apolyakov]

Splitted functionality between a few functions with different names. If this patch is good, I'll remove [WIP] status and usage examples (in MICmdCmdExec.cpp file).

[aprantl]

I think that works. Assuming that we can use these new helpers in lots of other places :-)

[apolyakov]

Removed usage examples. Also I wanted to ask if someone knows what (R) in Args - ... (R) from MI commands documentation means?

[aprantl]

Perhaps it's supposed to mark a return argument? The lldb-mi tool uses a (compared to the rest of the project) very odd Windows-like coding style that I'm not particularly familiar with. It would be nice to convert all of it over to just use the plain LLVM style so it is consistent with the rest.

tools/lldb-mi/MICmdBase.h

 //===-- MICmdBase.h ---------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #pragma once
 
 // C Includes
 // C++ Includes
 // Other libraries and framework includes
+#include "lldb/API/SBError.h"
+
 // Project includes
 #include "MICmdArgSet.h"
 #include "MICmdData.h"
 #include "MICmdFactory.h"
 #include "MICmdInvoker.h"
 #include "MICmnBase.h"
 #include "MICmnMIResultRecord.h"
 #include "MICmnResources.h"
public:
 
   // Overrideable:
   ~CMICmdBase() override;
   virtual bool GetExitAppOnCommandFailure() const;
 
   // Methods:
 protected:
   void SetError(const CMIUtilString &rErrMsg);
+  bool HandleSBError(const lldb::SBError &error,
+                     const std::function<bool()> &successHandler =
+                     [] { return MIstatus::success; },
+                     const std::function<void()> &errorHandler = [] {});
+  bool HandleSBErrorWithSuccess(const lldb::SBError &error,
+                                const std::function<bool()> &successHandler);
+  bool HandleSBErrorWithFailure(const lldb::SBError &error,
+                                const std::function<void()> &errorHandler);
   template <class T> T *GetOption(const CMIUtilString &vStrOptionName);
   bool ParseValidateCmdOptions();
 
   // Attributes:
   CMICmdFactory::CmdCreatorFnPtr m_pSelfCreatorFn;
   CMIUtilString m_strCurrentErrDescription; // Reason for Execute or Acknowledge
                                             // function failure
   SMICmdData m_cmdData; // Holds information/status of *this command. Used by

tools/lldb-mi/MICmdBase.cpp

const CMICmnMIResultRecord miResultRecord(
       m_cmdData.strMiCmdToken, CMICmnMIResultRecord::eResultClass_Error,
       valueResult);
   m_miResultRecord = miResultRecord;
   m_cmdData.strMiCmdResultRecord = miResultRecord.GetString();
 }
 
 //++
 //------------------------------------------------------------------------------------
+// Details: Short cut function to check MI command's execute status and
+//          set an error in case of failure.
+// Type:    Method.
+// Args:    error - (R) Error description object.
+//          successHandler - (R) function describing actions to execute
+//          in case of success state of passed SBError object.
+//          errorHandler - (R) function describing actions to execute
+//          in case of fail status of passed SBError object.
+// Return:  bool.
+// Throws:  None.
+//--
+bool CMICmdBase::HandleSBError(const lldb::SBError &error,
+                               const std::function<bool()> &successHandler,
+                               const std::function<void()> &errorHandler) {
+  if (error.Success())
+    return successHandler();
+
+  SetError(error.GetCString());
+  errorHandler();
+  return MIstatus::failure;
+}
+
+//++
+//------------------------------------------------------------------------------------
+// Details: Short cut function to check MI command's execute status and
+//          call specified handler function for success case.
+// Type:    Method.
+// Args:    error - (R) Error description object.
+//          successHandler - (R) function describing actions to execute
+//          in case of success state of passed SBError object.
+// Return:  bool.
+// Throws:  None.
+//--
+bool CMICmdBase::HandleSBErrorWithSuccess(
+    const lldb::SBError &error,
+    const std::function<bool()> &successHandler) {
+  return HandleSBError(error, successHandler);
+}
+
+//++
+//------------------------------------------------------------------------------------
+// Details: Short cut function to check MI command's execute status and
+//          call specified handler function for error case.
+// Type:    Method.
+// Args:    error - (R) Error description object.
+//          errorHandler - (R) function describing actions to execute
+//          in case of fail status of passed SBError object.
+// Return:  bool.
+// Throws:  None.
+//--
+bool CMICmdBase::HandleSBErrorWithFailure(
+    const lldb::SBError &error,
+    const std::function<void()> &errorHandler) {
+  return HandleSBError(error, [] { return MIstatus::success; }, errorHandler);
+}
+
+//++
+//------------------------------------------------------------------------------------
 // Details: Ask a command to provide its unique identifier.
 // Type:    Method.
 // Args:    A unique identifier for this command class.
 // Return:  None.
 // Throws:  None.
 //--
 MIuint CMICmdBase::GetGUID() {
   MIuint64 vptr = reinterpret_cast<MIuint64>(this);