From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id SpcYJ0bWvWoSoiEAWB0awg (envelope-from ) for ; Wed, 30 Sep 2026 23:40:54 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=g5lo+m3t; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 90A0A1E051; Wed, 30 Sep 2026 23:40:54 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-6.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham autolearn_force=no version=4.0.1 Received: from vm01.sourceware.org (vm01.sourceware.org [IPv6:2620:52:6:3111::32]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id A34EC1E033 for ; Wed, 30 Sep 2026 23:40:52 -0400 (EDT) Received: from vm01.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B1B434BB58B2 for ; Thu, 1 Oct 2026 03:40:51 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B1B434BB58B2 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=g5lo+m3t Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTP id D24DC4BB3BAE for ; Thu, 1 Oct 2026 03:40:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D24DC4BB3BAE Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org D24DC4BB3BAE Authentication-Results: sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1790826023; cv=none; b=TCbHrq0OU5VvzKPbf8niEWLeI2lmt3qcJC/8SMxsZ0682MqloudeFBFZCnfgLnlgvemHiMPDVnmzYJQdN+OrOyzQjdAG5Arz1vzSn3+5nK1FID4X6mm6cpxhRXvmNdd93ZQIrrnI/NAWMZO7Rg609cy2+3YNu66wnBpMThy1QGA= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1790826023; c=relaxed/simple; bh=l7bO7D1sj0KXcRA9UvTpyOao4SeigWDD3nPpatc2qP0=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=AOVfblRcNp5ccUTjk/LH3Ub9Z1JW5LUOfu2u9hc6vQJOkBWxh5QGvppHe3eV/K/e8ifCtjPODs+Q6Nam7LNkzZAoMFn5fdHUaJp0Wxb7h8jK6mtM9fFFtbGEwBOV9n9rBkfAqycxhNY3gTnZG5wDjGyxfobubMicMvWa3PoYjTo= ARC-Authentication-Results: i=1; sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=g5lo+m3t DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org D24DC4BB3BAE DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1790826022; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=2DQwl4jDdUAHtUH+xoqPerUFblW/8l6aIEG7XjQh6Eg=; b=g5lo+m3tZxgWUEthByNEn4iFFvH9BqrQ/IWCZ8gD6d0LxFTQl255PoKIFeNnaWS80xXFo4 CPNjpS0pfd5CEXd+oms3tBufBIaDyA81cX10qCtycCAbzp3GTGSdZLkpnG8W9cvKESXGbE HcuLSsOsdrx+OjBKequdcFQ3KH9d/ws= Received: from mx-prod-mc-05.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-155-eycikdDzMbW9aOqgoGO-Wg-1; Wed, 30 Sep 2026 23:40:19 -0400 X-MC-Unique: eycikdDzMbW9aOqgoGO-Wg-1 X-Mimecast-MFC-AGG-ID: eycikdDzMbW9aOqgoGO-Wg_1790826018 Received: from mx-prod-int-08.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-08.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.111]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-05.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 36D6D1955F1D for ; Thu, 1 Oct 2026 03:40:18 +0000 (UTC) Received: from f44-mesa-1 (unknown [10.22.88.46]) by mx-prod-int-08.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 0D4D618005AE; Thu, 1 Oct 2026 03:40:16 +0000 (UTC) Date: Wed, 30 Sep 2026 20:40:13 -0700 From: Kevin Buettner To: Alexandra =?ISO-8859-1?Q?H=E1jkov=E1?= Cc: gdb-patches@sourceware.org Subject: Re: [RFC PATCH v2] gdb: Add drgn-why-sleeping command Message-ID: <20260930204013.717302f1@f44-mesa-1> In-Reply-To: <20260929190349.312877-1-ahajkova@redhat.com> References: <20260929190349.312877-1-ahajkova@redhat.com> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.30.177.111 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: fPPJKb90vwabiAHORZC6djsHWW53lVmPCBoejFQCJ9M_1790826018 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org On Tue, 29 Sep 2026 21:00:06 +0200 Alexandra Hajkova 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 " 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.