[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil marked 3 inline comments as done.
jankratochvil added a comment.

In D67589#1670966 <https://reviews.llvm.org/D67589#1670966>, @labath wrote:

> I agree that this approach isn't nice, but probably there isn't a nice approach to this at this point. One possibility you could consider is storing a shared_ptr<CommandRO> inside SBCommandRO and encoding the ownership into the deleter of the shared_ptr (regular deleter => owned, noop deleter => unowned).


Maybe also possibly cheaper `llvm::PointerIntPair` representing an associated `bool` for the deletion.



================
Comment at: lldb/packages/Python/lldbsuite/test/api/command-return-object/main.cpp:18
+    result = subcommand(dbg, "help");
+    result = result;
+    if (!result.Succeeded())
----------------
labath wrote:
> Is that intentional? If so, why?
It has no purpose there for this bugfix but when already writing a testcase I wanted to test this generally fragile functionality.



================
Comment at: lldb/source/API/SBCommandInterpreter.cpp:165-172
+    SBCommandReturnObject sb_return;
+    std::swap(result, SBCommandReturnObject_ref(sb_return));
     SBCommandInterpreter sb_interpreter(&m_interpreter);
     SBDebugger debugger_sb(m_interpreter.GetDebugger().shared_from_this());
     bool ret = m_backend->DoExecute(
         debugger_sb, (char **)command.GetArgumentVector(), sb_return);
+    std::swap(result, SBCommandReturnObject_ref(sb_return));
----------------
clayborg wrote:
> Could this code just create a local SBCommandReturnObject and then copy the CommandReturnObject back into "result"?
> 
> ```
>  bool DoExecute(Args &command, CommandReturnObject &result) override {
>     SBCommandReturnObject sb_return;
>     SBCommandInterpreter sb_interpreter(&m_interpreter);
>     SBDebugger debugger_sb(m_interpreter.GetDebugger().shared_from_this());
>     bool ret = m_backend->DoExecute(
>         debugger_sb, (char **)command.GetArgumentVector(), sb_return);
>     std::swap(result, sb_return.ref());
>     return ret;
> }
> ```
I made it that way first but it breaks the testsuite. For example due to:
```
lldb/source/Commands/CommandObjectCommands.cpp:
1236	  bool DoExecute(llvm::StringRef raw_command_line,
1237	                 CommandReturnObject &result) override {
...
1242	    result.SetStatus(eReturnStatusInvalid);
 - ctored CommandReturnObject has eReturnStatusStarted.
1244	    if (!scripter ||
1245	        !scripter->RunScriptBasedCommand(m_function_name.c_str(),
1246	                                         raw_command_line, m_synchro, result,
1247	                                         error, m_exe_ctx)) {
1248	      result.AppendError(error.AsCString());
1249	      result.SetStatus(eReturnStatusFailed);
1250	    } else {
1251	      // Don't change the status if the command already set it...
1252	      if (result.GetStatus() == eReturnStatusInvalid) {
1253	        if (result.GetOutputData().empty())
1254	          result.SetStatus(eReturnStatusSuccessFinishNoResult);
1255	        else
1256	          result.SetStatus(eReturnStatusSuccessFinishResult);
1257	      }
1258	    }
```
So it would be possible but that would require refactorization more of the code in callers which I find outside of the scope of this patch.
This patch tries to make this bugfix fully transparent to existing code.



Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67589/new/

https://reviews.llvm.org/D67589