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. */