Page MenuHomePhabricator

mgorny (Michał Górny)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 15 2016, 6:00 AM (243 w, 1 d)

Recent Activity

Today

mgorny added a comment to D100191: [lldb] [llgs] Support owning and detaching extra processes.

@labath, if we're going to remove m_debugged_process_up, then I suppose it makes sense to merge D100256 first. Could you review that one, please?

Tue, Apr 13, 6:03 AM
mgorny updated the diff for D100153: [lldb] [Process] Introduce protocol extension support API.

Rebase + clang-format.

Tue, Apr 13, 6:00 AM
mgorny committed rGc8d18cba4e2f: Reland "[lldb] [Process] Watch for fork/vfork notifications" for Linux (authored by mgorny).
Reland "[lldb] [Process] Watch for fork/vfork notifications" for Linux
Tue, Apr 13, 5:47 AM
mgorny committed rG7da3b44d67f8: Reland "[lldb] [Process] Watch for fork/vfork notifications" for NetBSD (authored by mgorny).
Reland "[lldb] [Process] Watch for fork/vfork notifications" for NetBSD
Tue, Apr 13, 5:36 AM
mgorny added inline comments to D98822: [lldb] [Process] Watch for fork/vfork notifications.
Tue, Apr 13, 5:35 AM · Restricted Project
mgorny added inline comments to D100191: [lldb] [llgs] Support owning and detaching extra processes.
Tue, Apr 13, 5:02 AM
mgorny committed rG63d75641054a: Reland "[lldb] [Process] Watch for fork/vfork notifications" for FreeBSD (authored by mgorny).
Reland "[lldb] [Process] Watch for fork/vfork notifications" for FreeBSD
Tue, Apr 13, 4:20 AM
mgorny added a comment to D100191: [lldb] [llgs] Support owning and detaching extra processes.

I like this a lot more than the previous version. The thing I'd like to know is, whether we can replace m_additional_processes with something like m_debugged_processes (i.e., just have a single list of all processes we are debugging. We could replace all occurrences of m_debugged_process_up with m_current_process, which just points inside that list. Is there any reason for the "privileged" position of the original process? The way I see it, most of the operations would just work even if we treated them as equals. The only thing which might not work is resuming (or TBE, reporting the stops) of the subprocesses, but if we wanted to be safe, we could prevent that in some other way (preventing the changing of process through Hc; returning "crippled" process instances for subprocess, which return errors on resume operations; ...).

WDYT?

Tue, Apr 13, 3:14 AM
mgorny committed rGaab81c2f40d2: [lldb] [gdb-remote server] Refactor handling qSupported (authored by mgorny).
[lldb] [gdb-remote server] Refactor handling qSupported
Tue, Apr 13, 3:14 AM
mgorny closed D100140: [lldb] [gdb-remote server] Refactor handling qSupported.
Tue, Apr 13, 3:14 AM · Restricted Project
mgorny added a comment to D98822: [lldb] [Process] Watch for fork/vfork notifications.

I've reverted this (121cff78a8032a73aa4fb820625dc1ecae8e3997) because it made the linux bot super-flaky. I think it's likely that this affects _only_ linux (and the BSD code is fine -- though we don't have a bot to verify that), but I didn't want to experiment with partial reverts while the bots are wonky. However, it may be interesting the re-land the linux support in a separate commit, once we figure out the problem -- my money is on the nondeterminism of thread creation.

Tue, Apr 13, 3:04 AM · Restricted Project
mgorny added inline comments to D100153: [lldb] [Process] Introduce protocol extension support API.
Tue, Apr 13, 2:45 AM
mgorny added inline comments to D100140: [lldb] [gdb-remote server] Refactor handling qSupported.
Tue, Apr 13, 2:38 AM · Restricted Project
mgorny committed rGff31af4f55af: [lldb] [gdb-remote client] Refactor handling qSupported (authored by mgorny).
[lldb] [gdb-remote client] Refactor handling qSupported
Tue, Apr 13, 2:21 AM
mgorny added inline comments to D100146: [lldb] [gdb-remote client] Refactor handling qSupported.
Tue, Apr 13, 2:19 AM · Restricted Project
mgorny added a comment to D100146: [lldb] [gdb-remote client] Refactor handling qSupported.

looks great, just fix the build errors :)

