[Lldb-commits] [PATCH] D79614: Fix error reporting for qLaunchSuccess, check for fix/enable it via qSupported

Mon May 11 19:27:46 PDT 2020

jasonmolenda added a comment.

Thanks for the feedback.

In D79614#2029157 <https://reviews.llvm.org/D79614#2029157>, @labath wrote:

> I think that "piggy-backing" on the `qSupported` packet for communicating protocol fixes is a good idea. However, I agree with Greg, that it does not seem like it's needed for this case. Fixing the problem purely on the debugserver side seems preferable, as it avoids adding compatibility code to lldb. It's true that it's a one-off error reporting mechanism, but it's already there, so it seems better to just support what's already implemented then try to support two mechanisms.
>
> For a long-term solution, I am wondering whether we need `qLaunchSuccess` at all? It seems like the packet is completely redundant in a world where we can return textual error messages to _any_ packet. What was the reason for introducing it in the first place? Could we just switch to using error replies to the `A` packet for communicating the launch errors (with some transition plan for supporting `qLaunchSuccess` for a while)?

It's hard to tell what we were thinking (GREG :) back when we used the A packet to set the binary name / arguments /etc, and then the qLaunchSuccess packet to start the process.  Reading over the current era gdb Remote Serial Protocol docs, I think the vRun packet does the combination of these two things in one packet, which makes sense.

I tested a patch using Greg's suggestion of using the binary escape protocol for the error string and that does work, as expected, I'll land the patch like that.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79614