[Lldb-commits] [PATCH] D62931: [lldb-server] Add setting to force 'g' packet use

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D62931#1724433 <https://reviews.llvm.org/D62931#1724433>, @guiandrade wrote:

> In D62931#1719948 <https://reviews.llvm.org/D62931#1719948>, @labath wrote:
>
> > I'm sorry, this dropped off my radar. The code looks fine, but it could use some more tests. For instance, one test when you set the setting value to the non-default , and then check that lldb does _not_ use the `g` packet .
>
>
> Yeah, more tests would be useful. They made me notice an issue btw. I was simply sending a 'g' packet and checking if the server replied back nicely; however, IIUC with 'G' packets it isn't so simple because we also need to send the registers data in the same packet. I'm not sure how I could do that, and I'm also afraid that check could get too expensive. Do you have any idea what could be a nice way to handle that?


Well... I'd expect that a stub which does not support the `G` packet at all would return an "unsupported" response no matter what data you send it, whereas a stub which supports the `G` packet would return an "error" response to an incorrect `G` packet. But yeah, testing for the availability of packets which mutate the state is a bit tricky...

In D62931#1724451 <https://reviews.llvm.org/D62931#1724451>, @jasonmolenda wrote:

> fwiw I can't imagine a stub that supports g but not G.


well.. lldb-server is in that bucket right now. :) However, one could definitely argue that this behavior is wrong and we shouldn't support that.  However, that still leaves the question of what exactly should the `use-g-packet` setting do. In the previous version of this patch, it controlled both `g` and `G`. I think that was mostly an accident, because it was implemented by hooking into the code which was trying to support stubs which did not understand the `p`/`P` packet. But as the reason we're introducing it is performance, I think it'd make sense to separate these out, because writing is a much less common operation, and I expect wanting to write _all_ registers would be very rare. OTOH, wanting to _read_ all registers is not that uncommon, particularly as our stubs now expedite the most common registers, which means that the fact that one is attempting to read one of the non-expedited registers is a pretty good indication that he's going to read more than one.

So, one possibility would be to just have one setting (use-g-packet-for-reading), but make it so that it does not control the usage of the `G` packet -- the logic for that could stay as `if (p_supported) send_P() else send_G()`, while for reading we'd have something like `if (p_supported && !g_requested) send_p() else send_g();`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62931