[Lldb-commits] [PATCH] D98482: [lldb] Support for multiprocess extension

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

This should be fine, assuming the following statement is true: "all thread id's that we're passing from server to client are in the form of some lldb-specific extension to the gdb-remote protocol". If that is not the case, then we should also update the client to work with the new format.

In D98482#2631986 <https://reviews.llvm.org/D98482#2631986>, @mgorny wrote:

> In D98482#2631817 <https://reviews.llvm.org/D98482#2631817>, @labath wrote:
>
>> In D98482#2626237 <https://reviews.llvm.org/D98482#2626237>, @mgorny wrote:
>>
>>> Create a new `GDBRemoteError` class to pass gdb-remote `$E` codes through cleanly.
>>
>> The error codes we use right now are completely meaningless, so don't bother preserving them. I don't think we should be introducing a separate error class on their account, and I'm particularly unhappy about how this class has insinuated itself into the Status object.
>
> Well, I found the ability to use different codes useful for debugging tests but I guess it doesn't matter much. So a plain `StringError` then?

Yeah. Note we also have the ability to send the error messages through the protocol nowadays. I'm not sure if the lldb-server test suite enables them, but we could certainly change that.



================
Comment at: lldb/source/Utility/StringExtractorGDBRemote.cpp:625
+      uint64_t prev_index = m_index;
+      pid = GetHexMaxU64(false, 0);
+      if (m_index == prev_index || m_index == UINT64_MAX) {
----------------
mgorny wrote:
> labath wrote:
> > This should be something like `view.consumeInteger(16, pid)`. At that point you can stop keeping m_index in sync, and just update it at the very end (maybe even via llvm::scope_exit).
> Do you have some specific method of updating it mind? Comparing size before and after?
StringExtractor::GetNameColonValue does it by subtracting the .data() pointers, but this could be even better...


================
Comment at: lldb/unittests/Utility/StringExtractorGDBRemoteTest.cpp:142-145
+  ex.Reset("p-1.1234");
+  EXPECT_THAT(
+      ex.GetPidTid(100).getValue(),
+      ::testing::Pair(StringExtractorGDBRemote::AllProcesses, 0x1234ULL));
----------------
IIRC gdb docs say this is invalid, so I'd reject it immediately at this level.


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

https://reviews.llvm.org/D98482