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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
