From patchwork Mon Oct 30 13:41:22 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 78752 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 7EBF23857355 for ; Mon, 30 Oct 2023 13:41:50 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 04F023858D28 for ; Mon, 30 Oct 2023 13:41:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 04F023858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none 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 04F023858D28 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=1698673297; cv=none; b=H6yqkbbYssmS2tXFcPFHLQwY5zWYlrbvsZSRUAE3J0dKkaNrawMJ5Xy7rZ0RQYZ9PETAeHsKAJgNFF1DVRBrPiTLeL/+wWdHlsUyP3kTO/hGmKSdbunb5NNtyaUgDa4NJjITlvlGeZPxPA8BREqFs4Wb5lqnWcBBancghoGUTA8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698673297; c=relaxed/simple; bh=zIZ2iwni+r7h9qboMCelzxB5/aT+ypXYweZS4OOUXYY=; h=DKIM-Signature:From:To:Subject:Date:Message-Id:MIME-Version; b=pHyOE4lmxxOPqnJdFGXK88Zmo6tk1lShkgiBYvPMCcpfQ6iHK/LjJzWBQXzBWvTRTb7O2DAJGMEzSgcjq32DEL5jQhvaGL0HWRDbwWbATjOjd0X03D/D901t+MX4CMUWsY30kdnhCWG0g2aLzrd3/xXcjujqeMtRBqvwP9Ybiv8= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1698673294; 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=dV4giWC6s0y06kho2MMNQkM/KEwd2VsISnkMjkeQnLA=; b=NYTdi+Emu9vaWhpHCR//S/f4h1y95SZ3goRYcJq7TkWxusI4MRHDRDQeLWdcEXD9sgG4I8 +nRV/Pjc5h42QivWcWLaa2ZrLsi0l3voYUOpK++qdzoSlo9qmfBx7CpzTbO//7V/VmX5TB ao0u/cXNfNxU389qsVMjy/nUI1YHf88= Received: from mail-ej1-f72.google.com (mail-ej1-f72.google.com [209.85.218.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-223-NpCudmCBNRakHRJ47lIexw-1; Mon, 30 Oct 2023 09:41:31 -0400 X-MC-Unique: NpCudmCBNRakHRJ47lIexw-1 Received: by mail-ej1-f72.google.com with SMTP id a640c23a62f3a-9b2cf504e3aso308857366b.2 for ; Mon, 30 Oct 2023 06:41:31 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698673290; x=1699278090; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=D+gIFdKT4GFusBCMQ9HE2FA+hSD+oEcGL/cIWn+SIGY=; b=dS2Ww5NR37zxgfPWWoQjz0ehxJzmsDsXqQVpFg6jtwKVOeG7MJQB884DSgt/S/KpPp XNVW1otRPgllRQzwVR4DGYPas4NyqhVJr6Mr7B8MvOdJ6VquK0hM97OJOzFrXJUDut+t 4ECW2QBQ+q+4b/q/zyfxMOnhDGtno411Co2kbh88Yo3jWA6xY5nLVEGCHun4pJLOnX9i Adg/IUkpYObzqj6PiiRzkpjyf/0WUIe9ExtwHjh7I9TDbZmFFtQPoCexhaSMR4/GgcHN j8dCmohq7EklhKIYM/qbAlQkV5amoNy5+rBNrxQRKEh9x3r8J8dN6xeTkgiDbksOTXxs au6g== X-Gm-Message-State: AOJu0YyExcYb8Vtmypf1TOXXXqijM+tGKcrKY0wnkvuOmjAv2HnlhiBh 1d00qwQXwq3zyuOGj2f/E7SASj64ay0+OauiG7uA84B887a4P3h9FOV4IQvAHnmgpUiSQ/njT7E e0dcbLxV+6F6XF18hLKVDjVYwDAQ2YLUvKR4dIZdrJKsU1gjZ0E7pjBpzazTc3IXhQ/sk7HK6Xr TKwqASbg== X-Received: by 2002:a17:907:720f:b0:9be:4bb0:64f with SMTP id dr15-20020a170907720f00b009be4bb0064fmr7781921ejc.54.1698673289930; Mon, 30 Oct 2023 06:41:29 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFw5JeKaj3TlMuTsFHQBdGFD/l6fA6Ronv2PDpS5csiCMq3Im01Eygv60B71B5dPuuJr9oQ6A== X-Received: by 2002:a17:907:720f:b0:9be:4bb0:64f with SMTP id dr15-20020a170907720f00b009be4bb0064fmr7781895ejc.54.1698673289390; Mon, 30 Oct 2023 06:41:29 -0700 (PDT) Received: from localhost ([31.111.84.209]) by smtp.gmail.com with ESMTPSA id vr7-20020a170906bfe700b009add084a00csm5971866ejb.36.2023.10.30.06.41.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Oct 2023 06:41:28 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess , Lancelot SIX , Tom Tromey Subject: [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c Date: Mon, 30 Oct 2023 13:41:22 +0000 Message-Id: <5c0675d36f8e92891826782116b93271a3a8f933.1698673143.git.aburgess@redhat.com> X-Mailer: git-send-email 2.25.4 In-Reply-To: References: MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00, DKIM_INVALID, DKIM_SIGNED, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP 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.30 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 In the following commit I ran into a problem. The next commit aims to improve GDB's handling of the main executable being a file on a remote target (i.e. one with a 'target:' prefix). To do this I have replaced a system 'stat' call with a bfd_stat call. However, doing this caused a regression in gdb.base/attach.exp. The problem is that the bfd library caches open FILE* handles for bfd objects that it has accessed, which is great for short-lived, non interactive programs (e.g. the assembler, or objcopy, etc), however, for GDB this caching causes us a problem. If we open the main executable as a bfd then the bfd library will cache the open FILE*. If some time passes, maybe just sat at the GDB prompt, or with the inferior running, and then later we use bfd_stat to check if the underlying, on-disk file has changed, then the bfd library will actually use fstat on the underlying file descriptor. This is of course slightly different than using system stat on with the on-disk file name. If the on-disk file has changed then system stat will give results for the current on-disk file. But, if the bfd cache is still holding open the file descriptor for the original on-disk file (from before the change) then fstat will return a result based on the original file, and so show no change as having happened. This is a known problem in GDB, and so far this has been solved by scattering bfd_cache_close_all() calls throughout GDB. But, as I said, in the next commit I've made a change and run into a problem (gdb.base/attach.exp) where we are apparently missing a bfd_cache_close_all() call. Now I could solve this problem by adding a bfd_cache_close_all() call before the bfd_stat call that I plan to add in the next commit, that would for sure solve the problem, but feels a little crude. Better I think would be to track down where the bfd is being opened and add a corresponding bfd_cache_close_all() call elsewhere in GDB once we've finished doing whatever it is that caused us to open the bfd in the first place. This second solution felt like the better choice, so I tracked the problem down to elf_locate_base and fixed that. But that just exposed another problem in gdb_bfd_map_section which was also re-opening the bfd, so I fixed this (with another bfd_cache_close_all() call), and that exposed another issue in gdbarch_lookup_osabi... and at this point I wondered if I was approaching this problem the wrong way... .... And so, I wonder, is there a _better_ way to handle these bfd_cache_close_all() calls? I see two problems with the current approach: 1. It's fragile. Folk aren't always aware that they need to clear the bfd cache, and this feels like something that is easy to overlook in review. So adding new code to GDB can innocently touch a bfd, which populates the cache, which will then be a bug that can lie hidden until an on-disk file just happens to change at the wrong time ... and GDB fails to spot the change. Additionally, 2. It's in efficient. The caching is intended to stop the bfd library from continually having to re-open the on-disk file. If we have a function that touches a bfd then often that function is the obvious place to call bfd_cache_close_all. But if a single GDB command calls multiple functions, each of which touch the bfd, then we will end up opening and closing the same on-disk file multiple times. It feels like we would be better postponing the bfd_cache_close_all call until some later point, then we can benefit from the bfd cache. So, in this commit I propose a new approach. We now clear the bfd cache in two places: (a) Just before we display a GDB prompt. We display a prompt after completing a command, and GDB is about to enter an idle state waiting for further input from the user (or in async mode, for an inferior event). If while we are in this idle state the user changes the on-disk file(s) then we would like GDB to notice this the next time it leaves its idle state, e.g. the next time the user executes a command, or when an inferior event arrives, (b) When we resume the inferior. In synchronous mode, resuming the inferior is another time when GDB is blocked and sitting idle, but in this case we don't display a prompt. As with (a) above, when an inferior event arrives we want GDB to notice any changes to on-disk files. It turns out that there are existing observers for both of these cases (before_prompt and target_resumed respectively), so my initial thought was that I should attach to these observers in gdb_bfd.c, and in both cases call bfd_cache_close_all(). And this does indeed solve the gdb.base/attach.exp problem that I see with the following commit. However, I see a problem with this solution. Both of the observers I'm using are exposed through the Python API as events that a user can hook into. The user can potentially run any GDB command (using gdb.execute), so Python code might end up causing some bfds to be reopened, and inserted into the cache. To solve this one solution would be to add a bfd_cache_close_all() call into gdbpy_enter::~gdbpy_enter(). Unfortunately, there's no similar enter/exit object for Guile, though right now Guile doesn't offer the same event API, so maybe we could just ignore that problem... but this doesn't feel great. So instead, I think a better solution might be to not use observers for the bfd_cache_close_all() calls. Instead, I'll call bfd_cache_close_all() directly from core GDB after we've notified the before_prompt and target_resumed observers, this was we can be sure that the cache is cleared after the observers have run, and before GDB enters an idle state. This commit also removes all of the other bfd_cache_close_all() calls from GDB. My claim is that these are no longer needed. --- gdb/corefile.c | 5 ----- gdb/event-top.c | 20 +++++++++++++++++--- gdb/exec.c | 2 -- gdb/infcmd.c | 1 - gdb/inferior.c | 2 -- gdb/symfile.c | 1 - gdb/target.c | 5 ----- gdb/thread.c | 5 +++++ 8 files changed, 22 insertions(+), 19 deletions(-) diff --git a/gdb/corefile.c b/gdb/corefile.c index c27061a3ae3..19a96bc6f86 100644 --- a/gdb/corefile.c +++ b/gdb/corefile.c @@ -120,11 +120,6 @@ reopen_exec_file (void) && current_program_space->ebfd_mtime && current_program_space->ebfd_mtime != st.st_mtime) exec_file_attach (filename.c_str (), 0); - else - /* If we accessed the file since last opening it, close it now; - this stops GDB from holding the executable open after it - exits. */ - bfd_cache_close_all (); } /* If we have both a core file and an exec file, diff --git a/gdb/event-top.c b/gdb/event-top.c index 3d6fa896a9c..b8378f4e653 100644 --- a/gdb/event-top.c +++ b/gdb/event-top.c @@ -462,6 +462,22 @@ display_gdb_prompt (const char *new_prompt) } } +/* Notify the 'before_prompt' observer, and run any additional actions + that must be done before we display the prompt. */ +static void +notify_before_prompt (const char *prompt) +{ + /* Give observers a chance of changing the prompt. E.g., the python + `gdb.prompt_hook' is installed as an observer. */ + gdb::observers::before_prompt.notify (prompt); + + /* As we are about to display the prompt, and so GDB might be sitting + idle for some time, close all the cached BFDs. This ensures that + when we next start running a user command all BFDs will be reopened + as needed, and as a result, we will see any on-disk changes. */ + bfd_cache_close_all (); +} + /* Return the top level prompt, as specified by "set prompt", possibly overridden by the python gdb.prompt_hook hook, and then composed with the prompt prefix and suffix (annotations). */ @@ -469,9 +485,7 @@ display_gdb_prompt (const char *new_prompt) static std::string top_level_prompt (void) { - /* Give observers a chance of changing the prompt. E.g., the python - `gdb.prompt_hook' is installed as an observer. */ - gdb::observers::before_prompt.notify (get_prompt ().c_str ()); + notify_before_prompt (get_prompt ().c_str ()); const std::string &prompt = get_prompt (); diff --git a/gdb/exec.c b/gdb/exec.c index 5956012338f..59965b84d55 100644 --- a/gdb/exec.c +++ b/gdb/exec.c @@ -500,8 +500,6 @@ exec_file_attach (const char *filename, int from_tty) (*deprecated_exec_file_display_hook) (filename); } - bfd_cache_close_all (); - /* Are are loading the same executable? */ bfd *prev_bfd = exec_bfd_holder.get (); bfd *curr_bfd = current_program_space->exec_bfd (); diff --git a/gdb/infcmd.c b/gdb/infcmd.c index cf8cd527955..5153843dde8 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -2498,7 +2498,6 @@ kill_command (const char *arg, int from_tty) int infnum = current_inferior ()->num; target_kill (); - bfd_cache_close_all (); update_previous_thread (); diff --git a/gdb/inferior.c b/gdb/inferior.c index 1778723863e..927c5f16ae2 100644 --- a/gdb/inferior.c +++ b/gdb/inferior.c @@ -714,8 +714,6 @@ kill_inferior_command (const char *args, int from_tty) target_kill (); } - - bfd_cache_close_all (); } /* See inferior.h. */ diff --git a/gdb/symfile.c b/gdb/symfile.c index eebc5ea44b9..24570372316 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -1124,7 +1124,6 @@ symbol_file_add_with_addrs (const gdb_bfd_ref_ptr &abfd, const char *name, gdb::observers::new_objfile.notify (objfile); - bfd_cache_close_all (); return objfile; } diff --git a/gdb/target.c b/gdb/target.c index f688ff33e3b..aeb53dd91d0 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -2729,11 +2729,6 @@ target_mourn_inferior (ptid_t ptid) { gdb_assert (ptid.pid () == inferior_ptid.pid ()); current_inferior ()->top_target ()->mourn_inferior (); - - /* We no longer need to keep handles on any of the object files. - Make sure to release them to avoid unnecessarily locking any - of them while we're not actually debugging. */ - bfd_cache_close_all (); } /* Look for a target which can describe architectural features, starting diff --git a/gdb/thread.c b/gdb/thread.c index c8145da59bc..a1e003b082e 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -876,6 +876,11 @@ notify_target_resumed (ptid_t ptid) { interps_notify_target_resumed (ptid); gdb::observers::target_resumed.notify (ptid); + + /* We are about to resume the inferior. Close all cached BFDs so that + when the inferior next stops, and GDB regains control, we will spot + any on-disk changes to the BFDs we are using. */ + bfd_cache_close_all (); } /* See gdbthread.h. */ From patchwork Mon Oct 30 13:41:23 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 78753 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 A89C63858438 for ; Mon, 30 Oct 2023 13:42:28 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 6B2D43858D33 for ; Mon, 30 Oct 2023 13:41:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6B2D43858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none 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 6B2D43858D33 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698673297; cv=none; b=Sca56XzGkyeyDT5sItjmUFxFVZzKJtxT976w+slooKi9O9796tHvSk+f+mjst+/hJEFy18gH9nziA750JefjfKolNRS4aCtckcFaFHyQdScUniBBL8g9qOvTD4CHieFP81YQmyI3nt1Gxb0t7+U7TO/Mapuz7ibiyjvpJFHWvO0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698673297; c=relaxed/simple; bh=YRj6tm+5vT/c6qxsRak93rj98Ht0zDCItWDpim8y2pI=; h=DKIM-Signature:From:To:Subject:Date:Message-Id:MIME-Version; b=hIVlnfyOY+ceA7kFsytgdTqhmHIUX8mTJxArPr2/X/RkH5tv1IteR48JWPl2ut6xpE4a5QhIUpVol1Ez5lox/LNv/yny3t7KCe7Hvcghbmj7dNRAlICJ9UVTrXvLGRaMIK1e+zt41sTjWE3TB0he27Pnks6e7DM+i3xF8WMfHLo= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1698673295; 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=/L7szC+hdSCIpWopk4hyCoKmWRDtN5ZQ1A57O6ccji8=; b=KZnkrGL6QH5sm1Wlx2Hkv+0QfBeuMFbM5A7ldF1ME4uFNMgbsQoPPXOMK5U8rRKKYXEQEo viz76gymLlz988FSNXhPJ1L5zJkgReO+zzpLcn0L1D3uogRfmEGMujwTpT9NmzRkB8renh j/Ox1C7WgQrJQFIFgfm1L+kww9r1L1I= Received: from mail-lf1-f71.google.com (mail-lf1-f71.google.com [209.85.167.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-586-wCG0dU8TPWeiRbJqKGaDYA-1; Mon, 30 Oct 2023 09:41:33 -0400 X-MC-Unique: wCG0dU8TPWeiRbJqKGaDYA-1 Received: by mail-lf1-f71.google.com with SMTP id 2adb3069b0e04-5082705f6dfso3577416e87.2 for ; Mon, 30 Oct 2023 06:41:33 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698673292; x=1699278092; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=9N3sRZd1Onx/5tfYFweKgi7cpeb0Xb/RzOPaDtOTgaY=; b=VhCCtyu1z1b0Dl3FRNPISWkFHNrlwBCzZAIrB+X/fVw4B7scvVzgZZQlhH7lUAoKdn kJa8Z7g/vM6/bQXXcKNbbXYBRdeN3PJLtpAJ7px2JGyNYASmI3OOA1CAOE1YkHndChv/ 91NKE6mUvMVXuL5hOIy/DOw0sPzXV46GuoQlMVHnL7/ess6F6NyFMXfHdsF6w0KLvOQ/ UjztSQHmVKpGCEtjvbmk4wxjpLVJKl3oU6S69RYDlFi8sVSKPstf33Fx9NaQcoWe84A1 w6e4ieJ9pFFME9ZBkA9nP+gIE1y99hwQ1+Pyy3fNK5wHYwNErgBApT4zTE3oiRhnjbJl 876w== X-Gm-Message-State: AOJu0YwtnpfvYojuVf/OJaDLMlNEpE0Uuc4xbiM2xbRwzdXDPN6zT3DD P3PONlZwBw12QxDBTcu9AhrqeHvKioHcbKJb8FZbiPTcl341U6IZlWV+tRHF7jNnltMjX2rG7mS zmO+YItahrMEe+L8d9rroDrIJKBsXXwzcU/Q7pB2sh699zJuc1GhCwqtM0Pg+Mz2Oh04r0BRejo uyAnipSg== X-Received: by 2002:ac2:5a02:0:b0:507:a624:3f35 with SMTP id q2-20020ac25a02000000b00507a6243f35mr7160048lfn.41.1698673291973; Mon, 30 Oct 2023 06:41:31 -0700 (PDT) X-Google-Smtp-Source: AGHT+IF0EjZHrtaaXB1OqJJv9Dw6NNS287N2UO7AdntMkmicyV6TDpqpsQ6CnJgf12a6FRatiK9Gpw== X-Received: by 2002:ac2:5a02:0:b0:507:a624:3f35 with SMTP id q2-20020ac25a02000000b00507a6243f35mr7160022lfn.41.1698673291456; Mon, 30 Oct 2023 06:41:31 -0700 (PDT) Received: from localhost ([31.111.84.209]) by smtp.gmail.com with ESMTPSA id z6-20020a1709060f0600b009944e955e19sm6027315eji.30.2023.10.30.06.41.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Oct 2023 06:41:31 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess , Lancelot SIX , Tom Tromey Subject: [PATCH 2/2] gdb: fix reopen_exec_file for files with target: prefix Date: Mon, 30 Oct 2023 13:41:23 +0000 Message-Id: <06f0d493791c0a9523484e5a3c16ba5b9f3485d5.1698673143.git.aburgess@redhat.com> X-Mailer: git-send-email 2.25.4 In-Reply-To: References: MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00, DKIM_INVALID, DKIM_SIGNED, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP 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.30 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 Following on from this commit: commit f2c4f78c813a9cef38b7e9c9ad18822fb9e19345 Date: Thu Sep 21 16:35:30 2023 +0100 gdb: fix reread_symbols when an objfile has target: prefix In this commit I update reopen_exec_file to correctly handle executables with a target: prefix. Before this commit we used the system 'stat' call, which obviously isn't going to work for files with a target: prefix (files located on a possibly remote target machine). By switching to bfd_stat we will use remote fileio to stat the remote files, which means we should now correctly detect changes in a remote executable. The program_space::ebfd_mtime variable, with which we compare the result of bfd_stat is set with a call to bfd_get_mtime, which in turn calls bfd_stat, so comparing to the result of calling bfd_stat makes sense (I think). As I discussed in the commit f2c4f78c813a, if a BFD is an in-memory BFD, then calling bfd_stat will always return 0, while bfd_get_mtime will always return the time at which the BFD was created. As a result comparing the results will always show the file having changed. I don't believe that GDB can set the main executable to an in-memory BFD object, so, in this commit, I simply assert that the executable is not in-memory. If this ever changes then we would need to decide how to handle this case -- always reload, or never reload. The assert doesn't appear to trigger for our current test suite. --- gdb/corefile.c | 17 +++-- gdb/testsuite/gdb.server/target-exec-file.exp | 70 +++++++++++++++++++ 2 files changed, 80 insertions(+), 7 deletions(-) diff --git a/gdb/corefile.c b/gdb/corefile.c index 19a96bc6f86..b9c204d18dc 100644 --- a/gdb/corefile.c +++ b/gdb/corefile.c @@ -105,21 +105,24 @@ specify_exec_file_hook (void (*hook) (const char *)) void reopen_exec_file (void) { - int res; - struct stat st; + bfd *exec_bfd = current_program_space->exec_bfd (); /* Don't do anything if there isn't an exec file. */ - if (current_program_space->exec_bfd () == NULL) + if (exec_bfd == nullptr) return; + /* The main executable can't be an in-memory BFD object. If it was then + the use of bfd_stat below would not work as expected. */ + gdb_assert ((exec_bfd->flags & BFD_IN_MEMORY) == 0); + /* If the timestamp of the exec file has changed, reopen it. */ - std::string filename = bfd_get_filename (current_program_space->exec_bfd ()); - res = stat (filename.c_str (), &st); + struct stat st; + int res = bfd_stat (exec_bfd, &st); if (res == 0 - && current_program_space->ebfd_mtime + && current_program_space->ebfd_mtime != 0 && current_program_space->ebfd_mtime != st.st_mtime) - exec_file_attach (filename.c_str (), 0); + exec_file_attach (bfd_get_filename (exec_bfd), 0); } /* If we have both a core file and an exec file, diff --git a/gdb/testsuite/gdb.server/target-exec-file.exp b/gdb/testsuite/gdb.server/target-exec-file.exp index 9260df8b88d..40863538785 100644 --- a/gdb/testsuite/gdb.server/target-exec-file.exp +++ b/gdb/testsuite/gdb.server/target-exec-file.exp @@ -52,6 +52,19 @@ set target_exec [gdb_remote_download target $binfile] # prompt us if this is the right thing to do. gdb_test_no_output "set confirm off" +if { [allow_python_tests] } { + # Register an event handler for the executable changed event. + # This handler just copies the event into a global Python object. + gdb_test_multiline "Add connection_removed event" \ + "python" "" \ + "global_exec_changed_event = None" "" \ + "def executable_changed(event):" "" \ + " global global_exec_changed_event" "" \ + " global_exec_changed_event = event" "" \ + "gdb.events.executable_changed.connect (executable_changed)" "" \ + "end" "" +} + # Start gdbserver, but always in extended-remote mode, and then # connect to it from GDB. set res [gdbserver_start "--multi" $target_exec] @@ -59,6 +72,22 @@ set gdbserver_protocol "extended-remote" set gdbserver_gdbport [lindex $res 1] gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport +if { [allow_python_tests] } { + # When connecting to a remote target, if the user has not told GDB + # which executable to use, then GDB will figure out an executable + # from the remote target. + # + # As a result we expect to have seen an executable changed event. + with_test_prefix "after connecting" { + gdb_test "python print(global_exec_changed_event)" \ + "" + gdb_test "python print(global_exec_changed_event.progspace.executable_filename)" \ + [string_to_regexp target:$target_exec] + gdb_test "python print(global_exec_changed_event.reload)" "False" + gdb_test_no_output "python global_exec_changed_event = None" + } +} + # Issue a 'file' command and parse the output. We look for a couple # of specific things to ensure that we are correctly reading the exec # from the remote target. @@ -104,6 +133,20 @@ gdb_assert { $saw_read_of_remote_exec } \ gdb_assert { $saw_read_of_syms_from_exec } \ "symbols were read from remote exec file" +if { [allow_python_tests] } { + # The 'file' command forces GDB to always load the executable, + # even if the same filename is used. In this case, as the + # filename is the same, this will show as a reload event. + with_test_prefix "after 'file' command" { + gdb_test "python print(global_exec_changed_event)" \ + "" + gdb_test "python print(global_exec_changed_event.progspace.executable_filename)" \ + [string_to_regexp target:$target_exec] + gdb_test "python print(global_exec_changed_event.reload)" "True" + gdb_test_no_output "python global_exec_changed_event = None" + } +} + # Start the inferior (with the 'start' command), use TESTNAME for any # pass/fail calls. EXPECT_REREAD should be true or false and # indicates if we expect to too a line like: @@ -155,10 +198,24 @@ proc start_inferior { testname expect_reread } { # see the symbols re-read now. start_inferior "start inferior the first time" false +if { [allow_python_tests] } { + # The executable hasn't changed. + with_test_prefix "after starting inferior for the first time" { + gdb_test "python print(global_exec_changed_event)" "None" + } +} + # Re-start the inferior. The executable is unchanged so we should not # see the symbol file being re-read. start_inferior "start inferior a second time" false +if { [allow_python_tests] } { + # The executable still hasn't changed. + with_test_prefix "after starting inferior for the second time" { + gdb_test "python print(global_exec_changed_event)" "None" + } +} + # Delay for a short while so, when we touch the exec, we know the # timestamp will change. sleep 1 @@ -172,3 +229,16 @@ if { $status != 0 } { # Start the inferior again, we expect to see the symbols being re-read # from the remote file. start_inferior "start inferior a third time" true + +if { [allow_python_tests] } { + # The executable has now changed on disk. This will be a reload + # event. + with_test_prefix "after starting inferior for the third time" { + gdb_test "python print(global_exec_changed_event)" \ + "" + gdb_test "python print(global_exec_changed_event.progspace.executable_filename)" \ + [string_to_regexp target:$target_exec] + gdb_test "python print(global_exec_changed_event.reload)" "True" + gdb_test_no_output "python global_exec_changed_event = None" + } +}