Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Andrew Burgess <aburgess@redhat.com>
To: gdb-patches@sourceware.org
Cc: Andrew Burgess <aburgess@redhat.com>, Tom Tromey <tom@tromey.com>
Subject: [PATCHv2 1/2] gdb: check auto-load safe path after refreshing cached vector
Date: Wed, 28 Oct 2026 11:40:56 +0000	[thread overview]
Message-ID: <7a6aa52038b5b67884f684ab4d223dd6f43a70bb.1793187406.git.aburgess@redhat.com> (raw)
In-Reply-To: <cover.1793187406.git.aburgess@redhat.com>

This commit fixes an issue that exists in the interaction between
file_is_auto_load_safe and filename_is_in_auto_load_safe_path_vec.  I
believe that this issue has existed since the code was originally
added in commit bccbefd2aab863e2.

The file_is_auto_load_safe function takes a FILENAME and checks if
that file is in the auto-load safe path.  To avoid the expense of
expanding the auto-load safe path on every check GDB pre-computes the
expanded safe path in the global AUTO_LOAD_SAFE_PATH_VEC.  The
filename_is_in_auto_load_safe_path_vec checks if FILENAME is within
the pre-computed vector or not.

However, FILENAME might also be a symlink to another file.  In this
case, we also want to check if the real file is in the pre-computed
vector.  To do this we can call gdb_realpath on FILENAME and check
this against the vector too.  This work is also done in the
filename_is_in_auto_load_safe_path_vec function.

If filename_is_in_auto_load_safe_path_vec doesn't find FILENAME or the
gdb_realpath equivalent in the auto-load safe path vector, then we
need to consider that the vector might be out of date and needs
recomputing.

This recomputing is done in file_is_auto_load_safe with a call to the
function auto_load_safe_path_vec_update.  After this we can call
filename_is_in_auto_load_safe_path_vec again to check against the
updated vector.

As both filename_is_in_auto_load_safe_path_vec calls might need to
call gdb_realpath, and calling gdb_realpath is not cheap, the
filename_is_in_auto_load_safe_path_vec allows the answer from the
first call to be saved and reused in the second call.  This also means
that should the filesystem change under GDB's feet then the same real
filename will be used by both filename_is_in_auto_load_safe_path_vec
calls.  This is the FILENAME_REALP argument that is passed to
filename_is_in_auto_load_safe_path_vec; this is initially NULL, and
might be set by the first call if FILENAME is not found in the
auto-load safe path vector.

There are some optimisations in place to try and avoid duplicate
work.  In filename_is_in_auto_load_safe_path_vec, if the computed
FILENAME_REALP value is the same as the original FILENAME, then there
is no need to check for FILENAME_REALP in AUTO_LOAD_SAFE_PATH_VEC.
This will be a very common case and occurs when FILENAME is the actual
file, not a symlink to some other file.

However, there is another piece of code which I think is a similar
idea, but is actually a mistake.  The first check in
filename_is_in_auto_load_safe_path_vec looks like this:

  for (const gdb::unique_xmalloc_ptr<char> &p : auto_load_safe_path_vec)
    if (*filename_realp == NULL && filename_is_in_pattern (filename, p.get ()))
      {
	pattern = p.get ();
	break;
      }

This is where we check if FILENAME is in AUTO_LOAD_SAFE_PATH_VEC.  The
first time that filename_is_in_auto_load_safe_path_vec is called the
*FILENAME_REALP will always be NULL (see file_is_auto_load_safe to
confirm this).

But if the first filename_is_in_auto_load_safe_path_vec call returns
false then gdb_realpath will have been called and *FILENAME_REALP will
no longer be NULL.  Only when the first call to
filename_is_in_auto_load_safe_path_vec returns false do we make a
second call to filename_is_in_auto_load_safe_path_vec.

This means that for the second filename_is_in_auto_load_safe_path_vec
call *FILENAME_REALP will never be NULL, and so based on the above
code, which checks '*filename_realp == NULL', we will not check if
FILENAME is in AUTO_LOAD_SAFE_PATH_VEC.

But we already discussed the optimisation that avoids checking for
FILENAME_REALP in AUTO_LOAD_SAFE_PATH_VEC when FILENAME and
FILENAME_REALP are the same (which is often the case).  So this means
that, when FILENAME is a real file, one for which gdb_realpath returns
the same filename unchanged, the second call to
filename_is_in_auto_load_safe_path_vec will actually not perform any
checks at all.

This feels like it has to be a mistake.

I think that this first '*filename_realp == NULL' check should be
removed.  Then, after we update the auto-load safe path vector, and
make a second call to filename_is_in_auto_load_safe_path_vec, we will
correctly check if FILENAME is in the now updated
AUTO_LOAD_SAFE_PATH_VEC.
---
 gdb/auto-load.c                               |   2 +-
 .../auto-load-after-debug-dir-change-1.gdb    |  17 +++
 .../auto-load-after-debug-dir-change-2.gdb    |  17 +++
 .../auto-load-after-debug-dir-change.exp      | 101 ++++++++++++++++++
 4 files changed, 136 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.base/auto-load-after-debug-dir-change-1.gdb
 create mode 100644 gdb/testsuite/gdb.base/auto-load-after-debug-dir-change-2.gdb
 create mode 100644 gdb/testsuite/gdb.base/auto-load-after-debug-dir-change.exp

