gdb/py: fix gdb.parameter('data-directory')

Message ID 20210326174742.2640177-1-andrew.burgess@embecosm.com
State New
Headers show
Series
  • gdb/py: fix gdb.parameter('data-directory')
Related show

Commit Message

Andrew Burgess March 26, 2021, 5:47 p.m.
It was reported on IRC that using gdb.parameter('data-directory')
doesn't work correctly.

The problem is that the data directory is stored in 'gdb_datadir',
however the set/show command is associated with a temporary
'staged_gdb_datadir'.

When the user does 'set data-directory VALUE', the VALUE is stored in
'staged_gdb_datadir' by GDB, then set_gdb_datadir is called.  This in
turn calls set_gdb_data_directory to copy the value from
staged_gdb_datadir into gdb_datadir.

However, set_gdb_data_directory will resolve relative paths, so the
value stored in gdb_datadir might not match the value in
staged_gdb_datadir.

The Python gdb.parameter API fetches the parameter values by accessing
the variable associated with the show command, so in this case
staged_gdb_datadir.  This causes two problems:

1. Initially staged_gdb_datadir is NULL, and remains as such until the
user does 'set data-directory VALUE' (which might never happen), but
gdb_datadir starts with GDB's default data-directory value.  So
initially from Python gdb.parameter('data-directory') will return the
empty string, even though at GDB's CLI 'show data-directory' prints a
real path.

2. If the user does 'set data-directory ./some/relative/path', GDB
will resolve the relative path, thus, 'show data-directory' at the CLI
will print an absolute path.  However, the value is staged_gdb_datadir
will still be the relative path, and gdb.parameter('data-directory')
from Python will return the relative path.

In this commit I fix both of these issues by:

1. Initialising the value in staged_gdb_datadir based on the initial
value in gdb_datadir, and

2. In set_gdb_datadir, after calling set_gdb_data_directory, I copy
the value in gdb_datadir back into staged_gdb_datadir.

With these two changes in place the value in staged_gdb_datadir should
always match the value in gdb_datadir, and accessing data-directory
from Python should now work correctly.

gdb/ChangeLog:

	* top.c (staged_gdb_datadir): Update comment.
	(set_gdb_datadir): Copy the value of gdb_datadir back into
	staged_datadir.
	(init_main): Initialise staged_gdb_datadir.

gdb/testsuite/ChangeLog:

	* gdb.python/py-parameter.exp: Add test for reading data-directory
	using gdb.parameter API.
---
 gdb/ChangeLog                             |  7 ++++
 gdb/testsuite/ChangeLog                   |  5 +++
 gdb/testsuite/gdb.python/py-parameter.exp | 51 +++++++++++++++++++++++
 gdb/top.c                                 | 17 +++++++-
 4 files changed, 78 insertions(+), 2 deletions(-)

-- 
2.25.4

Comments

Tom Tromey April 1, 2021, 6:23 p.m. | #1
>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:


Andrew> The problem is that the data directory is stored in 'gdb_datadir',
Andrew> however the set/show command is associated with a temporary
Andrew> 'staged_gdb_datadir'.

There are many more messy cases with gdb.parameter.

Most of them are caused by the same problem, which is that validation
runs at the wrong moment.  gdb uses a post-setting hook, but really
validation should come first.

For some parameters, it would be useful to have a method to get a
"cooked" value, perhaps optionally "looking through" auto settings as
well.

Anyhow.

Andrew> gdb/ChangeLog:

Andrew> 	* top.c (staged_gdb_datadir): Update comment.
Andrew> 	(set_gdb_datadir): Copy the value of gdb_datadir back into
Andrew> 	staged_datadir.
Andrew> 	(init_main): Initialise staged_gdb_datadir.

This seems reasonable to me.  Thank you.

Tom

Patch

diff --git a/gdb/testsuite/gdb.python/py-parameter.exp b/gdb/testsuite/gdb.python/py-parameter.exp
index ef24dbe5d16..5543b21f3ac 100644
--- a/gdb/testsuite/gdb.python/py-parameter.exp
+++ b/gdb/testsuite/gdb.python/py-parameter.exp
@@ -37,6 +37,57 @@  if { [is_remote host] } {
 }
 gdb_test "python print (gdb.parameter ('directories'))" $directories
 
+# Check we can correctly read the data-directory parameter.  First,
+# grab the value as read directly from the GDB CLI.
+set dd ""
+gdb_test_multiple "show data-directory" \
+    "find the initial data-directory value" {
+	-re -wrap "GDB's data directory is \"(\[^\r\n\]+)\"\\." {
+	    set dd $expect_out(1,string)
+	    pass $gdb_test_name
+	}
+    }
+
+# Now print the data-directory from Python.
+gdb_test "python print (gdb.parameter ('data-directory'))" $dd
+
+# Next change the data-directory to a relative path.  Internally GDB
+# will resolve this to an absolute path, which Python should then see.
+#
+# GDB is currently running in '...../build/gdb/testsuite/' and the
+# test output is being written to:
+#   ...../build/gdb/testsuite/outputs/gdb.python/py-parameter/
+#
+# So create the relative path './outputs/gdb.python/py-parameter/' and
+# set the data-directory to that, we should then see the absolute path.
+
+set abs_path_to_output_dir [standard_output_file ""]
+set abs_path_to_cwd $objdir
+set rel_path_to_output_dir \
+    [file join "." [string replace ${abs_path_to_output_dir} 0 \
+			[string length ${abs_path_to_cwd}] ""]]
+gdb_test_no_output "set data-directory ${rel_path_to_output_dir}"
+
+gdb_test "python print (gdb.parameter ('data-directory'))" \
+    ${abs_path_to_output_dir} \
+    "python sees absolute version of data-directory path"
+
+# While we're here, check we see the correct path at GDB's CLI.
+gdb_test "show data-directory" \
+    "GDB's data directory is \"${abs_path_to_output_dir}\"\\." \
+    "check modified data-directory at the CLI"
+
+# Now lets set the data-directory back to what it was initially.
+gdb_test_no_output "set data-directory ${dd}"
+
+# And check we see the restored value at CLI and from Python.
+gdb_test "show data-directory" \
+    "GDB's data directory is \"${dd}\"\\." \
+    "check original data-directory was restored at the CLI"
+
+gdb_test "python print (gdb.parameter ('data-directory'))" ${dd} \
+    "python sees restored data-directory value"
+
 # Test a simple boolean parameter.
 with_test_prefix "boolean parameter" {
     gdb_test_multiline "Simple gdb booleanparameter" \
diff --git a/gdb/top.c b/gdb/top.c
index 3be95079654..99ac8fef137 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -2103,7 +2103,10 @@  show_exec_done_display_p (struct ui_file *file, int from_tty,
 		    value);
 }
 
-/* New values of the "data-directory" parameter are staged here.  */
+/* New values of the "data-directory" parameter are staged here.
+   Extension languages, for example Python's gdb.parameter API, will read
+   the value directory from this variable, so we must ensure that this
+   always contains the correct value.  */
 static char *staged_gdb_datadir;
 
 /* "set" command for the gdb_datadir configuration variable.  */
@@ -2112,6 +2115,14 @@  static void
 set_gdb_datadir (const char *args, int from_tty, struct cmd_list_element *c)
 {
   set_gdb_data_directory (staged_gdb_datadir);
+
+  /* SET_GDB_DATA_DIRECTORY will resolve relative paths in
+     STAGED_GDB_DATADIR, so we now copy the value from GDB_DATADIR
+     back into STAGED_GDB_DATADIR so the extension languages can read the
+     correct value.  */
+  free (staged_gdb_datadir);
+  staged_gdb_datadir = strdup (gdb_datadir.c_str ());
+
   gdb::observers::gdb_datadir_changed.notify ();
 }
 
@@ -2282,7 +2293,9 @@  Use \"on\" to enable the notification, and \"off\" to disable it."),
 When set, GDB uses the specified path to search for data files."),
 			   set_gdb_datadir, show_gdb_datadir,
 			   &setlist,
-			   &showlist);
+			    &showlist);
+  /* Prime the initial value for data-directory.  */
+  staged_gdb_datadir = strdup (gdb_datadir.c_str ());
 
   add_setshow_auto_boolean_cmd ("interactive-mode", class_support,
 				&interactive_mode, _("\