Tue, Apr 13, 12:45 AM · Restricted Project

Yesterday

mgorny committed rG3842de49f655: [lldb] [gdb-remote client] Refactor handling qSupported (authored by mgorny).
[lldb] [gdb-remote client] Refactor handling qSupported
Mon, Apr 12, 3:23 PM
mgorny closed D100146: [lldb] [gdb-remote client] Refactor handling qSupported.
Mon, Apr 12, 3:23 PM · Restricted Project
mgorny updated the diff for D100140: [lldb] [gdb-remote server] Refactor handling qSupported.

clang-format

Mon, Apr 12, 3:21 PM · Restricted Project
mgorny added inline comments to D100140: [lldb] [gdb-remote server] Refactor handling qSupported.
Mon, Apr 12, 5:14 AM · Restricted Project
mgorny updated the diff for D100140: [lldb] [gdb-remote server] Refactor handling qSupported.

Fix missing QListThreadsInStopReply+.

Mon, Apr 12, 5:14 AM · Restricted Project

Sun, Apr 11

mgorny requested review of D100267: [lldb] [gdb-remote client] Remove breakpoints throughout vfork.
Sun, Apr 11, 8:42 AM
mgorny updated the diff for D100208: [lldb] [Process/Linux] Report fork/vfork stop reason to client.

Report vfork-done stop to the client as well.

Sun, Apr 11, 8:38 AM
mgorny updated the diff for D100206: [lldb] [llgs client] Support minimal fork/vfork handling.

Base class Did*Fork() definitions have been moved to the earlier patch.

Sun, Apr 11, 8:37 AM
mgorny updated the diff for D100196: [lldb] Introduce new stop reasons for fork and vfork.

Add 'vforkdone' stop reason. Move overrideable actions to this patch.

Sun, Apr 11, 8:35 AM
mgorny abandoned D99864: [lldb] Fork/vfork support via gdb-remote protocol [WIP].

Let's close this as all the features are now part of other diffs.

Sun, Apr 11, 5:08 AM
mgorny added a comment to D100263: [lldb] [gdb-remote client] Remove breakpoints in forked processes.

(TODO: vfork support)

Sun, Apr 11, 5:05 AM
mgorny requested review of D100263: [lldb] [gdb-remote client] Remove breakpoints in forked processes.
Sun, Apr 11, 5:05 AM
mgorny requested review of D100262: [lldb] [gdb-remote client] Support switching PID along with TID.
Sun, Apr 11, 5:04 AM
mgorny requested review of D100261: [lldb] [gdb-remote server] Support selecting process via Hg.
Sun, Apr 11, 5:03 AM
mgorny updated the diff for D100208: [lldb] [Process/Linux] Report fork/vfork stop reason to client.

