Implement a new API for representing threads #129

ghost · 2021-11-23T21:14:57Z

This PR, once finished, will close #92. It is currently in draft mode as it is not yet finished. At present, the scaffolding has been set up following the design outlined in #92, and has been fully implemented for the case of debugging userspace coredumps.

ghost · 2021-11-23T23:15:24Z

Latest force-push didn't change anything, just signed off on the commit to fix the failing DCO Check

osandov

I love where this is going. I commented on a potential improvement for the thread map and a bunch of minor things. Please let me know if I missed your reasoning for anything I mentioned here. Thanks for tackling this!

_drgn.pyi

libdrgn/program.c

libdrgn/python/thread.c

_drgn.pyi

libdrgn/python/program.c

Signed-off-by: Kevin Svetlitski <[email protected]>

osandov

I only looked at the thread iterator parts, which looks great overall. I have some suggestions on improving the handling of the crashed thread, some cleanups, and some nits in the Python bindings.

_drgn.pyi

drgn/__init__.py

libdrgn/program.c

libdrgn/python/program.c

libdrgn/python/stack_trace.c

libdrgn/python/thread.c

Signed-off-by: Kevin Svetlitski <[email protected]>

ghost · 2021-12-03T20:12:14Z

I only looked at the thread iterator parts, which looks great overall. I have some suggestions on improving the handling of the crashed thread, some cleanups, and some nits in the Python bindings.

Just finished addressing these changes, I rebased them and combined them into the 2nd commit on this branch (i.e. the one with the message "Address @osandov's comments on #129")

Signed-off-by: Kevin Svetlitski <[email protected]>

.github/workflows/ci.yml

osandov

Looked at the thread API changes now, just a few minor things.

libdrgn/program.c

libdrgn/stack_trace.c

ghost · 2022-01-07T19:20:53Z

Thanks for chasing down those kdump reliability issues! Those were tricky. I tweaked your approach based on my comments and pushed it to https://github.com/osandov/drgn/tree/vmtest-kdump. Let me know what you think of those changes.

In the interest of keeping this PR small(ish) and getting some more code out the door, I'd like to merge the kdump testing framework before merging the thread API. Could you please open a separate PR for the kdump testing framework with a basic smoke test that makes sure that we can attach to /proc/vmcore?

See new PR #140

osandov · 2022-01-07T22:06:22Z

Now that #140 is merged, I think this is ready other than the last few comments I left. Please rebase and reorganize the commits for merging, then you can mark this as ready.

vmtest/enter_kdump.py

The majority of test cases already inherited from drgn's TestCase class. The few outliers that inherited directly from unittest.TestCase have been brought in line with the other tests. Signed-off-by: Kevin Svetlitski <[email protected]>

…celeration Disabling SMP is necessary to work around a bug in QEMU's handling of the capture kernel, but makes the tests run much slower. However, this bug only appears to manifest when KVM acceleration is disabled, so the testing harness has been modified to only disable SMP when this is true. [Omar: use an environment variable instead of touching a file] Signed-off-by: Omar Sandoval <[email protected]> Signed-off-by: Kevin Svetlitski <[email protected]>

This reverts commit 2b47583. After Kevin had completed this, we realized that there is a simpler method for iterating through tasks from libdrgn, which the next commit will implement. Revert the translation, but keep the improved tests.helpers.linux.test_pid.TestPid.test_for_each_task. Signed-off-by: Omar Sandoval <[email protected]>

The thread API needs a way to iterate over all task_structs in the kernel. Previously, we translated the existing for_each_task helper, which supports iterating through specific PID namespaces by walking through the PID radix tree or PID hashtable. However, we don't need specific namespaces for the thread API, so we can instead use the much simpler linked lists of thread groups and threads. Signed-off-by: Kevin Svetlitski <[email protected]>

osandov · 2022-01-11T22:41:27Z

I did a last pass for a few things:

Adding some additional test cases, and splitting up some larger tests into small tests.
Fleshing out the documentation a bit.
Some nitpicky style things like naming, function order, and changing // comments to /* */ comments.
Reordered the commits so that the unrelated test cleanups were first, then a straight revert of "Rewrite linux helper iterators in C", then adding the Linux kernel task iterator in its own commit. I also squashed the three thread API commits into one, because they weren't split up quite right. E.g., the first commit added the prstatus attribute which was removed by one of the later kernel commits, and the first commit didn't compile until a later commit renamed a function. Rather than rearranging things within the commits, I figured it'd be easier to squash everything into one.

Please look it over and make sure I didn't break anything. Once the tests pass and you take a look, I'll merge this. Thanks so much, this feature is going to be huge!

ghost

One or two minor correctness oversights, but otherwise looks good

libdrgn/python/thread.c

tests/test_thread.py

libdrgn/program.c

Previously, drgn had no way to represent a thread – retrieving a stack trace (the only extant thread-specific operation) was achieved by requiring the user to directly provide a tid. This commit introduces the scaffolding for the design outlined in issue osandov#92, and implements the corresponding methods for userspace core dumps, the live Linux kernel, and Linux kernel core dumps. Future work will build on top of this commit to support live userspace processes. Signed-off-by: Kevin Svetlitski <[email protected]>

osandov · 2022-01-12T01:30:48Z

Merged!

PR osandov#129 introduced the Threads API to drgn, but did not include support for live userspace processes. This commit changes that, both adding support for the existing Threads API, as well as extending it with a few additional methods which only make sense in the context of a live process, most notably `Thread.pause()` and `Thread.resume()` At present, only x86 is supported, but the plan is to support other architectures in the near future. Signed-off-by: Kevin Svetlitski <[email protected]>

