From patchwork Tue Jul 18 12:20:45 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 72862 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id CD2A1385AF85 for ; Tue, 18 Jul 2023 12:21:04 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mail-wr1-f52.google.com (mail-wr1-f52.google.com [209.85.221.52]) by sourceware.org (Postfix) with ESMTPS id 030213857734 for ; Tue, 18 Jul 2023 12:20:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 030213857734 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wr1-f52.google.com with SMTP id ffacd0b85a97d-316f589549cso2881578f8f.1 for ; Tue, 18 Jul 2023 05:20:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689682847; x=1692274847; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Rvot/v+zcDXL2uWylwLZf8E95qjAE5+RHWRKFMz5y+U=; b=bKejtEJEHiNzgFAn2Ubrc/MWkH66n9KZ+XeugOCDTN+Gw7oy4IPv+dVyDVrZOVmZTN gT301GRyVEJKGQ4/NOsXH6A4RtaFrOz2OB+BQl51hYrTtKNRlPGHPeu1F6CbAnXxiq6n 0MWoU7gw9SJ36WYk4sW4mpgcII/0SsBkcSaP4NoSfVJfs2ZWQmh6t9eNOQq24NtPXSeE SYax/fjsLEA2u/k/1lghu1PifGaoTm54dGGFyY1zZTs0A0jzJW7aM6TacoLW6vHbt8Jd hbpXav0mKATJsvEaP0rOT1ypDGXoiTWmBs0s/Wk1/Q0NLJwxy6gKu6wOb3dKRQkSihc9 Aw2Q== X-Gm-Message-State: ABy/qLb5BbN+fO4kwQ6wKaYniVBb4ParBY1MWgk6ffS6FA05L3jYi8Rv Tgn872hKPDm+/ON0C/ihvL8= X-Google-Smtp-Source: APBJJlE+GPzkqnhlG548XkeS6j1NxZ9tkaq7yO0R7Aa7vdJCiEedB4ILip4Q3JclJiLftIDXM15d2w== X-Received: by 2002:a5d:5146:0:b0:315:adca:4f26 with SMTP id u6-20020a5d5146000000b00315adca4f26mr12029647wrt.4.1689682847042; Tue, 18 Jul 2023 05:20:47 -0700 (PDT) Received: from ?IPV6:2001:8a0:f91d:bc00:2d72:1f79:bed1:65bb? ([2001:8a0:f91d:bc00:2d72:1f79:bed1:65bb]) by smtp.gmail.com with ESMTPSA id i3-20020adfe483000000b00313f61889ecsm2262363wrm.66.2023.07.18.05.20.46 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 18 Jul 2023 05:20:46 -0700 (PDT) Message-ID: <48df4693-3445-fe22-92e9-f11b9120b916@palves.net> Date: Tue, 18 Jul 2023 13:20:45 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: [PATCH] Fix gdb.Inferior.read_memory without execution (PR dap/30644) (was: Re: [PATCH v4 6/6] Use correct inferior in Inferior.read_memory et al) Content-Language: en-US To: Tom Tromey Cc: Andrew Burgess , gdb-patches@sourceware.org References: <20230714-py-inf-fixes-30615-v4-0-9189744d8547@adacore.com> <20230714-py-inf-fixes-30615-v4-6-9189744d8547@adacore.com> <878rbhi9nh.fsf@redhat.com> <87lefemphy.fsf@tromey.com> <586e6a45-988f-6fc9-2bb2-da50f0b53d74@palves.net> <87h6q2ml79.fsf@tromey.com> From: Pedro Alves In-Reply-To: <87h6q2ml79.fsf@tromey.com> X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: , Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" On 2023-07-17 19:44, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves writes: > > Pedro> I'm thinking that we should just remove the ptid parameter and the > Pedro> any_thread_of_inferior calls completely, and make > Pedro> scoped_restore_current_inferior_for_memory switch inferior_ptid to > Pedro> a pid ptid. > ... > > Pedro> While on some ports like on AMD GPU we have things like thread-specific > Pedro> address spaces, and so when accessing memory for those address spaces, > Pedro> we must have the right thread context (via inferior_ptid) selected, in > Pedro> Inferior.read_memory, we only have the inferior, so this means that this API as > Pedro> is can't be used to access such address spaces. > > Yeah. Probably for this case we need to add Thread.read_memory et al, > then make note of this in the gdb.Inferior docs. > > Pedro> I ran the testsuite on native and extended-gdbserver (x86_64 GNU/Linux), > Pedro> and saw no regressions. > > +1 on this from me. Thank you. > > BTW this is https://sourceware.org/bugzilla/show_bug.cgi?id=30644 > > Reviewed-By: Tom Tromey Alright, now as a proper patch, with ctor intro comment tweaked, and with new test added to gdb.python/py-inferior.exp. WDYT? ---8<--- From e15ab1d7209b65f344ec20931452861990706742 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Mon, 17 Jul 2023 18:31:02 +0100 Subject: [PATCH] Fix gdb.Inferior.read_memory without execution (PR dap/30644) Andrew reported that the previous change to gdb.Inferior.read_memory & friends introducing scoped_restore_current_inferior_for_memory broke gdb.dap/stop-at-main.exp. This is also reported as PR dap/30644. The root of the problem is that all the methods that now use scoped_restore_current_inferior_for_memory cause GDB to crash with a failed assert if they are run on an inferior that is not yet started. E.g.: (gdb) python i = gdb.selected_inferior () (gdb) python i.read_memory (4,4) gdb/thread.c:626: internal-error: any_thread_of_inferior: Assertion `inf->pid != 0' failed. This patch fixes the problem by removing scoped_restore_current_inferior_for_memory's ctor ptid parameter and the any_thread_of_inferior calls completely, and making scoped_restore_current_inferior_for_memory switch inferior_ptid to a pid ptid. I was a little worried that some port might be assuming inferior_ptid points at a thread in the xfer_partial memory access routines. We know that anything that supports forks must not assume that, due to how detach_breakpoints works. I looked at a number of xfer_partial implementations, and didn't see anything that is looking at inferior_ptid in a way that would misbehave. I'm thinking that we could go forward with this and just fix ports if they break. While on some ports like on AMD GPU we have thread-specific address spaces, and so when accessing memory for those address spaces, we must have the right thread context (via inferior_ptid) selected, in Inferior.read_memory, we only have the inferior to work with, so this API as is can't be used to access thread-specific address spaces. IOW, it can only be used to access the global address space that is visible to both the CPU and the GPUs. In proc-service.c:ps_xfer_memory, the other spot using scoped_restore_current_inferior_for_memory, we're always accessing per-inferior memory. If we end up using scoped_restore_current_inferior_for_memory later to set up the context to read memory from a specific thread, then we can add an alternative ctor that takes a thread_info pointer, and make inferior_ptid point to the thread, for example. New test added to gdb.python/py-inferior.exp, exercising Inferior.read_memory without execution. No regressions on native and extended-gdbserver x86_64 GNU/Linux. Reviewed-By: Tom Tromey Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30644 Change-Id: I11309c5ddbbb51a4594cf63c21b3858bfd9aed19 --- gdb/inferior.h | 8 ++++---- gdb/proc-service.c | 3 +-- gdb/python/py-inferior.c | 8 ++++---- gdb/testsuite/gdb.python/py-inferior.c | 1 + gdb/testsuite/gdb.python/py-inferior.exp | 10 ++++++++++ 5 files changed, 20 insertions(+), 10 deletions(-) base-commit: 4676d03804285d76a51cb7f98ebe72374321a1f9 diff --git a/gdb/inferior.h b/gdb/inferior.h index 98501652a27..df586701612 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -769,14 +769,14 @@ class scoped_restore_current_inferior_for_memory public: /* Save the current globals and switch to the given inferior and the - inferior's program space. PTID must name a thread in INF, it is - used as the new inferior_ptid. */ - scoped_restore_current_inferior_for_memory (inferior *inf, ptid_t ptid) + inferior's program space. inferior_ptid is set to point to the + inferior's process id (and not to any particular thread). */ + scoped_restore_current_inferior_for_memory (inferior *inf) : m_save_ptid (&inferior_ptid) { set_current_inferior (inf); set_current_program_space (inf->pspace); - inferior_ptid = ptid; + inferior_ptid = ptid_t (inf->pid); } DISABLE_COPY_AND_ASSIGN (scoped_restore_current_inferior_for_memory); diff --git a/gdb/proc-service.c b/gdb/proc-service.c index 366e0510070..f735eb0ac81 100644 --- a/gdb/proc-service.c +++ b/gdb/proc-service.c @@ -72,8 +72,7 @@ static ps_err_e ps_xfer_memory (const struct ps_prochandle *ph, psaddr_t addr, gdb_byte *buf, size_t len, int write) { - scoped_restore_current_inferior_for_memory save_inferior (ph->thread->inf, - ph->thread->ptid); + scoped_restore_current_inferior_for_memory save_inferior (ph->thread->inf); CORE_ADDR core_addr = ps_addr_to_core_addr (addr); diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c index f6000b944da..792f05b118e 100644 --- a/gdb/python/py-inferior.c +++ b/gdb/python/py-inferior.c @@ -550,7 +550,7 @@ infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw) /* Use this scoped-restore because we want to be able to read memory from an unwinder. */ scoped_restore_current_inferior_for_memory restore_inferior - (inf->inferior, any_thread_of_inferior (inf->inferior)->ptid); + (inf->inferior); buffer.reset ((gdb_byte *) xmalloc (length)); @@ -608,7 +608,7 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw) still used here, just to keep the code similar to other code in this file. */ scoped_restore_current_inferior_for_memory restore_inferior - (inf->inferior, any_thread_of_inferior (inf->inferior)->ptid); + (inf->inferior); write_memory_with_notification (addr, buffer, length); } @@ -683,7 +683,7 @@ infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw) still used here, just to keep the code similar to other code in this file. */ scoped_restore_current_inferior_for_memory restore_inferior - (inf->inferior, any_thread_of_inferior (inf->inferior)->ptid); + (inf->inferior); found = target_search_memory (start_addr, length, buffer, pattern_size, @@ -944,7 +944,7 @@ infpy_get_main_name (PyObject *self, void *closure) still used, just to keep the code similar to other code in this file. */ scoped_restore_current_inferior_for_memory restore_inferior - (inf->inferior, any_thread_of_inferior (inf->inferior)->ptid); + (inf->inferior); name = main_name (); } diff --git a/gdb/testsuite/gdb.python/py-inferior.c b/gdb/testsuite/gdb.python/py-inferior.c index 3ee9a462060..870f7196444 100644 --- a/gdb/testsuite/gdb.python/py-inferior.c +++ b/gdb/testsuite/gdb.python/py-inferior.c @@ -16,6 +16,7 @@ int64_t int64_search_buf[100]; static char *search_buf; static int search_buf_size; +int8_t int8_global = 42; int f2 (int a) { diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp index 13beebd08cc..6fbcdd6822f 100644 --- a/gdb/testsuite/gdb.python/py-inferior.exp +++ b/gdb/testsuite/gdb.python/py-inferior.exp @@ -34,6 +34,16 @@ switch [get_endianness] { big { set python_pack_char ">" } } +# Test memory read operations without execution. + +gdb_py_test_silent_cmd "python addr = gdb.lookup_global_symbol ('int8_global').value().address" \ + "get global variable address" 0 +gdb_test "python \ + int8_global_mv = gdb.selected_inferior().read_memory (addr, 1); \ + print(int.from_bytes(int8_global_mv\[0\], byteorder='little'))" \ + "\r\n42" \ + "read memory without execution" + # The following tests require execution. if {![runto_main]} {