diff --git a/gdb/auto-load.c b/gdb/auto-load.c
index ccbd091e1c2..371d24c2b37 100644
--- a/gdb/auto-load.c
+++ b/gdb/auto-load.c
@@ -457,7 +457,7 @@ filename_is_in_auto_load_safe_path_vec (const char *filename,
   const char *pattern = NULL;
 
   for (const gdb::unique_xmalloc_ptr<char> &p : auto_load_safe_path_vec)
-    if (*filename_realp == NULL && filename_is_in_pattern (filename, p.get ()))
+    if (filename_is_in_pattern (filename, p.get ()))
       {
 	pattern = p.get ();
 	break;
diff --git a/gdb/testsuite/gdb.base/auto-load-after-debug-dir-change-1.gdb b/gdb/testsuite/gdb.base/auto-load-after-debug-dir-change-1.gdb
new file mode 100644
index 00000000000..05505085e7f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/auto-load-after-debug-dir-change-1.gdb
@@ -0,0 +1,17 @@
+# Copyright 2026 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+echo script_1_loaded\n
+set $script_loaded=100
diff --git a/gdb/testsuite/gdb.base/auto-load-after-debug-dir-change-2.gdb b/gdb/testsuite/gdb.base/auto-load-after-debug-dir-change-2.gdb
new file mode 100644
index 00000000000..92bfe0f6b18
--- /dev/null
+++ b/gdb/testsuite/gdb.base/auto-load-after-debug-dir-change-2.gdb
@@ -0,0 +1,17 @@
+# Copyright 2026 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+echo script_2_loaded\n
+set $script_loaded=200
diff --git a/gdb/testsuite/gdb.base/auto-load-after-debug-dir-change.exp b/gdb/testsuite/gdb.base/auto-load-after-debug-dir-change.exp
new file mode 100644
index 00000000000..6cb7a0a8432
--- /dev/null
+++ b/gdb/testsuite/gdb.base/auto-load-after-debug-dir-change.exp
@@ -0,0 +1,101 @@
+# Copyright 2026 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test GDB's checking of the auto-load safe path via the $debugdir
+# variable.
+#
+# The test changes the auto-load search-path and the
+# debug-file-directory, and checks that GDB can keep up with these
+# changes as the executable is repeatedly reloaded.
+
+require {!is_remote host}
+
+standard_testfile main.c -1.gdb -2.gdb
+
+if { [build_executable "build executable" ${testfile} ${srcfile}] } {
+    return
+}
+
+set binfile_dir [file dirname $binfile]
+
+array set debug_dir {}
+foreach i { 1 2 } {
+    set debug_dir($i) [file normalize [standard_output_file "debug_dir_$i"]]
+    remote_exec host "mkdir -p $debug_dir($i)"
+    remote_exec host "mkdir -p $debug_dir($i)/$binfile_dir"
+}
+
+remote_exec host "cp $srcdir/$subdir/$srcfile2 $debug_dir(1)/$binfile_dir/$testfile-gdb.gdb"
+remote_exec host "cp $srcdir/$subdir/$srcfile3 $debug_dir(2)/$binfile_dir/$testfile-gdb.gdb"
+
+clean_restart
+
+# Only load scripts from the debug file directory.
+gdb_test_no_output "set auto-load safe-path \$debugdir"
+
+# Set the auto-load search path to be exactly the SEARCH_NUM entry in
+# the global debug_dir array.
+#
+# Then set the debug file directory to the DEBUG_NUM entry in the
+# global debug_dir array.  As GDB's default auto-load search path
+# includes '$debugdir', this means GDB will accept scripts found in
+# this directory.
+#
+# Finally, load the executable, and check to see if the expected
+# auto-load script was loaded.
+#
+# Every call to run_test_1 is done in the same GDB session so that any
+# caching of the auto-load safe path will carry over between calls to
+# this proc.
+proc run_test_1 { search_num debug_num } {
+    global debug_dir
+
+    gdb_test_no_output "set variable \$script_loaded = 0" \
+	"reset script_loaded variable to a known value"
+
+    # Where GDB should look for auto-load scripts.
+    gdb_test_no_output "set auto-load scripts-directory $debug_dir($search_num)" \
+	"setup scripts directory"
+
+    # The auto-load safe path is based on the debug file directory,
+    # which we setup here.
+    gdb_test_no_output "set debug-file-directory $debug_dir($debug_num)" \
+	"set debug-file-directory"
+
+    gdb_load ${::binfile}
+
+    gdb_test {print $script_loaded} " = ${search_num}00"
+
+    # Check that the expected script shows as auto-loaded in GDB's output.
+    gdb_assert {[regexp "script_${search_num}_loaded" $::gdb_file_cmd_msg]} \
+	"expected script was loaded"
+
+    # The '3 - SEARCH_NUM' converts: 2 => 1 and 1 => 2.  This checks
+    # that the other script was not auto-loaded.
+    gdb_assert {![regexp "script_[expr {3 - $search_num}]_loaded" $::gdb_file_cmd_msg]} \
+	"other script was not loaded"
+}
+
+# Wrapper around run_test_1 which adds a test prefix.  See run_test_1
+# for details.
+proc run_test { search_num debug_num } {
+    with_test_prefix "search=$search_num, debug=$debug_num" {
+	run_test_1 $search_num $debug_num
+    }
+}
+
+# Run the tests.
+run_test 1 1
+run_test 2 2
-- 
2.25.4


  reply	other threads:[~2026-10-28 11:42 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-21 21:47 [PATCH] gdb: add debug_file_directory changed observer Andrew Burgess
2026-10-23 18:40 ` Tom Tromey
2026-10-28 11:40   ` [PATCHv2 0/2] Address issues with auto-load safe path checking Andrew Burgess
2026-10-28 11:40     ` Andrew Burgess [this message]
2026-10-28 11:40     ` [PATCHv2 2/2] gdb: add debug_file_directory changed observer Andrew Burgess

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7a6aa52038b5b67884f684ab4d223dd6f43a70bb.1793187406.git.aburgess@redhat.com \
    --to=aburgess@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=tom@tromey.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox