Re: [RFC PATCH v2] gdb: Add drgn-why-sleeping command

[Kevin Buettner <kevinb@redhat.com>]

On Tue, 29 Sep 2026 21:00:06 +0200
Alexandra Hajkova <ahajkova@redhat.com> wrote:

I had GLM 5.3 Flash set up a test environment and then test your
changes.  I watched it work in an attached interactive terminal
(occasionally entering commands of my own) and inspected the output as
it arrived.  My own comment here is that it'd be nice to have stack
traces that provided a bit more information (if possible) than just the
names of the kernel functions.

I also had Claude Opus 5.5 conduct an in depth review.  Everything
below is the write-up following Opus 5.5's review.  I agree with its
recommendations about moving forward with this work.

Hi Alexandra,

Thanks for v2.  Seeing where a thread is blocked inside the kernel
is useful, and moving to the sudo helper is a big improvement over
v1.  v2 was run on Fedora 44 (kernel 7.2.7, drgn 0.2.0, GDB 17.2)
with a pipe reader and a mutex waiter.  The behaviour matched the
docstring: ptrace_stop while GDB holds the thread, anon_pipe_read /
futex_wait after "continue &", and the vfs_read member paths
printed as described.  So these comments are about the code, the
error handling and the process, not about whether it works.

Where I mention drgn behaviour below, I checked it against the
drgn 0.2.0 sources.

Line references are to the v2 mail as posted (patch line N).


Status of the v1 review comments
================================

1. "I'm not sure this is really something gdb ought to ship."
   NOT FIXED.  The v2 notes don't address this.  Importing drgn
   lazily means users without drgn pay nothing, which helps.  But
   it doesn't answer whether GDB should own this code.  See
   "Shipping and testing" below.

2. sleep_test.c shouldn't be in the patch.
   FIXED.  It's gone.  (I think a version of it should come back
   as a testsuite program, though.  See below.)

3. Name not GDB-like; use dashes; maybe a prefix command.
   PARTIALLY FIXED.  The name now uses dashes.  The prefix-command
   idea was neither adopted nor answered.  Also, the v2 notes say
   "rename the command to drg-why-sleeping" (patch line 83).
   That's a typo for drgn-.

4. Extra parentheses around a raised GdbError.
   FIXED.  That code is gone, and the new GdbError calls are fine.

5. "We can't ever recommend running gdb as root."
   FIXED in substance.  GDB stays unprivileged.  Only the short
   sudo helper runs as root, and it hands back an fd for
   /proc/kcore.  It's worth saying in the docs and commit message
   that GDB still ends up holding a read-only fd for all of kernel
   memory.  That's much better than running as root, but it isn't
   nothing.

   (Tom's comment that commands are normally auto-installed is
   also FIXED: the data-directory/Makefile.in entry is there, in
   the right alphabetical position.)


Major issues
============

M1. A PID namespace mismatch can silently show the wrong task
    (patch lines 209, 225)

  > +        tid = thread.ptid[1]
  ...
  > +        task = find_task(prog, tid)

  The LWP from thread.ptid is a TID in *GDB's* PID namespace.
  find_task(prog, pid) looks the TID up in init_pid_ns
  (drgn/helpers/linux/pid.py: "Defaults to the initial PID
  namespace").  If GDB runs in a container with its own PID
  namespace and can still read /proc/kcore, the lookup can return
  a completely unrelated task.  The command would then print that
  task's stack with no warning.  A wrong answer is worse than an
  error here.

  Ideally, look the TID up in GDB's own namespace.  You could
  match the inode of /proc/self/ns/pid against
  task_active_pid_ns(...)->ns.inum.  At minimum, do a sanity check
  and fail if it doesn't match: task.tgid should equal the
  inferior's pid (thread.ptid[0]), and task.comm should match
  thread.name.

M2. A module without debuginfo makes the whole command fail
    (patch line 222)

  > +            prog.load_debug_info(default=True)

  load_debug_info() raises MissingDebugInfoError when debuginfo is
  missing for *any* module, even though everything else was
  loaded (see the docstring in _drgn.pyi).  So a single
  out-of-tree or third-party module without debuginfo (nvidia,
  zfs, vbox, ...) makes the command fail with "Could not
  initialize drgn", even though vmlinux is all it needs.  drgn's
  own CLI treats this case as a warning unless the main module is
  the one missing (drgn/cli.py, _load_debugging_symbols).  Please
  do the same: catch MissingDebugInfoError separately, fail only
  if vmlinux debuginfo is missing, and otherwise warn.

M3. drgn exceptions after initialization reach the user unhandled
    (patch lines 225-249)

  Everything after the try block can raise drgn exceptions.  GDB
  then shows a "Python Exception <class ...>" traceback-style
  error instead of a clean message.  The most likely case:

  > +        stack = prog.stack_trace(task)

  If the selected thread is actually running on a CPU, drgn raises
  ValueError("cannot unwind stack of running task") for a live
  kernel (libdrgn/stack_trace.c).  After "continue &", that's
  exactly what happens with any thread that isn't sleeping, such
  as a busy loop.  find_task() (via FaultError),
  task_state_to_char(), stack_trace() and frame.locals() can all
  raise.  Please turn these into gdb.GdbError with a clear
  message, e.g. "thread is running on a CPU; its kernel stack
  can only be read while it is sleeping".

M4. The sudo path depends on a private drgn module and on
    sys.executable (patch lines 190-196, 219)

  > +            from drgn.internal.sudohelper import open_via_sudo

  drgn/internal/__init__.py says: "This package contains modules
  internal to drgn.  You should not use them."  So GDB would be
  shipping code that relies on an interface drgn can change or
  remove in any release.  If this stays in GDB, please either ask
  drgn upstream to make open_via_sudo public, or copy the
  roughly 40 lines into GDB.

  There is also a real problem today: open_via_sudo() runs
  [sudo, sys.executable, "-B", "-c", ...].  In Fedora 44's GDB
  17.2, sys.executable inside GDB's Python is '/usr/bin/python',
  not python3.  That path comes from the optional
  python-unversioned-command package.  Without that package, the
  sudo step fails even though drgn and python3 are installed.
  More generally, sys.executable in an embedded interpreter is a
  guess based on GDB's configuration.  It isn't guaranteed to
  name the Python that GDB is linked against, or any working
  Python at all.


Medium issues
=============

m1. sudo failures and user experience (patch lines 214-224)

  - If sudo isn't installed, subprocess raises FileNotFoundError.
    The user sees "Could not initialize drgn: [Errno 2] No such
    file or directory: 'sudo'", and the original
    PermissionError on /proc/kcore is lost.  drgn's CLI checks
    shutil.which("sudo") first and reports the PermissionError;
    please do the same.

  - If sudo is denied or the password is wrong, check_call raises
    CalledProcessError.  Its str() contains the whole argv,
    including the embedded Python helper script.  So the user gets
    a multi-line block of Python code inside the "Could not
    initialize drgn:" message.  Please catch CalledProcessError and
    say something like "sudo failed; could not open /proc/kcore".

  - sudo reads the password from the controlling tty.  That works
    in a terminal, but under an MI frontend (IDEs, VS Code, ...)
    or in batch mode there may be no tty.  In that case sudo
    either fails, or prompts somewhere the user can't see.  Also,
    in non-stop mode the inferior keeps running while sudo
    prompts, so its output can mix with the prompt.  Please
    consider checking from_tty / os.isatty(0), and if there's no
    terminal, fail with a clear message instead of starting sudo.
    A "set drgn use-sudo on|off" style setting would also help.

  - The lockdown=confidentiality case from the v1 cover letter
    also raises PermissionError (EPERM).  So the command will
    prompt for sudo and then fail anyway.  Please check for
    EPERM vs EACCES and give a lockdown-specific message.

m2. Every invocation starts over (patch lines 214-222)

  Each run creates a new drgn.Program, may prompt for sudo again
  (depending on sudo's timestamp), and reloads debuginfo for
  vmlinux and every module.  That takes noticeable time on each
  run, and the typical workflow runs the command several times
  (stack, then FRAME, then FRAME FIELD...).  The running kernel
  doesn't change during a GDB session, so cache the Program at
  module level after the first successful set-up.

m3. Frame selection (patch lines 236-246)

  - "Frame not found" is printed, and the command still succeeds.
    It should be raise gdb.GdbError(...), so that scripts and
    breakpoint command lists can tell that it failed.
  - StackFrame.name is Optional[str].  Frames without a symbol
    print as "None" (line 237), and they can never be selected.
    Please print something like hex(frame.pc) as a fallback.
  - Matching the first frame by name can't reach later frames
    with the same name: recursion, or inlined frames, which drgn
    reports as separate frames.  Please also accept a frame
    number, like GDB's "frame N" and drgn's trace[N], and number
    the frames in the stack listing.  That would make the output
    look more like "bt", which GDB users already know.

m4. Checking a thread GDB has stopped

  The docstring says a thread stopped by GDB shows ptrace_stop.
  The command already knows when that's the case
  (thread.is_stopped()), so it could print a one-line note, e.g.
  "note: thread is stopped by GDB; resume it with 'continue &'
  to see where it is blocked".  That's more reliable than
  expecting users to read the help text.

m5. Order of the checks (patch lines 182-208)

  drgn is imported and the host checked before the
  selected-thread / native-target checks.  So on a remote target
  without drgn installed, the user is told to install drgn, which
  wouldn't help.  Please do the cheap GDB-side checks first
  (thread selected, native connection, thread alive, LWP valid),
  then import drgn.  This also makes the GDB-side error paths
  testable on machines without drgn (see below).


Minor issues and nits
=====================

- Patch lines 258-265: KeyError is a subclass of LookupError, so
  listing it is redundant.  AttributeError isn't raised by
  member_() for a missing member (that's LookupError).  Catching
  it would only hide an API mismatch with the installed drgn,
  shown as "unavailable (...)".  Please drop both.

- Patch line 178: prefix=False is the default.  Line 177: please
  use plain super().__init__(...).

- Formatting: please run the gdb/ pre-commit hooks (black, isort,
  flake8).  black wants two blank lines before the class
  (line 134).  It also reflows lines 210-213 onto one line.

- Docstring (lines 136-174):
  - The usage line says VARIABLE.FIELD, but the later text says
    FIELD.  Please pick one metavariable name.
  - The two examples at lines 161-162 are indented by 7 spaces
    instead of 8.
  - Please put the "Without arguments / With FRAME / With FIELD"
    explanation directly after the Usage line, and the
    non-stop how-to after it.

- Command class (line 178): COMMAND_STATUS is meant for
  "info"/"show"-style status commands.  The output is a stack
  trace plus frame locals, so COMMAND_STACK seems a better fit
  ("help stack" is where users would look for it).

- Naming and prefix (Tom's v1 point): "why-sleeping" describes
  one use, but what the command does is "kernel backtrace" and
  "kernel frame locals".  I'd suggest a prefix command along the
  lines of "drgn backtrace" and "drgn locals FRAME [EXPR...]", or
  an "info kernel-stack" style name.  That leaves room for more
  drgn-backed commands without adding more top-level names.

- Copyright header: the format and year are fine.

- There are no NEWS entry and no gdb.texinfo documentation yet.
  For an RFC that's acceptable, but a non-RFC version adding a
  user-visible command will need both.  The manual should say
  that drgn and kernel debuginfo are required, and that sudo may
  be used.


Shipping and testing
====================

On whether GDB should ship this at all: as far as I know, nothing
in gdb/python/lib currently depends on a Python package outside
the standard library.  This would be the first.  The
drgn.internal problem (M4) shows the cost.  GDB's release cycle
and drgn's would be coupled through an interface neither project
documents for the other.  I see two reasonable options:

  (a) The command lives in drgn, e.g. contrib/ or a small module
      that users source or that GDB auto-loads.  drgn can then
      use its own internals, and the version matching takes care
      of itself.

  (b) It lives in GDB, but only uses drgn's public API, checks a
      minimum drgn.__version__ at import time with a clear
      message, and has documentation, NEWS and tests like any
      other command.

I lean towards (a), but this is for the maintainers to decide.  If
you want (b), I'd suggest asking that question directly in the v3
cover letter, so you get an answer before doing more work.

On testing: the testsuite can't assume drgn, kernel debuginfo or
kcore access, but this can still be covered:

  - A gdb.python/py-drgn-why-sleeping.exp test, with the old
    sleep_test.c reduced to one blocked pipe reader as the
    testcase .c file.  That's the right home for the reproducer
    Tom asked you to remove from the top level.  Skip it unless
    "python import drgn" works and /proc/kcore can be read
    without prompting.  Never let the testsuite start an
    interactive sudo; use "sudo -n" or skip.

  - Tests that always run, even without drgn: the command is
    registered and "help drgn-why-sleeping" works; the error with
    no thread selected; the error on a core-file target.  The
    "drgn is missing" path can be tested by running
    "python sys.modules['drgn'] = None" first, which makes the
    import raise ImportError.  Most of these depend on the
    reordering in m5.


Summary: this is a useful idea, and v2 fixes the concrete v1
problems.  Before v3, I'd like to see the "does this belong in
GDB" question settled with the maintainers.  On the code side, I'd
most like to see M1 (PID namespace), M2 (partial debuginfo), M3
(unhandled drgn exceptions) and the drgn.internal / sys.executable
dependency (M4) fixed.