Copy software breakpoints to the forked process, to future-proof this patch for breakpoint support (I suppose there's no point in splitting it more).

Sun, Apr 11, 4:40 AM
mgorny requested review of D100256: [lldb] [gdb-remote server] Abstract away getting current process.
Sun, Apr 11, 2:40 AM

Fri, Apr 9

mgorny added a comment to D100208: [lldb] [Process/Linux] Report fork/vfork stop reason to client.

Ok, found the culprit. Now it's ready ;-).

Fri, Apr 9, 10:45 AM
mgorny retitled D100208: [lldb] [Process/Linux] Report fork/vfork stop reason to client from [lldb] [Process/Linux] Report fork/vfork stop reason to client [WIP] to [lldb] [Process/Linux] Report fork/vfork stop reason to client.
Fri, Apr 9, 10:45 AM
mgorny updated the diff for D100153: [lldb] [Process] Introduce protocol extension support API.

Fix uninitialized flags variable in GDBRemoteCommunicationServerLLGS::SetEnabledExtensions().

Fri, Apr 9, 10:44 AM
mgorny added a comment to D100208: [lldb] [Process/Linux] Report fork/vfork stop reason to client.

This is the final piece for moving minimal fork/vfork support to gdb-remote protocol. However, for some reason (unlike the original code in D99864), it crashes on random assertions about m_enabled_extensions value — probably memory corruption somewhere. I'm debugging it.

Fri, Apr 9, 8:19 AM
mgorny requested review of D100208: [lldb] [Process/Linux] Report fork/vfork stop reason to client.
Fri, Apr 9, 8:17 AM
mgorny requested review of D100206: [lldb] [llgs client] Support minimal fork/vfork handling.
Fri, Apr 9, 8:13 AM
mgorny added a comment to D99864: [lldb] Fork/vfork support via gdb-remote protocol [WIP].

I've decided to take another approach for tracking forked processes. Rather than keeping them inside NativeProcess* and exposing via API to LLGS, I've decided to push them via NativeDelegate straight into LLGS and maintain there. This is now D100191.

Fri, Apr 9, 7:05 AM
mgorny updated the diff for D100191: [lldb] [llgs] Support owning and detaching extra processes.

Permit more-than-one-char 'D' packets.

Fri, Apr 9, 7:03 AM
mgorny requested review of D100196: [lldb] Introduce new stop reasons for fork and vfork.
Fri, Apr 9, 7:01 AM
mgorny requested review of D100191: [lldb] [llgs] Support owning and detaching extra processes.
Fri, Apr 9, 6:27 AM

Thu, Apr 8

mgorny updated the diff for D100153: [lldb] [Process] Introduce protocol extension support API.

Add to _KNOWN_QSUPPORTED_STUB_FEATURES and clang-format.

Thu, Apr 8, 4:28 PM
mgorny added a comment to D100153: [lldb] [Process] Introduce protocol extension support API.

Note that I've included full fork-events+ and vfork-events+ as demo of the API. I can split them further and/or move to more relevant commits as I progress with the split.

Thu, Apr 8, 4:24 PM
mgorny added a comment to D99864: [lldb] Fork/vfork support via gdb-remote protocol [WIP].

And now moved Extension API to D100153. I've added an API to query features supported by platform plugin, so now server doesn't report them unless they're actually going to be used.

Thu, Apr 8, 4:23 PM
mgorny requested review of D100153: [lldb] [Process] Introduce protocol extension support API.
Thu, Apr 8, 4:21 PM
mgorny added a comment to D98822: [lldb] [Process] Watch for fork/vfork notifications.

test/Shell/Subprocess/vfork-follow-parent-wp.test is failing on GreenDragon:

http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/31016/testReport/junit/lldb-shell/Subprocess/vfork_follow_parent_wp_test/

I'm going to temporarily disable the test on Darwin. Happy to provide help to investigate :-)

Thu, Apr 8, 3:33 PM · Restricted Project
mgorny added a comment to D99864: [lldb] Fork/vfork support via gdb-remote protocol [WIP].

Client-side qSupported processing is now refactored in D100146.

Thu, Apr 8, 3:24 PM
mgorny requested review of D100146: [lldb] [gdb-remote client] Refactor handling qSupported.
Thu, Apr 8, 3:23 PM · Restricted Project
mgorny added a comment to D99864: [lldb] Fork/vfork support via gdb-remote protocol [WIP].

The server-side qSupported handling is now split into D100140, and all relevant remarks were addressed there.

Thu, Apr 8, 2:31 PM
mgorny added a comment to D100140: [lldb] [gdb-remote server] Refactor handling qSupported.

This is split from D99864 as a minimal independent change.

Thu, Apr 8, 2:30 PM · Restricted Project
mgorny requested review of D100140: [lldb] [gdb-remote server] Refactor handling qSupported.
Thu, Apr 8, 2:29 PM · Restricted Project
mgorny committed rGd01bff8cbdc9: [lldb] [test] Skip clone() tests on Linux/aarch64 (authored by mgorny).
[lldb] [test] Skip clone() tests on Linux/aarch64
Thu, Apr 8, 11:04 AM
mgorny added a comment to D99864: [lldb] Fork/vfork support via gdb-remote protocol [WIP].

I'm going to have more questions, but let me start with this: What should the (v)fork-events extension actually mean? In the client, I guess it demonstrates its willingness to receive the appropriate type of event. But what should it mean on the server? Is it like "I promise to stop on every (v)fork", or just something like "hey, I know this word"? The second meaning only makes sense if these (v)fork-events come with some new protocol extensions/packets that the client can use when communicating with the server. Such is the case with the multiprocess extension (new thread id syntax), but I am not aware of anything similar in for these features. Are there any? The gdb documentation (This feature indicates whether GDB supports fork event extensions to the remote protocol. GDB does not use such extensions unless the stub also reports that it supports them by including ‘fork-events+’ in its ‘qSupported’ reply. ) seems to indicate they might exist, but I am not sure what would they be...

If there aren't any, then it would probably be better to use the first "definition" of the feature (or something similar), as the client can legitimately be interested in whether it will be able to intercept forks or not. And if that's the case, then we should first consult the relevant NativeProcess class before we reply (v)fork-events+.

Thu, Apr 8, 10:07 AM
mgorny committed rGa345419ee030: [lldb] [Process] Watch for fork/vfork notifications (authored by mgorny).
[lldb] [Process] Watch for fork/vfork notifications
Thu, Apr 8, 9:50 AM
mgorny closed D98822: [lldb] [Process] Watch for fork/vfork notifications.
Thu, Apr 8, 9:49 AM · Restricted Project
mgorny updated the diff for D98822: [lldb] [Process] Watch for fork/vfork notifications.

Implemented @labath's requests.

Thu, Apr 8, 7:54 AM · Restricted Project
mgorny added a comment to D98822: [lldb] [Process] Watch for fork/vfork notifications.

Ok, I think I've implemented all the requested changes. I'm going to test it on all three platforms now and attach the patch if it miraculously still works ;-).

