From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id jFc2NaX0IWhi8h8AWB0awg (envelope-from ) for ; Mon, 12 May 2025 09:16:21 -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=deohsSsC; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id BBB151E10E; Mon, 12 May 2025 09:16:21 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-10.1 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,RCVD_IN_VALIDITY_CERTIFIED, RCVD_IN_VALIDITY_RPBL,RCVD_IN_VALIDITY_SAFE autolearn=ham autolearn_force=no version=4.0.1 Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (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 C915D1E092 for ; Mon, 12 May 2025 09:16:20 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id F0DC03858C42 for ; Mon, 12 May 2025 13:16:19 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org F0DC03858C42 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=deohsSsC 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 0E1AD3858D1E for ; Mon, 12 May 2025 13:15:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0E1AD3858D1E 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 0E1AD3858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1747055745; cv=none; b=q5eCUEM1MTcSmZMO9ShAVhEVIU6KbSVCN4itZTZlNq+45ME1E79fff9KN5VapjyJFJjHSi+CqgmdYKyR/oLcanTEl1UKN9vu/hnYwnn/qU2YhNn8Kf5o1XrMeVecYbxJXSYYaflygFd2e7YBAyHCF2N713k2qfsuGBZuwRWIuVI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1747055745; c=relaxed/simple; bh=sTDuCon9ftAsQom2EsJ2kqvKgmjSLOAf4ruhDjTvsrM=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=gzP8UDPulMMH2O6oQStRtWBsmFIHOsMIWMEQmbwbsvMe1YzBmhj2jcvs2vdG6bXBZwED8S2Md6E10I+f8X1Oa9/yDaESucyV56cnPJuBR6LVxlpYzeTgud2hSh2nPZhrzHbN7X9pC/hLYBAK08X91ocPrwWbx2AnVUMA6KOBojg= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0E1AD3858D1E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1747055744; 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: in-reply-to:in-reply-to:references:references; bh=YvwG9eWbeDZwzFwRA+EzW9CjLHsHaAczp7LZsHMM/J4=; b=deohsSsCb9mT5867NBUhYyjtPqiFGlfofYBGzeJ1QKr4V+XhcXF13EVZ/XBdIujXAKvdvZ axJPdsalJ+yHyba71brCytzLCwPYEurJO2ba73Kf4ZwszEKdWm/rlF+MWznQLt2mq5cM4M il3Ml+7fSXmzkUIbxGbWOnMSvWlMWwM= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-441-SK-SWq0-NwuajVg-n7fu7A-1; Mon, 12 May 2025 09:15:43 -0400 X-MC-Unique: SK-SWq0-NwuajVg-n7fu7A-1 X-Mimecast-MFC-AGG-ID: SK-SWq0-NwuajVg-n7fu7A_1747055742 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-440667e7f92so21662935e9.3 for ; Mon, 12 May 2025 06:15:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1747055742; x=1747660542; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=YvwG9eWbeDZwzFwRA+EzW9CjLHsHaAczp7LZsHMM/J4=; b=wl8Lw6LwOYYZQRFHyHkUKh0jYnQTLEBeo1+G/71lqurNBrBgyHuXUpY+bDkiyKHI4V kOrSfblQ0HCREFAo6xtxlIRneafm5bEBlpQ26nGc6JIQK+Dpk1ecioMqvZKgSJ0qBX52 /KI22+SWqTBtjzkM33t/YsBOp4r81R4QDAJX+PNfJ8U8c1cdytYU/81k4XirsYkAXGsw 8S+yLBCQOywgfdbmfez3bXfAzt20zAVcvqlYrFngpl1vlKtPIQ59iFEgyz0ht9PdBgms FPp9P1+oV0q7/LxK6KsaPka81srr9gPf+VKBO34f8wY3tl3Udp4c/hCIY44FxV0I2Rag aZ4A== X-Forwarded-Encrypted: i=1; AJvYcCVcrYY2QtNLRj9Nx/I8Cgza77+ZF+Jp0Dnp6hyDXcC7PudRIFn4dYcUIroypWRQ5fMaTAaCkM1zlcEzsA==@sourceware.org X-Gm-Message-State: AOJu0YzXf1im7WEjkSZF0Niuiy9JOcrY9z57GQUrTe7bvxzPdCiehm9O mpYQeB+ItJu6pLXyBJBhIaoJ+IYkhdGD5/dMUnaSqLENXlnONdvgsDgQsWlS6CGzSsw9Q5vWolG 36mIbYuhMi759CQDwb9QCcVpLRmfuJVrDL5IYzjbq9yh6lI9QTtKrGkvZRMs= X-Gm-Gg: ASbGncuRWE5OCFkLGIViuCLrL8CSfsYHiVp+okFc8vQbAgkC7vV9CiUes7Dm+0PNcz8 3Ttff/MEURaLaoX9ytzDakgcw1/+HfH9xGh3Sg3kFx1oJkXjkUcKBsTPT5mrHJrNi/v7ZofA1PV L5m5/OqPh5xmslkEa7WcPdDEddbMbXATwbnoLfXjky1J7Lzs7DTwr+hcU0nEfI9MT+Kzx/88vD4 cnBYsuYDsuHq+w9oeVmrpr5KyWj7dfHp6vGQ8VCdif4RqG7hIqDbYBlU3k8NHEV6dfnv6RoVZL1 6jlnDrixnWjT70d1kr0mtzSPutGRQ3Ulp0q6 X-Received: by 2002:adf:e105:0:b0:39c:30d9:3b5c with SMTP id ffacd0b85a97d-3a1f649a919mr8753324f8f.39.1747055742123; Mon, 12 May 2025 06:15:42 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHJxtKgkZL/d+bhysqfpk5ciA+ru0Qy/m/WmBD6+nJVfvLwYdO+I5aVBKGykcjLp+je0eCs0Q== X-Received: by 2002:adf:e105:0:b0:39c:30d9:3b5c with SMTP id ffacd0b85a97d-3a1f649a919mr8753290f8f.39.1747055741604; Mon, 12 May 2025 06:15:41 -0700 (PDT) Received: from localhost (30.226.159.143.dyn.plus.net. [143.159.226.30]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3a1f5a2d2ffsm12589774f8f.66.2025.05.12.06.15.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 12 May 2025 06:15:41 -0700 (PDT) From: Andrew Burgess To: Craig Blackmore , gdb-patches@sourceware.org Cc: Craig Blackmore , Eli Zaretskii Subject: Re: [RFC PATCH v2] gdb: Add python support for demangling register unwind values In-Reply-To: <20250508101721.2000793-1-craig.blackmore@embecosm.com> References: <20250410110426.3488955-1-craig.blackmore@embecosm.com> <20250508101721.2000793-1-craig.blackmore@embecosm.com> Date: Mon, 12 May 2025 14:15:40 +0100 Message-ID: <87wmamotqb.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: Dk4pWtn8dQ6xLX7HhyeiDBBCQS-Qr_-wsKVhcth_A_o_1747055742 X-Mimecast-Originator: redhat.com Content-Type: text/plain 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 Craig Blackmore writes: > This patch addresses the scenario where a register value is saved in > some mangled form on the target and gdb does not have sufficient > information to demangle it. I'm assuming this is some kind of security related thing, e.g. return address is hashed or "encrypted" in some way? Some of my thoughts below are based on this assumption. > > After getting a register value from an unwinder, call out to extension > languages to allow the value to be demangled. This avoids having to > write a new unwinder and allows the standard unwinders to be used. > > This approach is purely for demangling values that have been read, there > is no similar support provided for mangling values being written back to > the target as this was not something that I needed to be able to do. OK, but, presumably, if you did try to modify one of these demangled registers, and then resumed the inferior, things would not continue as expected? If this is true, and right now you don't need to write the value back. Would it not be worth designing an API now that _could_ support this functionality? I have no problem, in theory, with having this functionality in the API, the problem is, once things are added to the Python API, we're usually pretty conservative about changing them, so we can only add new features. Now, sure, if, in the future, we want to support writing registers, we can add a whole new, parallel, API that supports the writing case. But wouldn't it be nice if we spent a little time thinking, up front, what that writing case might look like, even if it's not implemented? Maybe we can just document in this commit message what that support would look like, and how it would fit into the existing API. > > Only one demangler can be registered at any one time. In future, the > implementation could be extended to support multiple demanglers executed > in order of priority. Again, have you thought about how this might look? For the same reasons as before, while for changes to GDB's C++ it's fine to do "just enough", the Python API has a nasty habit of becoming locked in. I notice, for example, that register demanglers are created and registered by creating a UnwindRegisterValueDemangler instance. In contrast, instruct, many other similar functionality is done via a gdb.register_* or gdb.*.register_ function which take a locus that allows handlers to be attached to specific objfiles or program spaces. I think it might be a good idea if you switched to using this same approach. Currently, these register functions accept a locus of `None` to indicate that this should be a global registration. You could, for example, throw an error if a locus other than `None` is used. But this would then leave the door open, in future, to support other locus, which would allow per-program-space registrations for example. I also wonder if you should be more inline with the existing register functions when it comes to supporting multiple handlers, while at the same time, not actually changing what you support? For example, other register functions take a `replace` flag which controls whether existing handlers are replaced. You could just support replacing a handler with the same name, or adding a single handler, anything else: raise an error. This would be a little more work up front, but not significantly, and in the future. The benefit is the API is consistent, and future proof. I'm not sure how you'd extend the existing API to handle per-program-space registrations, which I think should be a minimum requirement. > > The demangler is registered globally. In future, the implementation > could be extended to register per program space and object file too. See above. Or maybe outline how you'd see that future design being. > > gdbpy_get_register_descriptor is now externally visible as it is used > to create a gdb.RegisterDescriptor object to pass to the demangler. > > This patch adds gdb.Value.is_lval_{register,memory} variables so that > the demangler can choose to skip modifying values that did not come > directly from the target and may not need demangling, for example, > values taken directly from DWARF. I do wonder if this is the right approach for dealing with lval types. If we ever want to handle different lval types in the future we're now stuck adding yet more is_lval_* methods. Wouldn't a better approach be to add a read-only lval type attribute? I don't think you'd even need to support all the types necessarily, you could add constants for maybe just: not_lval, lval_memory, lval_register, and map anything not caught be the above to not_lval, and document that other lval types might be added in the future. Though I'm happy to be convinced that separate flags is right way to go... > diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi > index 7bb650347f7..7fd0ba4f83b 100644 > --- a/gdb/doc/python.texi > +++ b/gdb/doc/python.texi > @@ -938,6 +938,16 @@ fetched when the value is needed, or when the @code{fetch_lazy} > method is invoked. > @end defvar > > +@defvar Value.is_lval_register > +The value of this read-only boolean attribute is @code{True} if this > +@code{gdb.Value} is from a register on the inferior. > +@end defvar > + > +@defvar Value.is_lval_memory > +The value of this read-only boolean attribute is @code{True} if this > +@code{gdb.Value} is from inferior memory. > +@end defvar > + > @defvar Value.bytes > The value of this attribute is a @code{bytes} object containing the > bytes that make up this @code{Value}'s complete value in little endian > @@ -3160,6 +3170,44 @@ the matching unwinders are enabled. The @code{enabled} field of each > matching unwinder is set to @code{True}. > @end table > > +@subheading Unwind Register Value Demangler > + > +After getting a register value from an unwinder, @value{GDBN} will call > +out to extension language demanglers to allow them to modify the value. > +This is useful in case a register value needs demangling before > +@value{GDBN} uses it and avoids the need to write a new unwinder. > + > +Currently, only one demangler can be registered at any one time and it > +is registered globally. > + > +@subheading Skeleton Code for a Register Value Demangler > + > +Here is an example of how to structure a user created demangler: > + > +@smallexample > +from gdb.unwinder import UnwindRegisterValueDemangler > + > +class MyUnwindRegisterValueDemangler(UnwindRegisterValueDemangler): > + def __init__(self): > + # Register self as the one and only demangler > + super().__init__("MyUnwinder_Name") > + > + def __call__(self, frame_type, reg, value): I'll add this comment here just because: I think it is a mistake to pass through the `frame_type` here. I think you'd be better off passing the gdb.Frame object itself, then let the demangled grab the frame type if that's what it needs. This is a pretty small change, but gives the API greater flexibility for the future. > + """ > + Return new value if demangled, otherwise None. > + """ > + if frame_type != : > + return None > + > + if reg.name == : > + new_value = ... compute demangled value ... > + return new_value > + > + return None > + > +my_demangler = MyUnwindRegisterValueDemangler() > +@end smallexample > + > @node Xmethods In Python > @subsubsection Xmethods In Python > @cindex xmethods in Python > diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c > index d43d7e97d99..6ec239664f5 100644 > --- a/gdb/python/py-unwind.c > +++ b/gdb/python/py-unwind.c > @@ -30,6 +30,7 @@ > #include "stack.h" > #include "charset.h" > #include "block.h" > +#include "value.h" > > > /* Debugging of Python unwinders. */ > @@ -1181,3 +1182,82 @@ PyTypeObject unwind_info_object_type = > 0, /* tp_init */ > 0, /* tp_alloc */ > }; > + > +/* Call demangler and handle the result. */ > + > +enum ext_lang_rc > +gdbpy_demangle_unwind_register_value (frame_info_ptr frame, int regnum, > + struct value **val) > +{ > + if (!gdb_python_initialized) > + return EXT_LANG_RC_ERROR; If you checkout gdbpy_print_insn you'll notice that it uses this: if (!gdb_python_initialized || !python_print_insn_enabled) return {}; The `python_print_insn_enabled` is a C++ global that is set from Python code when any handlers are registered. The point here is that for the common case, where no handler is registered, GDB doesn't need to enter the Python environment, or create, access, or setup, any Python state at all. This is a pretty easy thing to add, but my thinking at the time, is that this will give a small improvement (of no Python work) for the common case. Do you think you could look at how hard it would be to do something similar in this case too please? > + > + gdbarch *gdbarch = get_frame_arch (frame); > + gdbpy_enter enter_py (gdbarch); > + > + gdbpy_ref<> py_frame_type > + = gdb_py_object_from_longest (get_frame_type (frame)); > + > + gdbpy_ref<> py_reg_obj (gdbpy_get_register_descriptor (gdbarch, regnum)); > + if (py_reg_obj == NULL) Remember s/NULL/nullptr/ throughout this patch. > + { > + gdbpy_print_stack (); > + return EXT_LANG_RC_ERROR; > + } > + > + gdbpy_ref<> py_value_obj (value_to_value_object (*val)); > + if (py_value_obj == NULL) > + { > + gdbpy_print_stack (); > + return EXT_LANG_RC_ERROR; > + } > + > + /* Call demangler. */ > + const char *func_name = "_execute_unwind_register_value_demangler"; > + if (gdb_python_module == NULL > + || ! PyObject_HasAttrString (gdb_python_module, func_name)) > + { > + PyErr_SetString (PyExc_NameError, > + "Installation error: gdb._execute_unwind_register_value_demangler " > + "function is missing"); > + gdbpy_print_stack (); > + return EXT_LANG_RC_ERROR; > + } > + gdbpy_ref<> pyo_execute ( > + PyObject_GetAttrString (gdb_python_module, func_name)); > + if (pyo_execute == nullptr) > + { > + gdbpy_print_stack (); > + return EXT_LANG_RC_ERROR; > + } > + > + /* Demangler should return a gdb.Value if value has been changed, otherwise > + None. */ > + gdbpy_ref<> pyo_execute_ret > + (PyObject_CallFunctionObjArgs (pyo_execute.get (), py_frame_type.get (), > + py_reg_obj.get (), py_value_obj.get (), > + NULL)); > + if (pyo_execute_ret == nullptr) > + { > + gdbpy_print_stack (); > + return EXT_LANG_RC_ERROR; > + } > + /* Use original value. */ > + if (pyo_execute_ret == Py_None) > + return EXT_LANG_RC_NOP; > + > + /* Verify the return value is a gdb.Value. */ > + struct value *new_val = convert_value_from_python (pyo_execute_ret.get ()); > + > + if (new_val == nullptr) > + { > + gdbpy_print_stack (); > + error (_("An unwind register value demangler should return a gdb.Value " > + "instance if the value is changed, otherwise None")); > + return EXT_LANG_RC_ERROR; There's no need for a return after a call to error(). I also notice that this error message doesn't show up in the testsuite, so it would appear that this case isn't being tested maybe? Thanks, Andrew > + } > + /* Finally update val to the demangled value. */ > + *val = new_val; > + > + return EXT_LANG_RC_OK; > +}