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

mgorny added a comment. Ok, I'm going to use this review to push the server part, then commit the uint64 client change separately, then create a new diff for client. If you find a few more minutes, the server-side fork/vfork patch is ready for review (and independent of this one). CHANGES

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

labath added a comment. I think it'd be better to commit this in two patches after all. The first one could include the server bits (and the multiprocess+ server thingy) -- I believe this is good to go already. The second patch/review could deal with the thread/process id business on the

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

mgorny added inline comments. Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:741 if (response.GetChar() == 'C') { - m_curr_pid = response.GetHexMaxU32(false, LLDB_INVALID_PROCESS_ID); + m_curr_pid =

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

mgorny updated this revision to Diff 332324. mgorny added a comment. try uploading again CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98482/new/ https://reviews.llvm.org/D98482 Files: lldb/include/lldb/Utility/StringExtractorGDBRemote.h

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

mgorny updated this revision to Diff 332322. mgorny added a comment. Added client support for PIDs. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98482/new/ https://reviews.llvm.org/D98482 Files: lldb/include/lldb/Utility/StringExtractorGDBRemote.h

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

labath added a comment. In D98482#2641041 , @mgorny wrote: > In D98482#2640981 , @labath wrote: > >> This should be fine, assuming the following statement is true: "all thread >> id's that we're passing from

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

mgorny updated this revision to Diff 332255. mgorny edited the summary of this revision. mgorny added a comment. Move 0/-1 value checking down to StringExtractorGDBRemote. Reject 0 unconditionally, since we do not use it. TODO: client support CHANGES SINCE LAST ACTION

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

mgorny added a comment. In D98482#2640981 , @labath wrote: > 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

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

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

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

mgorny updated this revision to Diff 331327. mgorny marked 2 inline comments as done. mgorny added a comment. - switched thread-id parsing to use `view.consumeInteger()` and replaced `m_index` manipulations with a single update at the very end - added additional test for parsing multiple

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

mgorny added a comment. In D98482#2631817 , @labath wrote: > In D98482#2626237 , @mgorny wrote: > >> Create a new `GDBRemoteError` class to pass gdb-remote `$E` codes through >> cleanly. > > The error codes we use

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

labath added a comment. In 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

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

mgorny updated this revision to Diff 331220. mgorny edited the summary of this revision. mgorny added a comment. 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.

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

mgorny updated this revision to Diff 330674. mgorny added a comment. clang-format. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98482/new/ https://reviews.llvm.org/D98482 Files: lldb/include/lldb/Utility/GDBRemoteError.h lldb/include/lldb/Utility/StringExtractorGDBRemote.h

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

mgorny updated this revision to Diff 330671. mgorny edited the summary of this revision. mgorny added a comment. Updated `GetPidTid()` to use `llvm::Optional>` return type with a default PID approach as suggested by @labath. Updated `ReadTid()` to use `llvm::Expected`. Create a new

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

mgorny added a comment. Or scratch that. What you propose is cleaner and requires less changes to tests ;-). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98482/new/ https://reviews.llvm.org/D98482 ___ lldb-commits mailing list

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

mgorny added a comment. In D98482#2624973 , @labath wrote: > I'm still bugged by the GetPidTid interface -- the pair of optionals is very > complicated. What if we had this function take a `default_pid` argument > (which would be equal to the currently

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

labath added a comment. I'm still bugged by the GetPidTid interface -- the pair of optionals is very complicated. What if we had this function take a `default_pid` argument (which would be equal to the currently selected process), and it returned that as the process id, in case the packet does

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

mgorny updated this revision to Diff 330441. mgorny added a comment. Added support for 0 and -1 values in `ReadTid()` helper, controlled via `allow_any` and `allow_all` parameters. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98482/new/ https://reviews.llvm.org/D98482 Files:

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

mgorny updated this revision to Diff 330435. mgorny added a comment. Add a `GDBRemoteCommunicationServerLLGS::ReadTid()` helper that does appropriate PID verification and constructs error packets. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98482/new/ https://reviews.llvm.org/D98482

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

mgorny added a comment. In D98482#2623005 , @labath wrote: > - I wouldn't add `multiprocess` to the qSupported packets just yet. Since > this is accepting the extended syntax regardless of whether the other side > claimed to support it (there's no hard

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

mgorny updated this revision to Diff 330432. mgorny marked an inline comment as done. mgorny added a comment. Updated utility function to use `StringRef` and reformatted. Further changes incoming. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98482/new/ https://reviews.llvm.org/D98482

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

labath added a comment. Some initial remarks: - the parsing function is clearly gdb-related, so it should go into StringExtractorGDBRemote.h, and not its base class - the universalness of the function makes its interface pretty complicated, and bloats the call sites. Its probably good for the

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

mgorny updated this revision to Diff 330225. mgorny edited the summary of this revision. mgorny added a comment. Updated to include `multiprocess` in `qSupported`, and proposed how to handle it server-side on the example of `H` packet. Basically, accept the correct PID of current process,

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

mgorny created this revision. mgorny added reviewers: labath, emaste, krytarowski. mgorny requested review of this revision. This is a work-in-progress to provide minimal support for multiprocess GDB protocol extension. For a start, a new function to parse thread-ids accounting for the possible