ghost force-pushed the add-thread-support branch 4 times, most recently from 74ac552 to 27b872c Compare November 23, 2021 23:14

ghost force-pushed the add-thread-support branch from 0201cd6 to 601bcd3 Compare November 23, 2021 23:31

osandov reviewed Nov 24, 2021

View reviewed changes

ghost pushed a commit to Svetlitski/drgn that referenced this pull request Nov 29, 2021

Address @osandov's comments on osandov#129

6c78d8f

Signed-off-by: Kevin Svetlitski <[email protected]>

ghost force-pushed the add-thread-support branch from 0ed73b8 to 6c78d8f Compare November 29, 2021 17:59

osandov reviewed Dec 2, 2021

View reviewed changes

ghost pushed a commit to Svetlitski/drgn that referenced this pull request Dec 3, 2021

Address @osandov's comments on osandov#129

2cc3305

Signed-off-by: Kevin Svetlitski <[email protected]>

ghost force-pushed the add-thread-support branch from a494996 to bd43853 Compare December 3, 2021 20:10

ghost pushed a commit to Svetlitski/drgn that referenced this pull request Dec 3, 2021

Address @osandov's comments on osandov#129

2986a4c

Signed-off-by: Kevin Svetlitski <[email protected]>

ghost force-pushed the add-thread-support branch 5 times, most recently from 10693ee to d61a5c3 Compare December 3, 2021 22:59

ghost pushed a commit to Svetlitski/drgn that referenced this pull request Dec 6, 2021

Address @osandov's comments on osandov#129

6b94ea1

Signed-off-by: Kevin Svetlitski <[email protected]>

ghost force-pushed the add-thread-support branch 5 times, most recently from cadd964 to b674d51 Compare December 7, 2021 21:11

ghost pushed a commit to Svetlitski/drgn that referenced this pull request Dec 7, 2021

Address @osandov's comments on osandov#129

4a32bc9

Signed-off-by: Kevin Svetlitski <[email protected]>

ghost force-pushed the add-thread-support branch 4 times, most recently from 3108389 to 43e7a79 Compare December 8, 2021 19:55

osandov reviewed Jan 7, 2022

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

osandov reviewed Jan 7, 2022

View reviewed changes

libdrgn/program.c Outdated Show resolved Hide resolved

libdrgn/program.c Outdated Show resolved Hide resolved

libdrgn/program.c Show resolved Hide resolved

libdrgn/stack_trace.c Outdated Show resolved Hide resolved

ghost force-pushed the add-thread-support branch from 8da787a to 55a282a Compare January 7, 2022 20:35

ghost force-pushed the add-thread-support branch 2 times, most recently from ffb97e8 to 7d6d20c Compare January 7, 2022 23:49

ghost commented Jan 7, 2022

View reviewed changes

vmtest/enter_kdump.py Outdated Show resolved Hide resolved

ghost force-pushed the add-thread-support branch from 7d6d20c to 413aea7 Compare January 7, 2022 23:52

ghost marked this pull request as ready for review January 8, 2022 00:22

ghost requested a review from osandov January 8, 2022 00:25

ghost force-pushed the add-thread-support branch 2 times, most recently from 7852e96 to c04889b Compare January 11, 2022 21:21

Svetlitski and others added 3 commits January 11, 2022 14:34

osandov force-pushed the add-thread-support branch from c04889b to 9710b6e Compare January 11, 2022 22:41

osandov approved these changes Jan 11, 2022

View reviewed changes

ghost commented Jan 12, 2022

View reviewed changes

libdrgn/python/thread.c Show resolved Hide resolved

tests/test_thread.py Outdated Show resolved Hide resolved

libdrgn/program.c Outdated Show resolved Hide resolved

libdrgn/program.c Outdated Show resolved Hide resolved

osandov force-pushed the add-thread-support branch from 9710b6e to 8c93492 Compare January 12, 2022 01:10

osandov merged commit 301cc76 into osandov:main Jan 12, 2022

ghost deleted the add-thread-support branch January 12, 2022 02:57

ghost mentioned this pull request Jan 12, 2022

Live userspace process support for the Threads API #142

Closed

osandov mentioned this pull request Jul 3, 2023

Live userspace process debugging #320

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a new API for representing threads #129

Implement a new API for representing threads #129

ghost commented Nov 23, 2021

ghost commented Nov 23, 2021 •

edited by ghost

Loading

osandov left a comment

osandov left a comment

ghost commented Dec 3, 2021

osandov left a comment

ghost commented Jan 7, 2022

osandov commented Jan 7, 2022

osandov commented Jan 11, 2022

ghost left a comment

osandov commented Jan 12, 2022

Implement a new API for representing threads #129

Implement a new API for representing threads #129

Conversation

ghost commented Nov 23, 2021

ghost commented Nov 23, 2021 • edited by ghost Loading

osandov left a comment

Choose a reason for hiding this comment

osandov left a comment

Choose a reason for hiding this comment

ghost commented Dec 3, 2021

osandov left a comment

Choose a reason for hiding this comment

ghost commented Jan 7, 2022

osandov commented Jan 7, 2022

osandov commented Jan 11, 2022

ghost left a comment

Choose a reason for hiding this comment

osandov commented Jan 12, 2022

ghost commented Nov 23, 2021 •

edited by ghost

Loading