Page MenuHomePhabricator

Implement new methods for handling an error in MI commands.
ClosedPublic

Authored by apolyakov on Jun 18 2018, 1:05 PM.

Diff Detail

Event Timeline

apolyakov created this revision.Jun 18 2018, 1:05 PM

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 accepted this revision.Jun 18 2018, 1:59 PM
aprantl added inline comments.
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 ...

This revision is now accepted and ready to land.Jun 18 2018, 1:59 PM

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 added inline comments.Jun 18 2018, 2:04 PM
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 added inline comments.Jun 18 2018, 2:27 PM
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 added a comment.EditedJun 18 2018, 2:38 PM

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...

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

apolyakov added a comment.EditedJun 18 2018, 3:35 PM

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).

SuccessHandler and ErrorHandler will be a member variables of CMICmdBase.

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.

clayborg accepted this revision.Jun 18 2018, 5:44 PM
apolyakov updated this revision to Diff 151906.Jun 19 2018, 6:17 AM

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;
}
  1. discard this idea and keep duplicating code;

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;

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?

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.

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?

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;
};
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;

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

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 updated this revision to Diff 152069.Jun 20 2018, 5:29 AM

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.

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.

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.

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?

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

apolyakov updated this revision to Diff 152310.Jun 21 2018, 8:20 AM
apolyakov retitled this revision from [WIP] Implement new ReturnMIStatus method of CMICmdBase class. to [WIP] Implement new methods for handling an error in MI commands..
apolyakov edited the summary of this revision. (Show Details)

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 accepted this revision.Jun 21 2018, 9:00 AM

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

apolyakov retitled this revision from [WIP] Implement new methods for handling an error in MI commands. to Implement new methods for handling an error in MI commands..

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

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.

This revision was automatically updated to reflect the committed changes.