[PATCH] gdb: improve error reporting for 'save gdb-index'

[PATCH] gdb: improve error reporting for 'save gdb-index'

[Andrew Burgess]

While making recent changes to 'save gdb-index' command I triggered
some errors -- of the kind a user might be expected to trigger if they
do something wrong -- and I didn't find GDB's output as helpful as it
might be.

For example:

  $ gdb -q /tmp/hello.x
  ...
  (gdb) save gdb-index /non_existing_dir
  Error while writing index for `/tmp/hello': mkstemp: No such file or directory.

That the error message mentions '/tmp/hello', which does exit, but
doesn't mention '/non_existing_dir', which doesn't is, I think,
confusing.

Also, I find the 'mkstemp' in the error message confusing for a user
facing error.  A user might not know what mkstemp means, and even if
they do, that it appears in the error message is an internal GDB
detail.  The user doesn't care what function failed, why want to know
what was wrong with their input, and what they should do to fix
things.

Similarly, for a directory that does exist, but can't be written to:

  (gdb) save gdb-index /no_access_dir
  Error while writing index for `/tmp/hello': mkstemp: Permission denied.

In this case, the 'Permission denied' might make the user thing there
is a permissions issue with '/tmp/hello', which is not the case.

After this patch, the new errors are:

  (gdb) save gdb-index /non_existing_dir
  Error while writing index for `/tmp/hello': `/non_existing_dir': No such file or directory.

and:

  (gdb) save gdb-index /no_access_dir
  Error while writing index for `/tmp/hello': `/no_access_dir': Permission denied.

we also have:

  (gdb) save gdb-index /tmp/not_a_directory
  Error while writing index for `/tmp/hello': `/tmp/not_a_directory': Is not a directory.

I think these do a better job of guiding the user towards fixing the
problem.

I've added a new test that exercises all of these cases, and also
checks the case where a user tries to use an executable that already
contains an index in order to generate an index.

Nothing that worked correctly before this patch should give an error
after this patch; I've only changed the output when the user was going
to get an error anyway.
---
 gdb/dwarf2/index-write.c                 |  10 +-
 gdb/testsuite/gdb.base/gdb-index-err.c   |  22 +++++
 gdb/testsuite/gdb.base/gdb-index-err.exp | 118 +++++++++++++++++++++++
 3 files changed, 149 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.base/gdb-index-err.c
 create mode 100644 gdb/testsuite/gdb.base/gdb-index-err.exp

diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index b4a0117330e..4e5490e734e 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -1569,6 +1569,13 @@ struct index_wip_file
   index_wip_file (const char *dir, const char *basename,
 		  const char *suffix)
   {
+    /* Validate DIR is a  valid directory.  */
+    struct stat buf;
+    if (stat (dir, &buf) == -1)
+      perror_with_name (string_printf (_("`%s'"), dir).c_str ());
+    if ((buf.st_mode & S_IFDIR) != S_IFDIR)
+      error (_("`%s': Is not a directory."), dir);
+
     filename = (std::string (dir) + SLASH_STRING + basename
 		+ suffix);
 
@@ -1577,7 +1584,8 @@ struct index_wip_file
     scoped_fd out_file_fd = gdb_mkostemp_cloexec (filename_temp.data (),
 						  O_BINARY);
     if (out_file_fd.get () == -1)
-      perror_with_name (("mkstemp"));
+      perror_with_name (string_printf (_("couldn't open `%s'"),
+				       filename_temp.data ()).c_str ());
 
     out_file = out_file_fd.to_file ("wb");
 
diff --git a/gdb/testsuite/gdb.base/gdb-index-err.c b/gdb/testsuite/gdb.base/gdb-index-err.c
new file mode 100644
index 00000000000..3a264f239ed
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-index-err.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2023 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/>.  */
+
+int
+main ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/gdb-index-err.exp b/gdb/testsuite/gdb.base/gdb-index-err.exp
new file mode 100644
index 00000000000..9493c3aafd2
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-index-err.exp
@@ -0,0 +1,118 @@
+# Copyright 2023 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 some error messages that can arise from the 'save gdb-index'
+# command.
+
+standard_testfile
+
+if {[build_executable "build executable" $testfile $srcfile] == -1} {
+    return -1
+}
+
+clean_restart $binfile
+
+# This test isn't going to work when the board file automatically adds
+# an index section, or if the debug information is split into a
+# separate objfile.
+set has_cooked_index false
+gdb_test_multiple "maint print objfiles ${binfile}" "check debug style" -lbl {
+    -re "\r\n\\.gdb_index: version ${decimal}(?=\r\n)" {
+	gdb_test_lines "" $gdb_test_name ".*"
+    }
+    -re "\r\n\\.debug_names: exists(?=\r\n)" {
+	gdb_test_lines "" $gdb_test_name ".*"
+    }
+    -re "\r\n(Cooked index in use:|Psymtabs)(?=\r\n)" {
+	set has_cooked_index true
+	gdb_test_lines "" $gdb_test_name ".*"
+    }
+    -re ".gdb_index: faked for \"readnow\"" {
+	gdb_test_lines "" $gdb_test_name ".*"
+    }
+    -re -wrap "" {
+	# Have no debug information!
+    }
+}
+
+if { !$has_cooked_index } {
+    unsupported "cannot test without a cooked index"
+    return -1
+}
+
+
+# The name of a directory that doesn't exist.
+set bad_dir [standard_output_file "non-existent"]
+
+# Try to write the index into a non-existent directory.
+foreach_with_prefix flag { "" "-dwarf-5" } {
+    gdb_test "save gdb-index ${flag} ${bad_dir}" \
+	"Error while writing index for `[string_to_regexp $binfile]': `[string_to_regexp $bad_dir]': No such file or directory\\." \
+	"try to write index to non-existent directory"
+}
+
+# Create a text-file.
+set text_file [standard_output_file "text-file"]
+set fd [open $text_file w]
+puts $fd "A line of text.\n"
+close $fd
+
+# Try to write the index into something that is not a directory.
+foreach_with_prefix flag { "" "-dwarf-5" } {
+    gdb_test "save gdb-index ${flag} ${text_file}" \
+	"Error while writing index for `[string_to_regexp $binfile]': `[string_to_regexp $text_file]': Is not a directory\\." \
+	"try to write index to something that's not a directory"
+}
+
+# Create a directory which can't be written too.
+set non_writable_dir [standard_output_file "private"]
+remote_exec host "mkdir -p ${non_writable_dir}"
+remote_exec host "chmod u-w,g-w ${non_writable_dir}"
+
+# Try to write the index into a non-writable directory.
+foreach_with_prefix flag { "" "-dwarf-5" } {
+    gdb_test "save gdb-index ${flag} ${non_writable_dir}" \
+	"Error while writing index for `[string_to_regexp $binfile]': couldn't open `[string_to_regexp $non_writable_dir]/${gdb_test_file_name}.*': Permission denied\\." \
+	"try to write index to a non-writable directory"
+}
+
+# Create copies of the executable, we will add an index section to
+# each of these.
+remote_exec host "cp $binfile ${binfile}.gdb_index"
+remote_exec host "cp $binfile ${binfile}.dwarf_5"
+
+# Create a directory in which we can try to generate the index files.
+set already_indexed_dir [standard_output_file "already_indexed"]
+remote_exec host "mkdir -p $already_indexed_dir"
+
+foreach_with_prefix flag { "" "-dwarf-5" } {
+    if { $flag eq "" } {
+	set extension "gdb_index"
+    } else {
+	set extension "dwarf_5"
+    }
+
+    # Add the index section to the executable.
+    clean_restart ${binfile}.${extension}
+    gdb_assert {[ensure_gdb_index ${binfile}.${extension} ${flag}] == 1} \
+	"add index to executable"
+
+    # Reload the executable (which now has an index), and try to
+    # generate and index from it.  This will fail.
+    clean_restart ${binfile}.${extension}
+    gdb_test "save gdb-index ${flag} $already_indexed_dir" \
+	"Error while writing index for `[string_to_regexp $binfile.$extension]': Cannot use an index to create the index" \
+	"try to generate an index from a binary with an index"
+}

base-commit: 2c4caca9873ef4e97a51a538dfbecde8fe334082
-- 
2.25.4

Re: [PATCH] gdb: improve error reporting for 'save gdb-index'

[Tom de Vries]

On 12/4/23 20:29, Andrew Burgess wrote:
> While making recent changes to 'save gdb-index' command I triggered
> some errors -- of the kind a user might be expected to trigger if they
> do something wrong -- and I didn't find GDB's output as helpful as it
> might be.
> 
> For example:
> 
>    $ gdb -q /tmp/hello.x
>    ...
>    (gdb) save gdb-index /non_existing_dir
>    Error while writing index for `/tmp/hello': mkstemp: No such file or directory.
> 
> That the error message mentions '/tmp/hello', which does exit, but

exit -> exist

> doesn't mention '/non_existing_dir', which doesn't is, I think,
> confusing.
> 

Agreed.

> Also, I find the 'mkstemp' in the error message confusing for a user
> facing error.  A user might not know what mkstemp means, and even if
> they do, that it appears in the error message is an internal GDB
> detail.  The user doesn't care what function failed, why want to know

why want -> but wants ?

> what was wrong with their input, and what they should do to fix
> things.
> 
> Similarly, for a directory that does exist, but can't be written to:
> 
>    (gdb) save gdb-index /no_access_dir
>    Error while writing index for `/tmp/hello': mkstemp: Permission denied.
> 
> In this case, the 'Permission denied' might make the user thing there
> is a permissions issue with '/tmp/hello', which is not the case.
> 
> After this patch, the new errors are:
> 
>    (gdb) save gdb-index /non_existing_dir
>    Error while writing index for `/tmp/hello': `/non_existing_dir': No such file or directory.
> 
> and:
> 
>    (gdb) save gdb-index /no_access_dir
>    Error while writing index for `/tmp/hello': `/no_access_dir': Permission denied.
> 
> we also have:
> 
>    (gdb) save gdb-index /tmp/not_a_directory
>    Error while writing index for `/tmp/hello': `/tmp/not_a_directory': Is not a directory.
> 
> I think these do a better job of guiding the user towards fixing the
> problem.
> 

Agreed, I think this is a nice improvement.

> I've added a new test that exercises all of these cases, and also
> checks the case where a user tries to use an executable that already
> contains an index in order to generate an index.
> 
> Nothing that worked correctly before this patch should give an error
> after this patch; I've only changed the output when the user was going
> to get an error anyway.
> ---
>   gdb/dwarf2/index-write.c                 |  10 +-
>   gdb/testsuite/gdb.base/gdb-index-err.c   |  22 +++++
>   gdb/testsuite/gdb.base/gdb-index-err.exp | 118 +++++++++++++++++++++++
>   3 files changed, 149 insertions(+), 1 deletion(-)
>   create mode 100644 gdb/testsuite/gdb.base/gdb-index-err.c
>   create mode 100644 gdb/testsuite/gdb.base/gdb-index-err.exp
> 
> diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
> index b4a0117330e..4e5490e734e 100644
> --- a/gdb/dwarf2/index-write.c
> +++ b/gdb/dwarf2/index-write.c
> @@ -1569,6 +1569,13 @@ struct index_wip_file
>     index_wip_file (const char *dir, const char *basename,
>   		  const char *suffix)
>     {
> +    /* Validate DIR is a  valid directory.  */

double space.

> +    struct stat buf;
> +    if (stat (dir, &buf) == -1)
> +      perror_with_name (string_printf (_("`%s'"), dir).c_str ());
> +    if ((buf.st_mode & S_IFDIR) != S_IFDIR)
> +      error (_("`%s': Is not a directory."), dir);
> +
>       filename = (std::string (dir) + SLASH_STRING + basename
>   		+ suffix);
>   
> @@ -1577,7 +1584,8 @@ struct index_wip_file
>       scoped_fd out_file_fd = gdb_mkostemp_cloexec (filename_temp.data (),
>   						  O_BINARY);
>       if (out_file_fd.get () == -1)
> -      perror_with_name (("mkstemp"));
> +      perror_with_name (string_printf (_("couldn't open `%s'"),
> +				       filename_temp.data ()).c_str ());
>   
>       out_file = out_file_fd.to_file ("wb");
>   
> diff --git a/gdb/testsuite/gdb.base/gdb-index-err.c b/gdb/testsuite/gdb.base/gdb-index-err.c
> new file mode 100644
> index 00000000000..3a264f239ed
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gdb-index-err.c
> @@ -0,0 +1,22 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2023 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/>.  */
> +
> +int
> +main ()
> +{
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/gdb-index-err.exp b/gdb/testsuite/gdb.base/gdb-index-err.exp
> new file mode 100644
> index 00000000000..9493c3aafd2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gdb-index-err.exp
> @@ -0,0 +1,118 @@
> +# Copyright 2023 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 some error messages that can arise from the 'save gdb-index'
> +# command.
> +
> +standard_testfile
> +
> +if {[build_executable "build executable" $testfile $srcfile] == -1} {
> +    return -1
> +}
> +
> +clean_restart $binfile
> +

I think the usual shorthand for this is prepare_for_testing.

> +# This test isn't going to work when the board file automatically adds
> +# an index section, or if the debug information is split into a
> +# separate objfile.
> +set has_cooked_index false
> +gdb_test_multiple "maint print objfiles ${binfile}" "check debug style" -lbl {
> +    -re "\r\n\\.gdb_index: version ${decimal}(?=\r\n)" {
> +	gdb_test_lines "" $gdb_test_name ".*"
> +    }
> +    -re "\r\n\\.debug_names: exists(?=\r\n)" {
> +	gdb_test_lines "" $gdb_test_name ".*"
> +    }
> +    -re "\r\n(Cooked index in use:|Psymtabs)(?=\r\n)" {
> +	set has_cooked_index true
> +	gdb_test_lines "" $gdb_test_name ".*"
> +    }
> +    -re ".gdb_index: faked for \"readnow\"" {
> +	gdb_test_lines "" $gdb_test_name ".*"
> +    }
> +    -re -wrap "" {
> +	# Have no debug information!
> +    }
> +}
> +

I think ideally we do this kind of checks using a factored out function 
in gdb.exp.  There may already be one, I haven't checked.  If not, maybe 
consider moving it there.

Anyway, LGTM.

Reviewed-By: Tom de Vries <tdevries@suse.de>

Thanks,
- Tom

> +if { !$has_cooked_index } {
> +    unsupported "cannot test without a cooked index"
> +    return -1
> +}
> +
> +
> +# The name of a directory that doesn't exist.
> +set bad_dir [standard_output_file "non-existent"]
> +
> +# Try to write the index into a non-existent directory.
> +foreach_with_prefix flag { "" "-dwarf-5" } {
> +    gdb_test "save gdb-index ${flag} ${bad_dir}" \
> +	"Error while writing index for `[string_to_regexp $binfile]': `[string_to_regexp $bad_dir]': No such file or directory\\." \
> +	"try to write index to non-existent directory"
> +}
> +
> +# Create a text-file.
> +set text_file [standard_output_file "text-file"]
> +set fd [open $text_file w]
> +puts $fd "A line of text.\n"
> +close $fd
> +
> +# Try to write the index into something that is not a directory.
> +foreach_with_prefix flag { "" "-dwarf-5" } {
> +    gdb_test "save gdb-index ${flag} ${text_file}" \
> +	"Error while writing index for `[string_to_regexp $binfile]': `[string_to_regexp $text_file]': Is not a directory\\." \
> +	"try to write index to something that's not a directory"
> +}
> +
> +# Create a directory which can't be written too.
> +set non_writable_dir [standard_output_file "private"]
> +remote_exec host "mkdir -p ${non_writable_dir}"
> +remote_exec host "chmod u-w,g-w ${non_writable_dir}"
> +
> +# Try to write the index into a non-writable directory.
> +foreach_with_prefix flag { "" "-dwarf-5" } {
> +    gdb_test "save gdb-index ${flag} ${non_writable_dir}" \
> +	"Error while writing index for `[string_to_regexp $binfile]': couldn't open `[string_to_regexp $non_writable_dir]/${gdb_test_file_name}.*': Permission denied\\." \
> +	"try to write index to a non-writable directory"
> +}
> +
> +# Create copies of the executable, we will add an index section to
> +# each of these.
> +remote_exec host "cp $binfile ${binfile}.gdb_index"
> +remote_exec host "cp $binfile ${binfile}.dwarf_5"
> +
> +# Create a directory in which we can try to generate the index files.
> +set already_indexed_dir [standard_output_file "already_indexed"]
> +remote_exec host "mkdir -p $already_indexed_dir"
> +
> +foreach_with_prefix flag { "" "-dwarf-5" } {
> +    if { $flag eq "" } {
> +	set extension "gdb_index"
> +    } else {
> +	set extension "dwarf_5"
> +    }
> +
> +    # Add the index section to the executable.
> +    clean_restart ${binfile}.${extension}
> +    gdb_assert {[ensure_gdb_index ${binfile}.${extension} ${flag}] == 1} \
> +	"add index to executable"
> +
> +    # Reload the executable (which now has an index), and try to
> +    # generate and index from it.  This will fail.
> +    clean_restart ${binfile}.${extension}
> +    gdb_test "save gdb-index ${flag} $already_indexed_dir" \
> +	"Error while writing index for `[string_to_regexp $binfile.$extension]': Cannot use an index to create the index" \
> +	"try to generate an index from a binary with an index"
> +}
> 
> base-commit: 2c4caca9873ef4e97a51a538dfbecde8fe334082

[PATCHv2] gdb: improve error reporting for 'save gdb-index'

[Andrew Burgess]

Changes in V2:

  - Typos that Tom pointed out all fixed,

  - Now using prepare_for_testing in the new test,

  - Split up an existing function in lib/gdb.exp, which I can now use
    from my new test, this cleans up how I check the index type in the
    new test.

Thanks,
Andrew


---

While making recent changes to 'save gdb-index' command I triggered
some errors -- of the kind a user might be expected to trigger if they
do something wrong -- and I didn't find GDB's output as helpful as it
might be.

For example:

  $ gdb -q /tmp/hello.x
  ...
  (gdb) save gdb-index /non_existing_dir
  Error while writing index for `/tmp/hello': mkstemp: No such file or directory.

That the error message mentions '/tmp/hello', which does exist, but
doesn't mention '/non_existing_dir', which doesn't is, I think,
confusing.

Also, I find the 'mkstemp' in the error message confusing for a user
facing error.  A user might not know what mkstemp means, and even if
they do, that it appears in the error message is an internal GDB
detail.  The user doesn't care what function failed, but wants to know
what was wrong with their input, and what they should do to fix
things.

Similarly, for a directory that does exist, but can't be written to:

  (gdb) save gdb-index /no_access_dir
  Error while writing index for `/tmp/hello': mkstemp: Permission denied.

In this case, the 'Permission denied' might make the user thing there
is a permissions issue with '/tmp/hello', which is not the case.

After this patch, the new errors are:

  (gdb) save gdb-index /non_existing_dir
  Error while writing index for `/tmp/hello': `/non_existing_dir': No such file or directory.

and:

  (gdb) save gdb-index /no_access_dir
  Error while writing index for `/tmp/hello': `/no_access_dir': Permission denied.

we also have:

  (gdb) save gdb-index /tmp/not_a_directory
  Error while writing index for `/tmp/hello': `/tmp/not_a_directory': Is not a directory.

I think these do a better job of guiding the user towards fixing the
problem.

I've added a new test that exercises all of these cases, and also
checks the case where a user tries to use an executable that already
contains an index in order to generate an index.  As part of the new
test I've factored out some code from ensure_gdb_index (lib/gdb.exp)
into a new proc, which I've then used in the new test.  I've confirmed
that all the tests that use ensure_gdb_index still pass.

Nothing that worked correctly before this patch should give an error
after this patch; I've only changed the output when the user was going
to get an error anyway.

Reviewed-By: Tom de Vries <tdevries@suse.de>
---
 gdb/dwarf2/index-write.c                 | 10 ++-
 gdb/testsuite/gdb.base/gdb-index-err.c   | 22 ++++++
 gdb/testsuite/gdb.base/gdb-index-err.exp | 97 ++++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                | 78 ++++++++++++++-----
 4 files changed, 185 insertions(+), 22 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/gdb-index-err.c
 create mode 100644 gdb/testsuite/gdb.base/gdb-index-err.exp

diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index b4a0117330e..842708a266e 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -1569,6 +1569,13 @@ struct index_wip_file
   index_wip_file (const char *dir, const char *basename,
 		  const char *suffix)
   {
+    /* Validate DIR is a valid directory.  */
+    struct stat buf;
+    if (stat (dir, &buf) == -1)
+      perror_with_name (string_printf (_("`%s'"), dir).c_str ());
+    if ((buf.st_mode & S_IFDIR) != S_IFDIR)
+      error (_("`%s': Is not a directory."), dir);
+
     filename = (std::string (dir) + SLASH_STRING + basename
 		+ suffix);
 
@@ -1577,7 +1584,8 @@ struct index_wip_file
     scoped_fd out_file_fd = gdb_mkostemp_cloexec (filename_temp.data (),
 						  O_BINARY);
     if (out_file_fd.get () == -1)
-      perror_with_name (("mkstemp"));
+      perror_with_name (string_printf (_("couldn't open `%s'"),
+				       filename_temp.data ()).c_str ());
 
     out_file = out_file_fd.to_file ("wb");
 
diff --git a/gdb/testsuite/gdb.base/gdb-index-err.c b/gdb/testsuite/gdb.base/gdb-index-err.c
new file mode 100644
index 00000000000..3a264f239ed
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-index-err.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2023 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/>.  */
+
+int
+main ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/gdb-index-err.exp b/gdb/testsuite/gdb.base/gdb-index-err.exp
new file mode 100644
index 00000000000..31f133efbcd
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-index-err.exp
@@ -0,0 +1,97 @@
+# Copyright 2023 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 some error messages that can arise from the 'save gdb-index'
+# command.
+
+standard_testfile
+
+if {[prepare_for_testing "prepare for test" $testfile $srcfile] == -1} {
+    return -1
+}
+
+# This test isn't going to work when the board file automatically adds
+# an index section, or if the debug information is split into a
+# separate objfile.
+set index_type [get_index_type $binfile "check debug style"]
+if { $index_type ne "cooked" } {
+    unsupported "cannot test without a cooked index"
+    return -1
+}
+
+
+# The name of a directory that doesn't exist.
+set bad_dir [standard_output_file "non-existent"]
+
+# Try to write the index into a non-existent directory.
+foreach_with_prefix flag { "" "-dwarf-5" } {
+    gdb_test "save gdb-index ${flag} ${bad_dir}" \
+	"Error while writing index for `[string_to_regexp $binfile]': `[string_to_regexp $bad_dir]': No such file or directory\\." \
+	"try to write index to non-existent directory"
+}
+
+# Create a text-file.
+set text_file [standard_output_file "text-file"]
+set fd [open $text_file w]
+puts $fd "A line of text.\n"
+close $fd
+
+# Try to write the index into something that is not a directory.
+foreach_with_prefix flag { "" "-dwarf-5" } {
+    gdb_test "save gdb-index ${flag} ${text_file}" \
+	"Error while writing index for `[string_to_regexp $binfile]': `[string_to_regexp $text_file]': Is not a directory\\." \
+	"try to write index to something that's not a directory"
+}
+
+# Create a directory which can't be written too.
+set non_writable_dir [standard_output_file "private"]
+remote_exec host "mkdir -p ${non_writable_dir}"
+remote_exec host "chmod u-w,g-w ${non_writable_dir}"
+
+# Try to write the index into a non-writable directory.
+foreach_with_prefix flag { "" "-dwarf-5" } {
+    gdb_test "save gdb-index ${flag} ${non_writable_dir}" \
+	"Error while writing index for `[string_to_regexp $binfile]': couldn't open `[string_to_regexp $non_writable_dir]/${gdb_test_file_name}.*': Permission denied\\." \
+	"try to write index to a non-writable directory"
+}
+
+# Create copies of the executable, we will add an index section to
+# each of these.
+remote_exec host "cp $binfile ${binfile}.gdb_index"
+remote_exec host "cp $binfile ${binfile}.dwarf_5"
+
+# Create a directory in which we can try to generate the index files.
+set already_indexed_dir [standard_output_file "already_indexed"]
+remote_exec host "mkdir -p $already_indexed_dir"
+
+foreach_with_prefix flag { "" "-dwarf-5" } {
+    if { $flag eq "" } {
+	set extension "gdb_index"
+    } else {
+	set extension "dwarf_5"
+    }
+
+    # Add the index section to the executable.
+    clean_restart ${binfile}.${extension}
+    gdb_assert {[ensure_gdb_index ${binfile}.${extension} ${flag}] == 1} \
+	"add index to executable"
+
+    # Reload the executable (which now has an index), and try to
+    # generate and index from it.  This will fail.
+    clean_restart ${binfile}.${extension}
+    gdb_test "save gdb-index ${flag} $already_indexed_dir" \
+	"Error while writing index for `[string_to_regexp $binfile.$extension]': Cannot use an index to create the index" \
+	"try to generate an index from a binary with an index"
+}
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 7e478ab9b60..f765214cbe6 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -9323,47 +9323,83 @@ proc add_gdb_index { program {style ""} } {
     return 1
 }
 
-# Add a .gdb_index section to PROGRAM, unless it alread has an index
-# (.gdb_index/.debug_names).  Gdb doesn't support building an index from a
-# program already using one.  Return 1 if a .gdb_index was added, return 0
-# if it already contained an index, and -1 if an error occurred.
+# Use 'maint print objfiles OBJFILE' to determine what (if any) type
+# of index is present in OBJFILE.  Return a string indicating the
+# index type:
 #
-# STYLE controls which style of index to add, if needed.  The empty
-# string (the default) means .gdb_index; "-dwarf-5" means .debug_names.
-
-proc ensure_gdb_index { binfile {style ""} } {
-    global decimal
+# 'gdb' - Contains a .gdb_index style index,
+#
+# 'dwarf5' - Contain DWARF5 style index sections,
+#
+# 'readnow' - A fake .gdb_index as a result of readnow being used,
+#
+# 'cooked' - The cooked index created when reading non-indexed debug
+#            information,
+#
+# 'none' - There's no index, and no debug information to create a
+#          cooked index from.
+#
+# If something goes wrong then this proc will emit a FAIL and return
+# an empty string.
+#
+# TESTNAME is used as part of any pass/fail emitted from this proc.
+proc get_index_type { objfile { testname "" } } {
+    if { $testname eq "" } {
+	set testname "find index type"
+    }
 
-    set testfile [file tail $binfile]
-    set test "check if index present"
-    set has_index 0
-    set has_readnow 0
-    gdb_test_multiple "mt print objfiles ${testfile}" $test -lbl {
-	-re "\r\n\\.gdb_index: version ${decimal}(?=\r\n)" {
-	    set has_index 1
+    set index_type "unknown"
+    gdb_test_multiple "maint print objfiles ${objfile}" $testname -lbl {
+	-re "\r\n\\.gdb_index: version ${::decimal}(?=\r\n)" {
+	    set index_type "gdb"
 	    gdb_test_lines "" $gdb_test_name ".*"
 	}
 	-re "\r\n\\.debug_names: exists(?=\r\n)" {
-	    set has_index 1
+	    set index_type "dwarf5"
 	    gdb_test_lines "" $gdb_test_name ".*"
 	}
 	-re "\r\n(Cooked index in use:|Psymtabs)(?=\r\n)" {
+	    set index_type "cooked"
 	    gdb_test_lines "" $gdb_test_name ".*"
 	}
 	-re ".gdb_index: faked for \"readnow\"" {
-	    set has_readnow 1
+	    set index_type "readnow"
 	    gdb_test_lines "" $gdb_test_name ".*"
 	}
 	-re -wrap "" {
-	    fail $gdb_test_name
+	    set index_type "none"
 	}
     }
 
-    if { $has_index } {
+    gdb_assert { $index_type ne "unknown" } \
+	"$testname, check type is valid"
+
+    if { $index_type eq "unknown" } {
+	set index_type ""
+    }
+
+    return $index_type
+}
+
+# Add a .gdb_index section to PROGRAM, unless it alread has an index
+# (.gdb_index/.debug_names).  Gdb doesn't support building an index from a
+# program already using one.  Return 1 if a .gdb_index was added, return 0
+# if it already contained an index, and -1 if an error occurred.
+#
+# STYLE controls which style of index to add, if needed.  The empty
+# string (the default) means .gdb_index; "-dwarf-5" means .debug_names.
+
+proc ensure_gdb_index { binfile {style ""} } {
+    set testfile [file tail $binfile]
+
+    set test "check if index present"
+    set index_type [get_index_type $testfile $test]
+
+    if { $index_type eq "gdb" || $index_type eq "dwarf5" } {
 	return 0
     }
 
-    if { $has_readnow } {
+    if { $index_type eq "readnow" } {
 	return -1
     }
 

base-commit: 5a22e042e41db962cd6a79cd59cab46cbbe58a98
-- 
2.25.4

Re: [PATCHv2] gdb: improve error reporting for 'save gdb-index'

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> --- a/gdb/testsuite/lib/gdb.exp
Andrew> +++ b/gdb/testsuite/lib/gdb.exp

Andrew> +proc get_index_type { objfile { testname "" } } {
Andrew> +    if { $testname eq "" } {
Andrew> +	set testname "find index type"
Andrew> +    }

...

This seems to overlap with proc have_index.
I wonder if they can be combined.

Tom

[PATCHv3] gdb: improve error reporting for 'save gdb-index'

[Andrew Burgess]

Changes in V3:

  - Have updated the existing have_index testsuite proc to call the
    new get_index_type proc to avoid duplication that Tom (Tromey)
    pointed out.

Changes in V2:

  - Typos that Tom (de Vries) pointed out all fixed,

  - Now using prepare_for_testing in the new test,

  - Split up an existing function in lib/gdb.exp, which I can now use
    from my new test, this cleans up how I check the index type in the
    new test.

Thanks,
Andrew

---

While making recent changes to 'save gdb-index' command I triggered
some errors -- of the kind a user might be expected to trigger if they
do something wrong -- and I didn't find GDB's output as helpful as it
might be.

For example:

  $ gdb -q /tmp/hello.x
  ...
  (gdb) save gdb-index /non_existing_dir
  Error while writing index for `/tmp/hello': mkstemp: No such file or directory.

That the error message mentions '/tmp/hello', which does exist, but
doesn't mention '/non_existing_dir', which doesn't is, I think,
confusing.

Also, I find the 'mkstemp' in the error message confusing for a user
facing error.  A user might not know what mkstemp means, and even if
they do, that it appears in the error message is an internal GDB
detail.  The user doesn't care what function failed, but wants to know
what was wrong with their input, and what they should do to fix
things.

Similarly, for a directory that does exist, but can't be written to:

  (gdb) save gdb-index /no_access_dir
  Error while writing index for `/tmp/hello': mkstemp: Permission denied.

In this case, the 'Permission denied' might make the user thing there
is a permissions issue with '/tmp/hello', which is not the case.

After this patch, the new errors are:

  (gdb) save gdb-index /non_existing_dir
  Error while writing index for `/tmp/hello': `/non_existing_dir': No such file or directory.

and:

  (gdb) save gdb-index /no_access_dir
  Error while writing index for `/tmp/hello': `/no_access_dir': Permission denied.

we also have:

  (gdb) save gdb-index /tmp/not_a_directory
  Error while writing index for `/tmp/hello': `/tmp/not_a_directory': Is not a directory.

I think these do a better job of guiding the user towards fixing the
problem.

I've added a new test that exercises all of these cases, and also
checks the case where a user tries to use an executable that already
contains an index in order to generate an index.  As part of the new
test I've factored out some code from ensure_gdb_index (lib/gdb.exp)
into a new proc (get_index_type), which I've then used in the new
test.  I've confirmed that all the tests that use ensure_gdb_index
still pass.

During review it was pointed out that the testsuite proc
have_index (lib/gdb.exp) is similar to the new get_index_type proc, so
I've rewritten have_index to also use get_index_type, I've confirmed
that all the tests that use have_index still pass.

Nothing that worked correctly before this patch should give an error
after this patch; I've only changed the output when the user was going
to get an error anyway.

Reviewed-By: Tom de Vries <tdevries@suse.de>
Reviewed-By: Tom Tromey <tom@tromey.com>
---
 gdb/dwarf2/index-write.c                 |  10 +-
 gdb/testsuite/gdb.base/gdb-index-err.c   |  22 +++++
 gdb/testsuite/gdb.base/gdb-index-err.exp |  97 +++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                | 113 ++++++++++++++---------
 4 files changed, 198 insertions(+), 44 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/gdb-index-err.c
 create mode 100644 gdb/testsuite/gdb.base/gdb-index-err.exp

diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index b4a0117330e..842708a266e 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -1569,6 +1569,13 @@ struct index_wip_file
   index_wip_file (const char *dir, const char *basename,
 		  const char *suffix)
   {
+    /* Validate DIR is a valid directory.  */
+    struct stat buf;
+    if (stat (dir, &buf) == -1)
+      perror_with_name (string_printf (_("`%s'"), dir).c_str ());
+    if ((buf.st_mode & S_IFDIR) != S_IFDIR)
+      error (_("`%s': Is not a directory."), dir);
+
     filename = (std::string (dir) + SLASH_STRING + basename
 		+ suffix);
 
@@ -1577,7 +1584,8 @@ struct index_wip_file
     scoped_fd out_file_fd = gdb_mkostemp_cloexec (filename_temp.data (),
 						  O_BINARY);
     if (out_file_fd.get () == -1)
-      perror_with_name (("mkstemp"));
+      perror_with_name (string_printf (_("couldn't open `%s'"),
+				       filename_temp.data ()).c_str ());
 
     out_file = out_file_fd.to_file ("wb");
 
diff --git a/gdb/testsuite/gdb.base/gdb-index-err.c b/gdb/testsuite/gdb.base/gdb-index-err.c
new file mode 100644
index 00000000000..3a264f239ed
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-index-err.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2023 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/>.  */
+
+int
+main ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/gdb-index-err.exp b/gdb/testsuite/gdb.base/gdb-index-err.exp
new file mode 100644
index 00000000000..31f133efbcd
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-index-err.exp
@@ -0,0 +1,97 @@
+# Copyright 2023 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 some error messages that can arise from the 'save gdb-index'
+# command.
+
+standard_testfile
+
+if {[prepare_for_testing "prepare for test" $testfile $srcfile] == -1} {
+    return -1
+}
+
+# This test isn't going to work when the board file automatically adds
+# an index section, or if the debug information is split into a
+# separate objfile.
+set index_type [get_index_type $binfile "check debug style"]
+if { $index_type ne "cooked" } {
+    unsupported "cannot test without a cooked index"
+    return -1
+}
+
+
+# The name of a directory that doesn't exist.
+set bad_dir [standard_output_file "non-existent"]
+
+# Try to write the index into a non-existent directory.
+foreach_with_prefix flag { "" "-dwarf-5" } {
+    gdb_test "save gdb-index ${flag} ${bad_dir}" \
+	"Error while writing index for `[string_to_regexp $binfile]': `[string_to_regexp $bad_dir]': No such file or directory\\." \
+	"try to write index to non-existent directory"
+}
+
+# Create a text-file.
+set text_file [standard_output_file "text-file"]
+set fd [open $text_file w]
+puts $fd "A line of text.\n"
+close $fd
+
+# Try to write the index into something that is not a directory.
+foreach_with_prefix flag { "" "-dwarf-5" } {
+    gdb_test "save gdb-index ${flag} ${text_file}" \
+	"Error while writing index for `[string_to_regexp $binfile]': `[string_to_regexp $text_file]': Is not a directory\\." \
+	"try to write index to something that's not a directory"
+}
+
+# Create a directory which can't be written too.
+set non_writable_dir [standard_output_file "private"]
+remote_exec host "mkdir -p ${non_writable_dir}"
+remote_exec host "chmod u-w,g-w ${non_writable_dir}"
+
+# Try to write the index into a non-writable directory.
+foreach_with_prefix flag { "" "-dwarf-5" } {
+    gdb_test "save gdb-index ${flag} ${non_writable_dir}" \
+	"Error while writing index for `[string_to_regexp $binfile]': couldn't open `[string_to_regexp $non_writable_dir]/${gdb_test_file_name}.*': Permission denied\\." \
+	"try to write index to a non-writable directory"
+}
+
+# Create copies of the executable, we will add an index section to
+# each of these.
+remote_exec host "cp $binfile ${binfile}.gdb_index"
+remote_exec host "cp $binfile ${binfile}.dwarf_5"
+
+# Create a directory in which we can try to generate the index files.
+set already_indexed_dir [standard_output_file "already_indexed"]
+remote_exec host "mkdir -p $already_indexed_dir"
+
+foreach_with_prefix flag { "" "-dwarf-5" } {
+    if { $flag eq "" } {
+	set extension "gdb_index"
+    } else {
+	set extension "dwarf_5"
+    }
+
+    # Add the index section to the executable.
+    clean_restart ${binfile}.${extension}
+    gdb_assert {[ensure_gdb_index ${binfile}.${extension} ${flag}] == 1} \
+	"add index to executable"
+
+    # Reload the executable (which now has an index), and try to
+    # generate and index from it.  This will fail.
+    clean_restart ${binfile}.${extension}
+    gdb_test "save gdb-index ${flag} $already_indexed_dir" \
+	"Error while writing index for `[string_to_regexp $binfile.$extension]': Cannot use an index to create the index" \
+	"try to generate an index from a binary with an index"
+}
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 7e478ab9b60..eb8f6998b1e 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -9230,36 +9230,27 @@ proc readnow { } {
 		  || [lsearch -exact $::GDBFLAGS --readnow] != -1}]
 }
 
-# Return index name if symbols were read in using an index.
-# Otherwise, return "".
+# Return 'gdb_index' if the symbols from OBJFILE were read using a
+# .gdb_index index.  Return 'debug_names' if the symbols were read
+# using a DWARF-5 style .debug_names index.  Otherwise, return an
+# empty string.
 
 proc have_index { objfile } {
+
     # This proc is mostly used with $binfile, but that gives problems with
     # remote host, while using $testfile would work.
     # Fix this by reducing $binfile to $testfile.
     set objfile [file tail $objfile]
 
-    set res ""
-    set cmd "maint print objfiles $objfile"
-    gdb_test_multiple $cmd "" -lbl {
-	-re "\r\n.gdb_index: faked for \"readnow\"" {
-	    set res ""
-	    exp_continue
-	}
-	-re "\r\n.gdb_index:" {
-	    set res "gdb_index"
-	    exp_continue
-	}
-	-re "\r\n.debug_names:" {
-	    set res "debug_names"
-	    exp_continue
-	}
-	-re -wrap "" {
-	    # We don't care about any other input.
-	}
-    }
+    set index_type [get_index_type $objfile]
 
-    return $res
+    if { $index_type eq "gdb" } {
+	return "gdb_index"
+    } elseif { $index_type eq "dwarf5" } {
+	return "debug_names"
+    } else {
+	return ""
+    }
 }
 
 # Return 1 if partial symbols are available.  Otherwise, return 0.
@@ -9323,47 +9314,83 @@ proc add_gdb_index { program {style ""} } {
     return 1
 }
 
-# Add a .gdb_index section to PROGRAM, unless it alread has an index
-# (.gdb_index/.debug_names).  Gdb doesn't support building an index from a
-# program already using one.  Return 1 if a .gdb_index was added, return 0
-# if it already contained an index, and -1 if an error occurred.
+# Use 'maint print objfiles OBJFILE' to determine what (if any) type
+# of index is present in OBJFILE.  Return a string indicating the
+# index type:
 #
-# STYLE controls which style of index to add, if needed.  The empty
-# string (the default) means .gdb_index; "-dwarf-5" means .debug_names.
-
-proc ensure_gdb_index { binfile {style ""} } {
-    global decimal
+# 'gdb' - Contains a .gdb_index style index,
+#
+# 'dwarf5' - Contain DWARF5 style index sections,
+#
+# 'readnow' - A fake .gdb_index as a result of readnow being used,
+#
+# 'cooked' - The cooked index created when reading non-indexed debug
+#            information,
+#
+# 'none' - There's no index, and no debug information to create a
+#          cooked index from.
+#
+# If something goes wrong then this proc will emit a FAIL and return
+# an empty string.
+#
+# TESTNAME is used as part of any pass/fail emitted from this proc.
+proc get_index_type { objfile { testname "" } } {
+    if { $testname eq "" } {
+	set testname "find index type"
+    }
 
-    set testfile [file tail $binfile]
-    set test "check if index present"
-    set has_index 0
-    set has_readnow 0
-    gdb_test_multiple "mt print objfiles ${testfile}" $test -lbl {
-	-re "\r\n\\.gdb_index: version ${decimal}(?=\r\n)" {
-	    set has_index 1
+    set index_type "unknown"
+    gdb_test_multiple "maint print objfiles ${objfile}" $testname -lbl {
+	-re "\r\n\\.gdb_index: version ${::decimal}(?=\r\n)" {
+	    set index_type "gdb"
 	    gdb_test_lines "" $gdb_test_name ".*"
 	}
 	-re "\r\n\\.debug_names: exists(?=\r\n)" {
-	    set has_index 1
+	    set index_type "dwarf5"
 	    gdb_test_lines "" $gdb_test_name ".*"
 	}
 	-re "\r\n(Cooked index in use:|Psymtabs)(?=\r\n)" {
+	    set index_type "cooked"
 	    gdb_test_lines "" $gdb_test_name ".*"
 	}
 	-re ".gdb_index: faked for \"readnow\"" {
-	    set has_readnow 1
+	    set index_type "readnow"
 	    gdb_test_lines "" $gdb_test_name ".*"
 	}
 	-re -wrap "" {
-	    fail $gdb_test_name
+	    set index_type "none"
 	}
     }
 
-    if { $has_index } {
+    gdb_assert { $index_type ne "unknown" } \
+	"$testname, check type is valid"
+
+    if { $index_type eq "unknown" } {
+	set index_type ""
+    }
+
+    return $index_type
+}
+
+# Add a .gdb_index section to PROGRAM, unless it alread has an index
+# (.gdb_index/.debug_names).  Gdb doesn't support building an index from a
+# program already using one.  Return 1 if a .gdb_index was added, return 0
+# if it already contained an index, and -1 if an error occurred.
+#
+# STYLE controls which style of index to add, if needed.  The empty
+# string (the default) means .gdb_index; "-dwarf-5" means .debug_names.
+
+proc ensure_gdb_index { binfile {style ""} } {
+    set testfile [file tail $binfile]
+
+    set test "check if index present"
+    set index_type [get_index_type $testfile $test]
+
+    if { $index_type eq "gdb" || $index_type eq "dwarf5" } {
 	return 0
     }
 
-    if { $has_readnow } {
+    if { $index_type eq "readnow" } {
 	return -1
     }
 

base-commit: cff71358132db440b82747707b3c7c99efca6670
-- 
2.25.4

Re: [PATCHv3] gdb: improve error reporting for 'save gdb-index'

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> Changes in V3:
Andrew>   - Have updated the existing have_index testsuite proc to call the
Andrew>     new get_index_type proc to avoid duplication that Tom (Tromey)
Andrew>     pointed out.

Thank you.  FWIW this looks good to me now.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCHv3] gdb: improve error reporting for 'save gdb-index'

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> Changes in V3:
> Andrew>   - Have updated the existing have_index testsuite proc to call the
> Andrew>     new get_index_type proc to avoid duplication that Tom (Tromey)
> Andrew>     pointed out.
>
> Thank you.  FWIW this looks good to me now.
> Approved-By: Tom Tromey <tom@tromey.com>

Pushed.

Thanks,
Andrew