Thu, Apr 8, 5:38 AM · Restricted Project
mgorny added inline comments to D98822: [lldb] [Process] Watch for fork/vfork notifications.
Thu, Apr 8, 5:23 AM · Restricted Project
mgorny committed rGb601c6719226: [lldb] [client] Support for multiprocess extension (authored by mgorny).
[lldb] [client] Support for multiprocess extension
Thu, Apr 8, 4:46 AM
mgorny closed D99603: [lldb] [client] Support for multiprocess extension.
Thu, Apr 8, 4:46 AM · Restricted Project
mgorny added a comment to D98822: [lldb] [Process] Watch for fork/vfork notifications.

Answered where answer was due, will update the rest once I finish retesting the multiprocess patch.

Thu, Apr 8, 4:23 AM · Restricted Project
mgorny added a comment to D99603: [lldb] [client] Support for multiprocess extension.

@labath, any further comments or is this good to go then?

Thu, Apr 8, 3:55 AM · Restricted Project
mgorny updated the diff for D99603: [lldb] [client] Support for multiprocess extension.

Removed the hanging test.

Thu, Apr 8, 3:55 AM · Restricted Project

Wed, Apr 7

mgorny added inline comments to D99864: [lldb] Fork/vfork support via gdb-remote protocol [WIP].
Wed, Apr 7, 11:02 AM
mgorny updated the diff for D99864: [lldb] Fork/vfork support via gdb-remote protocol [WIP].

Added breakpoint cleanup via gdb-remote protocol. Only minimal code coverage so far. If this approach is okay, I guess I'll add subprocess guards to all server commands.

Wed, Apr 7, 2:39 AM

Sun, Apr 4

mgorny requested review of D99864: [lldb] Fork/vfork support via gdb-remote protocol [WIP].
Sun, Apr 4, 12:54 PM

Fri, Apr 2

mgorny added a comment to D98822: [lldb] [Process] Watch for fork/vfork notifications.

