Page MenuHomePhabricator

[lldb] [Process/Linux] Report fork/vfork stop reason
ClosedPublic

Authored by mgorny on Apr 9 2021, 8:17 AM.

Details

Summary

Enable reporting fork/vfork events to the server when supported.
At this moment, this is used only to test the server code, as real
client does not report fork-events and vfork-events as supported.

Diff Detail

Event Timeline

mgorny requested review of this revision.Apr 9 2021, 8:17 AM
mgorny created this revision.

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.

mgorny retitled this revision from [lldb] [Process/Linux] Report fork/vfork stop reason to client [WIP] to [lldb] [Process/Linux] Report fork/vfork stop reason to client.Apr 9 2021, 10:45 AM

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

mgorny updated this revision to Diff 336657.Apr 11 2021, 4:40 AM

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

Add GetSupportedExtensions() to actually enable the new code.

mgorny updated this revision to Diff 336670.Apr 11 2021, 8:38 AM

Report vfork-done stop to the client as well.

mgorny updated this revision to Diff 337238.Apr 13 2021, 12:45 PM

Rebase. Store mainloop arg of the original process, and pass it to the children to make them independent of the 'main' process.

mgorny updated this revision to Diff 339257.Apr 21 2021, 8:44 AM
mgorny retitled this revision from [lldb] [Process/Linux] Report fork/vfork stop reason to client to [lldb] [Process/Linux] Report fork/vfork stop reason.
mgorny edited the summary of this revision. (Show Details)

Now includes lldb-server tests.

@labath, please let me know if I should merge this with previous patches, or if it's fine to commit in split.

This seems reasonable.

Why don't we have tests that detach from the parent process (and maybe continue the child)? Does that already work? (or in general, what is expected to work after this patch?)

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
932

Store directly into unique_ptr<NativeProcessLinux> above.

938–940

I'd consider merging the two functions into SetStoppedByFork(is_vfork, child_pid)

lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
401–402

duplicated

409–410

ditto

lldb/test/API/tools/lldb-server/main.cpp
322–327 ↗(On Diff #339257)

I guess these would need to be ifdefed too.

Note that you don't actually have to use/extend this particular inferior for these tests. Feel free to create special purpose inferiors, if that's a better fit for the test, just like we do with "normal" tests...

This seems reasonable.

Why don't we have tests that detach from the parent process (and maybe continue the child)? Does that already work? (or in general, what is expected to work after this patch?)

It doesn't, we need D100261 for that. Right now current_process will become nullptr if we detach the parent.

That said, technically we could test that after detaching the parent we get an error from other requests ;-).

mgorny updated this revision to Diff 339350.Apr 21 2021, 12:34 PM
mgorny marked 5 inline comments as done.

Implemented all the requests, modulo claim that PID and TID are duplicate ;-). Also found cases for bool() with Extension stuff.

mgorny added inline comments.Apr 22 2021, 8:05 AM
lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
932

Heh, I suppose it started working because we no longer pass-by-reference to NewSubprocess(). Cool!

lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
401–402

Sure but that's only on Linux. FreeBSD and NetBSD use distinct thread IDs.

labath accepted this revision.Apr 23 2021, 12:33 PM

This seems reasonable.

Why don't we have tests that detach from the parent process (and maybe continue the child)? Does that already work? (or in general, what is expected to work after this patch?)

It doesn't, we need D100261 for that. Right now current_process will become nullptr if we detach the parent.

That said, technically we could test that after detaching the parent we get an error from other requests ;-).

I think that's a good thing to test, though I suppose it could also be a part of the Hg patch.

lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
402–403

Still duplicated. :)

This revision is now accepted and ready to land.Apr 23 2021, 12:33 PM
mgorny added inline comments.Apr 23 2021, 1:20 PM
lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
402–403

I'm confused. Maybe you're not noticing the tiny one-letter difference, i.e. pid vs tid? ;-) Linux is the only target where both are equal and the stop info needs to be generic, so that other platforms can assign distinct pid and tid.

mgorny updated this revision to Diff 340187.Apr 23 2021, 3:49 PM

Add multiprocess to supported features.

Tests were moved to earlier commit. Now this commit only enables the relevant category.

This revision was landed with ongoing or failed builds.Apr 24 2021, 2:26 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2021, 2:26 AM