From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 AC66A38582BE for ; Wed, 25 Oct 2023 15:08:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org AC66A38582BE 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 AC66A38582BE 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=1698246519; cv=none; b=J0gz2bZAvanYUuMQ99Bv2FraNwU0RYg2/vudVU0WaGHJZKJpEEuLuTldjmh2YQk3CpCWMNejnsouoie0obZcHCdrymPiGiRMDVIJyAVOJ41OJm0r0rCwarSP9Qr6Zb3UeqHwLpR3NVATLb7Kj0Zu6Y6k19rPkzHNQRiHqSMZaDU= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698246519; c=relaxed/simple; bh=g+IQpooFkjvNlk8uT8ZY6B7FAO/syiXL/mrhiWZ5iro=; h=DKIM-Signature:From:To:Subject:Date:Message-Id:MIME-Version; b=gk1mHDaKuhBtOdpXD7KWZvFaXsRUli3VFH588sbarECgIRH7wYtBeu7te/Wc/rindUrpAxEwgvxPBil1eDb75jNgw1h1Kbeew7/vsmdEUs/lxjzFtBIOZW6Jm1qiR43fRl2KbH9GmV/gW4cW8dNJmlQuWztuMQofG6ZBJwKObrQ= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1698246517; 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=CfZ+tBd01hrrB1QDah0XsV6jA/e/bkN0V8g09ifL1Dw=; b=hirzOw7bOIMvumn9FwmWLChd1VvFX5A0OAGD3E5DImF/V25fgsOt8b/Ym81+Epj8soR503 116YBVxwW8/ICtnW3R/J5zCYrVYjF/fJqx/E2wS3an1ASJmPhorZCPYAkzhOhp2XCAuDnD NPFXYYg1ssaAtLOFh2VuqudU5Acd2qI= Received: from mail-ej1-f71.google.com (mail-ej1-f71.google.com [209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-685-pjFwg91qOb-IejVRcP7MMQ-1; Wed, 25 Oct 2023 11:08:25 -0400 X-MC-Unique: pjFwg91qOb-IejVRcP7MMQ-1 Received: by mail-ej1-f71.google.com with SMTP id a640c23a62f3a-9bf1047cb28so354188366b.2 for ; Wed, 25 Oct 2023 08:08:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698246504; x=1698851304; 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=UCGCZGJyDM6K0wdpIzq8Mu+RGaK5L1E7nht74zxpaSU=; b=MqBeJGQk1g8ZULqQWCd3eDAHmY4mzZNu6Rzbt0H0PRdw88HkyioMagrXjswX6PwtYr nHc3s54B32BtiAP6WXaemg6yLZgN1hz5ASRjKFqBH6xbovtfFobLEB9ugEIYcbLGWWIN bhrk7wtAlVfMkYKXystZFt9naeG0mcSiyVEAtRY404QYQdW99rBSd7bMjIFvrQNu9tGu gzx2wkC7PaY5MvKEevgpX5Ey1VZwv9GuiKKeoXIZyh1REnAfv1eZO9JCLYELu0vE4FZ8 kPGfFrCgeSRnAIsZH7vK7T+axErI0heoPCtrPnSEUv4SreZP1e9TKM9tSawH27X/hDhT JMFg== X-Gm-Message-State: AOJu0YwWSh+2q1kD2zKchGLIngu3MFGJZaPTDRcMbAxY7QhGgRjTop4o nQ9N7G7t9mHZezbhi2s8mGgnwbB3iqc1HNSMrAlbzC45+qIhnrL5Dx8LlU3UL11f9URnUrIRQ2+ UoSNy9S0te+7gprntTIRz4sujmM0frUhCkHeuiyd9XCjW/FcddQ4/M8B6VNyoID2u7PXQ/9qM8S ZSWYJ9Bg== X-Received: by 2002:a17:907:1c93:b0:9bf:70ea:6926 with SMTP id nb19-20020a1709071c9300b009bf70ea6926mr13044083ejc.2.1698246504347; Wed, 25 Oct 2023 08:08:24 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGCaWQ6KOfaabQTr+gjua621CqJi09Y2H34auEfDUdyAjf5XRC/3nVG0ATuqrBh6jrrLir82g== X-Received: by 2002:a17:907:1c93:b0:9bf:70ea:6926 with SMTP id nb19-20020a1709071c9300b009bf70ea6926mr13044044ejc.2.1698246503737; Wed, 25 Oct 2023 08:08:23 -0700 (PDT) Received: from localhost ([31.111.84.209]) by smtp.gmail.com with ESMTPSA id s16-20020a1709066c9000b0099bc08862b6sm10161706ejr.171.2023.10.25.08.08.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 25 Oct 2023 08:08:23 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c Date: Wed, 25 Oct 2023 16:08:17 +0100 Message-Id: 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 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true X-Spam-Status: No, score=-11.6 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 List-Id: 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 replace 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. Nicely, there are existing observers for both of these cases (before_prompt and target_resumed respectively), so, in gdb_bfd.c I've hooked into both of these and arranged to clear the bfd cache. With this commit in place the next commit no longer shows any issues with gdb.base/attach.exp. One possible problem that I see with this commit is: what if some other user of one of the observers I've hooked into causes a bfd to be opened after my new observers have run? If this happened then we would proceed with a populated bfd cache, and this might causes problems. Right now, the only other users of the observers that I'm connecting too are the extension languages, specifically, Python. I suspect it must be possible for Python to carry out actions that can cause the bfd cache to be populated, so maybe there is a risk here. To counter this risk, we could move the bfd_cache_close_all() calls out of observers, and move them into GDB core close too, but after the two observers in questions are actually called, so into top_level_prompt for the before_prompt observer and into notify_target_resumed for the target_resumed observer. Another choice would be to add a bfd_cache_close_all() into gdbpy_enter::~gdbpy_enter, so whenever we call into a Python extension, we always clear the bfd cache once we're done. For now I haven't made either of these changes, maybe I'm worrying about nothing? I'd appreciate peoples thoughts. 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/exec.c | 2 -- gdb/gdb_bfd.c | 20 ++++++++++++++++++++ gdb/infcmd.c | 1 - gdb/inferior.c | 2 -- gdb/symfile.c | 1 - gdb/target.c | 5 ----- 7 files changed, 20 insertions(+), 16 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/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/gdb_bfd.c b/gdb/gdb_bfd.c index 56a4c5ecc91..9b7a06da231 100644 --- a/gdb/gdb_bfd.c +++ b/gdb/gdb_bfd.c @@ -33,6 +33,7 @@ #include "gdbsupport/fileio.h" #include "inferior.h" #include "cli/cli-style.h" +#include "observable.h" #include /* An object of this type is stored in the section's user data when @@ -1171,10 +1172,29 @@ gdb_bfd_error_handler (const char *fmt, va_list ap) (*default_bfd_error_handler) (fmt, ap); } +/* A before_prompt observer. */ + +static void +gdb_bfd_before_prompt (const char * /* ignored */) +{ + bfd_cache_close_all (); +} + +/* A target_resumed observer. */ + +static void +gdb_bfd_target_resumed (ptid_t /* ignored */) +{ + bfd_cache_close_all (); +} + void _initialize_gdb_bfd (); void _initialize_gdb_bfd () { + gdb::observers::target_resumed.attach (gdb_bfd_target_resumed, "gdb-bfd"); + gdb::observers::before_prompt.attach (gdb_bfd_before_prompt, "gdb-bfd"); + all_bfds = htab_create_alloc (10, htab_hash_pointer, htab_eq_pointer, NULL, xcalloc, xfree); 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 -- 2.25.4