@labath, I think I've made all the requested changes.

Fri, Apr 2, 7:52 AM · Restricted Project
mgorny updated the diff for D98822: [lldb] [Process] Watch for fork/vfork notifications.

Simplified FreeBSD/NetBSD to use per-PID waitpid(). Removed arch restrictions from wp tests — made them consistent with Python tests.

Fri, Apr 2, 7:51 AM · Restricted Project
mgorny added inline comments to D98822: [lldb] [Process] Watch for fork/vfork notifications.
Fri, Apr 2, 7:11 AM · Restricted Project
mgorny updated the diff for D98822: [lldb] [Process] Watch for fork/vfork notifications.

Update the Linux plugin and tests, mostly.

Fri, Apr 2, 6:27 AM · Restricted Project
mgorny updated the diff for D99603: [lldb] [client] Support for multiprocess extension.

Added a skip to the hanging new test.

Fri, Apr 2, 3:02 AM · Restricted Project

Thu, Apr 1

mgorny added inline comments to D98822: [lldb] [Process] Watch for fork/vfork notifications.
Thu, Apr 1, 3:52 PM · Restricted Project
mgorny updated the diff for D99603: [lldb] [client] Support for multiprocess extension.

Add some tests.

Thu, Apr 1, 9:15 AM · Restricted Project
mgorny updated the diff for D99603: [lldb] [client] Support for multiprocess extension.

Changed GetCurrentProcessAndThreadIDs() to return the vector, and reformatted.

Thu, Apr 1, 7:37 AM · Restricted Project
mgorny committed rGfcea4181bbfb: [lldb] [test] Mark lldb-server multiprocess tests as LLGS cat (authored by mgorny).
[lldb] [test] Mark lldb-server multiprocess tests as LLGS cat
Thu, Apr 1, 5:18 AM

Wed, Mar 31

mgorny updated the diff for D99603: [lldb] [client] Support for multiprocess extension.

Added better PID mismatch handling in SetThreadStopInfo(). Not that most of the call sites actually check the return value...

Wed, Mar 31, 4:22 AM · Restricted Project

Tue, Mar 30

mgorny updated the summary of D98822: [lldb] [Process] Watch for fork/vfork notifications.
Tue, Mar 30, 4:09 PM · Restricted Project
mgorny updated the diff for D98822: [lldb] [Process] Watch for fork/vfork notifications.

Removed software breakpoint cleanup. Modified tests not to expect child's output at any specific point.

Tue, Mar 30, 4:09 PM · Restricted Project
mgorny added a comment to D98482: [lldb] [server] Support for multiprocess extension.

Looks like this broke the Windows lldb buildbot:

https://lab.llvm.org/buildbot/#/builders/83/builds/5173

Tue, Mar 30, 9:49 AM · Restricted Project
mgorny committed rGc62ef12079bc: [lldb] [test] Mark more lldb-server tests xfail on Windows (authored by mgorny).
[lldb] [test] Mark more lldb-server tests xfail on Windows
Tue, Mar 30, 9:49 AM
mgorny added a comment to D99603: [lldb] [client] Support for multiprocess extension.

@labath, split GetCurrentThreadIDs() as suggested. I've also modified GetCurrentProcessID() to prefer explicit PID over TID when available.

Tue, Mar 30, 9:19 AM · Restricted Project
mgorny requested review of D99603: [lldb] [client] Support for multiprocess extension.
Tue, Mar 30, 9:17 AM · Restricted Project
mgorny added a comment to D98822: [lldb] [Process] Watch for fork/vfork notifications.

Yeah, but then we have to maintain two copies of breakpoint-unsetting code in perpetuity. Given that we've managed to come this far with this being completely broken, I think we can wait a little longer until the client support is in place.

Tue, Mar 30, 8:13 AM · Restricted Project
mgorny added inline comments to D98822: [lldb] [Process] Watch for fork/vfork notifications.
Tue, Mar 30, 6:20 AM · Restricted Project
mgorny added a comment to D98822: [lldb] [Process] Watch for fork/vfork notifications.

