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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.

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

> I've just discovered that `LLDB_INVALID_*_ID` isn't -1/UINT_MAX as I thought, so I've had to introduce additional constants. That said, I'm not convinced about the current 0/-1 API. Maybe it'd make sense to handle them directly in `ReadTid()` and return some (current?) thread ID for 0, and `LLDB_INVALID_*_ID` for -1?

I think that decision which thread to use for the "any" thread should not be handled at such a low level, as it can have real impact on how the inferior behaves. I think using constants for this is fine. Although I applaud the avoidance of macros, I think the constants should also use non-macro nomenclature. Maybe just put the names inside the StringExtractorGdbRemote class, and call them `AnyThread` (etc.) ? Also, I think that these days we can just use constexpr variables instead of the enum hack.



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


================
Comment at: lldb/unittests/Utility/StringExtractorGDBRemoteTest.cpp:8-10
+namespace {
+class StringExtractorGDBRemoteTest : public ::testing::Test {};
+} // namespace
----------------
For empty fixtures, you can just drop this, along with the _F in the TEST_F macro.


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

https://reviews.llvm.org/D98482