From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x333.google.com (mail-wm1-x333.google.com [IPv6:2a00:1450:4864:20::333]) by sourceware.org (Postfix) with ESMTPS id 09646388E830 for ; Fri, 24 Jul 2020 09:58:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 09646388E830 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wm1-x333.google.com with SMTP id k20so66213wmi.5 for ; Fri, 24 Jul 2020 02:57:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=uyxOmi8QaHmhwd0pmRFNSmrxFaVfUXCsgMy1z1e+Muw=; b=YfwBGQP6aeRYbIFuU0G/ESoG/wVlmAgVmunLVF7p5IAuOCUCPLJ07mkQwSx4aa0asE IcJFBZ9WzYPPXP0YYAt6iaBEgWbkolrk3O3PP+NXRQihTqOP3M74jlHvgwu/PIJyXxaj l3MZtH6Wfrut/l1rGapsTSfUt4WPAlUfVwXjgT+za5HNVzEK3Aw2Zt/eqDB38dzrYlN5 KF0EtyIe2FxK1htFtuLGlbblrsHwuWliXA1FWX+Yum8gH6DTPtfRU2nBYxg/Ic7J2Gi0 eVVRXcnXngOrHmEQahC2s2oyj/EwYglwTqGNbIyvagnP/8Bo23kovm4GFRu/+mjm+yFr b+uA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=uyxOmi8QaHmhwd0pmRFNSmrxFaVfUXCsgMy1z1e+Muw=; b=c5m8zcPci8Y1HQG78iqnHDc7YYKgeYMddhi4M1ni2BnQG1vdDBznipgBXc2jBXrViH Ie4SC5x8SbXADZc9v5OIieTh7B2zflXA+gU7exkw5KhVcHhbvrDQP+uSoXKkG7tTYSZe FY5aUJx6DUpwbyiesi/MHGn+OPA7rlgAi8tGJ04FB8Zskd7l2tZdvbLKzgzhXWX+HF1h 6q3Yu656l5n6voC13E6O8dS1jeUXnhDPVYUXeyCWf4b0uvoESe2Ptl45VUMzmuc63uVo x9lcOz4cU4+LxOHwWH3I1bPRKDjmoz3XEuPqzmtbgblDytneBXtjEZMK1f8I51Ywf7GP fk4Q== X-Gm-Message-State: AOAM531gavLYMb75QnaNm0foGfpnOLU2w2LPDspyfTNbV3JwVW/0fUTX v5kVNZWrhTv1mB6Yl2x1jlPJiT9cULQ= X-Google-Smtp-Source: ABdhPJyf6NdNbM2fAIXkqm/vnnTLhZqMziNyifsYGAesXoZigoVj1P5haVFeMnJ232///T0WnrGlOw== X-Received: by 2002:a7b:c5d8:: with SMTP id n24mr8144069wmk.153.1595584678228; Fri, 24 Jul 2020 02:57:58 -0700 (PDT) Received: from localhost (host86-134-151-238.range86-134.btcentralplus.com. [86.134.151.238]) by smtp.gmail.com with ESMTPSA id l81sm3697050wmf.4.2020.07.24.02.57.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 24 Jul 2020 02:57:57 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Subject: [PATCH 2/2] gdb/python: make more use of RegisterDescriptors Date: Fri, 24 Jul 2020 10:57:52 +0100 Message-Id: X-Mailer: git-send-email 2.25.4 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-9.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Jul 2020 09:58:02 -0000 This commit unifies all of the Python register lookup code (used by Frame.read_register, PendingFrame.read_register, and gdb.UnwindInfo.add_saved_register), and adds support for using a gdb.RegisterDescriptor for register lookup. Currently the register unwind code (PendingFrame and UnwindInfo) allow registers to be looked up either by name, or by GDB's internal number. I suspect the number was added for performance reasons, when unwinding we don't want to repeatedly map from name to number for every unwind. However, this kind-of sucks, it means Python scripts could include GDB's internal register numbers, and if we ever change this numbering in the future users scripts will break in unexpected ways. Meanwhile, the Frame.read_register method only supports accessing registers using a string, the register name. This commit unifies all of the register to register-number lookup code in our Python bindings, and adds a third choice into the mix, the use of gdb.RegisterDescriptor. The register descriptors can be looked up by name, but once looked up, they contain GDB's register number, and so provide all of the performance benefits of using a register number directly. However, as they are looked up by name we are no longer tightly binding the Python API to GDB's internal numbering scheme. As we may already have scripts in the wild that are using the register numbers directly I have kept support for this in the API, but I have listed this method last in the manual, and I have tried to stress that this is NOT a good method to use and that users should use either a string or register descriptor approach. After this commit all existing Python code should function as before, but users now have new options for how to identify registers. gdb/ChangeLog: * python/py-frame.c: Remove 'user-regs.h' include. (frapy_read_register): Rewrite to make use of gdbpy_parse_register_id. * python/py-registers.c (gdbpy_parse_register_id): New function, moved here from python/py-unwind.c. Updated the return type, and also accepts register descriptor objects. * python/py-unwind.c: Remove 'user-regs.h' include. (pyuw_parse_register_id): Moved to python/py-registers.c. (unwind_infopy_add_saved_register): Update to use gdbpy_parse_register_id. (pending_framepy_read_register): Likewise. * python/python-internal.h (gdbpy_parse_register_id): Declare. gdb/testsuite/ChangeLog: * gdb.python/py-unwind.py: Update to make use of a register descriptor. gdb/doc/ChangeLog: * python.texi (Unwinding Frames in Python): Update descriptions for PendingFrame.read_register and gdb.UnwindInfo.add_saved_register. (Frames In Python): Update description of Frame.read_register. --- gdb/ChangeLog | 15 +++++++++ gdb/doc/ChangeLog | 7 ++++ gdb/doc/python.texi | 41 ++++++++++++++++------- gdb/python/py-frame.c | 22 ++++++------ gdb/python/py-registers.c | 48 +++++++++++++++++++++++++++ gdb/python/py-unwind.c | 36 ++------------------ gdb/python/python-internal.h | 19 +++++++++++ gdb/testsuite/ChangeLog | 5 +++ gdb/testsuite/gdb.python/py-unwind.py | 11 +++++- 9 files changed, 147 insertions(+), 57 deletions(-) diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi index 4c0432372ab..d0a8b8c8a0d 100644 --- a/gdb/doc/python.texi +++ b/gdb/doc/python.texi @@ -2458,12 +2458,11 @@ @defun PendingFrame.read_register (reg) This method returns the contents of the register @var{reg} in the -frame as a @code{gdb.Value} object. @var{reg} can be either a -register number or a register name; the values are platform-specific. -They are usually found in the corresponding -@file{@var{platform}-tdep.h} file in the @value{GDBN} source tree. If -@var{reg} does not name a register for the current architecture, this -method will throw an exception. +frame as a @code{gdb.Value} object. For a description of the +acceptable values of @var{reg} see +@ref{gdbpy_frame_read_register,,Frame.read_register}. If @var{reg} +does not name a register for the current architecture, this method +will throw an exception. Note that this method will always return a @code{gdb.Value} for a valid register name. This does not mean that the value will be valid. @@ -2532,8 +2531,8 @@ specify caller registers that have been saved in this frame: @defun gdb.UnwindInfo.add_saved_register (reg, value) -@var{reg} identifies the register. It can be a number or a name, just -as for the @code{PendingFrame.read_register} method above. +@var{reg} identifies the register, for a description of the acceptable +values see @ref{gdbpy_frame_read_register,,Frame.read_register}. @var{value} is a register value (a @code{gdb.Value} object). @end defun @@ -4687,10 +4686,28 @@ @anchor{gdbpy_frame_read_register} @defun Frame.read_register (register) -Return the value of @var{register} in this frame. The @var{register} -argument must be a string (e.g., @code{'sp'} or @code{'rax'}). -Returns a @code{Gdb.Value} object. Throws an exception if @var{register} -does not exist. +Return the value of @var{register} in this frame. Returns a +@code{Gdb.Value} object. Throws an exception if @var{register} does +not exist. The @var{register} argument must be one of the following: +@enumerate +@item +A string that is the name of a valid register (e.g., @code{'sp'} or +@code{'rax'}). +@item +A @code{gdb.RegisterDescriptor} object, see @ref{Registers In Python} +for further information. +@item +A @value{GDBN} internal, platform specific number. Using these +numbers is supported for historic reasons, but is not recommended as +future changes to @value{GDBN} could change the mapping between numbers +and the registers they represent, breaking any Python code. The +numbers are usually found in the corresponding +@file{@var{platform}-tdep.h} file in the @value{GDBN} source tree. +@end enumerate +Using a string to access registers will be slightly slower than the +other two methods as @value{GDBN} must lookup the mapping between name +and internal register number. If performance is critical consider +looking up and caching a @code{gdb.RegisterDescriptor} object. @end defun @defun Frame.read_var (variable @r{[}, block@r{]}) diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c index 3fd1e7fabcc..e121afb222d 100644 --- a/gdb/python/py-frame.c +++ b/gdb/python/py-frame.c @@ -27,7 +27,6 @@ #include "python-internal.h" #include "symfile.h" #include "objfiles.h" -#include "user-regs.h" typedef struct { PyObject_HEAD @@ -242,12 +241,11 @@ frapy_pc (PyObject *self, PyObject *args) static PyObject * frapy_read_register (PyObject *self, PyObject *args) { - const char *regnum_str; + PyObject *pyo_reg_id; struct value *val = NULL; - if (!PyArg_ParseTuple (args, "s", ®num_str)) + if (!PyArg_UnpackTuple (args, "read_register", 1, 1, &pyo_reg_id)) return NULL; - try { struct frame_info *frame; @@ -255,14 +253,18 @@ frapy_read_register (PyObject *self, PyObject *args) FRAPY_REQUIRE_VALID (self, frame); - regnum = user_reg_map_name_to_regnum (get_frame_arch (frame), - regnum_str, - strlen (regnum_str)); - if (regnum >= 0) - val = value_of_register (regnum, frame); + if (!gdbpy_parse_register_id (get_frame_arch (frame), pyo_reg_id, + ®num)) + { + PyErr_SetString (PyExc_ValueError, "Bad register"); + return NULL; + } + + gdb_assert (regnum >= 0); + val = value_of_register (regnum, frame); if (val == NULL) - PyErr_SetString (PyExc_ValueError, _("Unknown register.")); + PyErr_SetString (PyExc_ValueError, _("Can't read register.")); } catch (const gdb_exception &except) { diff --git a/gdb/python/py-registers.c b/gdb/python/py-registers.c index fffe3ecb1e6..e5bca7e6064 100644 --- a/gdb/python/py-registers.c +++ b/gdb/python/py-registers.c @@ -370,6 +370,54 @@ register_descriptor_iter_find (PyObject *self, PyObject *args, PyObject *kw) Py_RETURN_NONE; } +/* See python-internal.h. */ + +bool +gdbpy_parse_register_id (struct gdbarch *gdbarch, PyObject *pyo_reg_id, + int *reg_num) +{ + if (pyo_reg_id == NULL) + return false; + + /* The register could be a string, its name. */ + if (gdbpy_is_string (pyo_reg_id)) + { + gdb::unique_xmalloc_ptr reg_name (gdbpy_obj_to_string (pyo_reg_id)); + + if (reg_name != NULL) + { + *reg_num = user_reg_map_name_to_regnum (gdbarch, reg_name.get (), + strlen (reg_name.get ())); + return *reg_num >= 0; + } + } + /* The register could be its internal GDB register number. */ + else if (PyInt_Check (pyo_reg_id)) + { + long value; + if (gdb_py_int_as_long (pyo_reg_id, &value) && (int) value == value) + { + if (user_reg_map_regnum_to_name (gdbarch, value) != NULL) + { + *reg_num = (int) value; + return true; + } + } + } + /* The register could be a gdb.RegisterDescriptor object. */ + else if (PyObject_IsInstance (pyo_reg_id, + (PyObject *) ®ister_descriptor_object_type)) + { + register_descriptor_object *reg + = (register_descriptor_object *) pyo_reg_id; + if (reg->gdbarch != gdbarch) + return false; + *reg_num = reg->regnum; + return true; + } + return false; +} + /* Initializes the new Python classes from this file in the gdb module. */ int diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c index 1cef491cedf..55d6a310c93 100644 --- a/gdb/python/py-unwind.c +++ b/gdb/python/py-unwind.c @@ -27,7 +27,6 @@ #include "python-internal.h" #include "regcache.h" #include "valprint.h" -#include "user-regs.h" #define TRACE_PY_UNWIND(level, args...) if (pyuw_debug >= level) \ { fprintf_unfiltered (gdb_stdlog, args); } @@ -101,37 +100,6 @@ static unsigned int pyuw_debug = 0; static struct gdbarch_data *pyuw_gdbarch_data; -/* Parses register id, which can be either a number or a name. - Returns 1 on success, 0 otherwise. */ - -static int -pyuw_parse_register_id (struct gdbarch *gdbarch, PyObject *pyo_reg_id, - int *reg_num) -{ - if (pyo_reg_id == NULL) - return 0; - if (gdbpy_is_string (pyo_reg_id)) - { - gdb::unique_xmalloc_ptr reg_name (gdbpy_obj_to_string (pyo_reg_id)); - - if (reg_name == NULL) - return 0; - *reg_num = user_reg_map_name_to_regnum (gdbarch, reg_name.get (), - strlen (reg_name.get ())); - return *reg_num >= 0; - } - else if (PyInt_Check (pyo_reg_id)) - { - long value; - if (gdb_py_int_as_long (pyo_reg_id, &value) && (int) value == value) - { - *reg_num = (int) value; - return user_reg_map_regnum_to_name (gdbarch, *reg_num) != NULL; - } - } - return 0; -} - /* Convert gdb.Value instance to inferior's pointer. Return 1 on success, 0 on failure. */ @@ -275,7 +243,7 @@ unwind_infopy_add_saved_register (PyObject *self, PyObject *args) if (!PyArg_UnpackTuple (args, "previous_frame_register", 2, 2, &pyo_reg_id, &pyo_reg_value)) return NULL; - if (!pyuw_parse_register_id (pending_frame->gdbarch, pyo_reg_id, ®num)) + if (!gdbpy_parse_register_id (pending_frame->gdbarch, pyo_reg_id, ®num)) { PyErr_SetString (PyExc_ValueError, "Bad register"); return NULL; @@ -376,7 +344,7 @@ pending_framepy_read_register (PyObject *self, PyObject *args) } if (!PyArg_UnpackTuple (args, "read_register", 1, 1, &pyo_reg_id)) return NULL; - if (!pyuw_parse_register_id (pending_frame->gdbarch, pyo_reg_id, ®num)) + if (!gdbpy_parse_register_id (pending_frame->gdbarch, pyo_reg_id, ®num)) { PyErr_SetString (PyExc_ValueError, "Bad register"); return NULL; diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h index 1e6dcf3dbb0..6874543441b 100644 --- a/gdb/python/python-internal.h +++ b/gdb/python/python-internal.h @@ -776,4 +776,23 @@ struct Py_buffer_deleter /* A unique_ptr specialization for Py_buffer. */ typedef std::unique_ptr Py_buffer_up; +/* Parse a register number from PYO_REG_ID and place the register number + into *REG_NUM. The register is a register for GDBARCH. + + If a register is parsed successfully then *REG_NUM will have been + updated, and true is returned. Otherwise the contents of *REG_NUM are + undefined, and false is returned. + + The PYO_REG_ID object can be a string, the name of the register. This + is the slowest approach as GDB has to map the name to a number for each + call. Alternatively PYO_REG_ID can be an internal GDB register + number. This is quick but should not be encouraged as this means + Python scripts are now dependent on GDB's internal register numbering. + Final PYO_REG_ID can be a gdb.RegisterDescriptor object, these objects + can be looked up by name once, and then cache the register number so + should be as quick as using a register number. */ + +extern bool gdbpy_parse_register_id (struct gdbarch *gdbarch, + PyObject *pyo_reg_id, int *reg_num); + #endif /* PYTHON_PYTHON_INTERNAL_H */ diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py index d01da80f25b..59602d69028 100644 --- a/gdb/testsuite/gdb.python/py-unwind.py +++ b/gdb/testsuite/gdb.python/py-unwind.py @@ -33,12 +33,19 @@ class FrameId(object): class TestUnwinder(Unwinder): AMD64_RBP = 6 AMD64_RSP = 7 - AMD64_RIP = 16 + AMD64_RIP = None def __init__(self): Unwinder.__init__(self, "test unwinder") self.char_ptr_t = gdb.lookup_type("unsigned char").pointer() self.char_ptr_ptr_t = self.char_ptr_t.pointer() + self._last_arch = None + + # Update the register descriptor AMD64_RIP based on ARCH. + def _update_register_descriptors (self, arch): + if (self._last_arch != arch): + TestUnwinder.AMD64_RIP = arch.registers ().find ("rip") + self._last_arch = arch def _read_word(self, address): return address.cast(self.char_ptr_ptr_t).dereference() @@ -77,6 +84,8 @@ class TestUnwinder(Unwinder): if (inf_arch != frame_arch): raise gdb.GdbError ("architecture mismatch") + self._update_register_descriptors (frame_arch) + try: # NOTE: the registers in Unwinder API can be referenced # either by name or by number. The code below uses both -- 2.25.4