It includes refactoring clone/fork/vfork event monitoring into a single function. Right now, only forks are supported — the handler creates a local NativeProcessLinux instance (@labath, any suggestions how to make this less hacky?), duplicates parent's breakpoints into it (just like the child gets parent's memory) and uses it to clear these breakpoints and detach the child process.

I don't find this particularly hacky. However, I have doubts about the breakpoint clearing aspect. If we add that now, I think it will be tricky to move that process to the client in the future (how will the client know whether the breakpoints were auto-removed?) Given that breakpoints in forked process have been borked since forever, I don't see harm in keeping that broken for a little while longer. Even a client with extremely limited of multiple processes (like the one we had in mind) should not have a problem with sending the appropriate z packets before it detaches. This patch could still come first (it's great that you've separated that out) -- it just needs to ensure that there are no breakpoints in the child path which would need clearing.

Tue, Mar 30, 6:16 AM · Restricted Project
mgorny committed rG6c1a8039de46: [lldb] [server] Support for multiprocess extension (authored by mgorny).
[lldb] [server] Support for multiprocess extension
Tue, Mar 30, 6:09 AM
mgorny closed D98482: [lldb] [server] Support for multiprocess extension.
Tue, Mar 30, 6:09 AM · Restricted Project
mgorny updated the diff for D98482: [lldb] [server] Support for multiprocess extension.

Limited to server changes. Added definitions for constexpr vars.

Tue, Mar 30, 6:09 AM · Restricted Project
mgorny committed rG64bb9cf7bf8d: [lldb] [Process/gdb-remote] Fix TID reading to use U64 (authored by mgorny).
[lldb] [Process/gdb-remote] Fix TID reading to use U64
Tue, Mar 30, 5:00 AM
mgorny added a comment to D98482: [lldb] [server] Support for multiprocess extension.

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.

Tue, Mar 30, 4:16 AM · Restricted Project

Mon, Mar 29

mgorny updated the diff for D98822: [lldb] [Process] Watch for fork/vfork notifications.

Fix wrongly using the debugged process' TID instead of PID.

Mon, Mar 29, 6:15 AM · Restricted Project

Sat, Mar 27

mgorny updated the diff for D98822: [lldb] [Process] Watch for fork/vfork notifications.

Complete FreeBSD and NetBSD support. Includes a fix to Detach() on NetBSD.

Sat, Mar 27, 7:00 AM · Restricted Project

Fri, Mar 26

mgorny added a comment to rG0c208d1f42be: [lldb] Fix flakyness in TestGdbRemote_vContThreads.

Ahh, I've seen them fail randomly recently too. But then it suddenly worked again and I couldn't reproduce...

Fri, Mar 26, 1:23 PM
mgorny added a comment to D93164: [AST] Add generator for source location introspection.

This change breaks cross-compilation now, as it tries running an executable built for the target system:

Fri, Mar 26, 9:09 AM · Restricted Project, Restricted Project

Thu, Mar 25

mgorny updated the diff for D98822: [lldb] [Process] Watch for fork/vfork notifications.

Now includes initial FreeBSD support. The watchpoint test still fails, we probably need to clean dbregs on fork too.

Thu, Mar 25, 3:32 PM · Restricted Project
mgorny updated the diff for D98822: [lldb] [Process] Watch for fork/vfork notifications.

Move saved breakpoint list directly into NativeProcessProtocol.

Thu, Mar 25, 4:19 AM · Restricted Project
mgorny added inline comments to D98822: [lldb] [Process] Watch for fork/vfork notifications.
Thu, Mar 25, 1:45 AM · Restricted Project
mgorny added inline comments to D98822: [lldb] [Process] Watch for fork/vfork notifications.
Thu, Mar 25, 12:45 AM · Restricted Project

Wed, Mar 24

mgorny added inline comments to D98822: [lldb] [Process] Watch for fork/vfork notifications.
Wed, Mar 24, 2:27 PM · Restricted Project