Re: [PATCH] gdb/source: Fix open_source_file error handling

[Keith Seitz <keiths@redhat.com>]

On 1/27/23 19:37, Aaron Merey via Gdb-patches wrote:
> 
> Fix this by having open_source_file return a negative errno if it fails
> to open a source file.  Use this value to generate the error message
> instead of errno. Also add new function throw_sys_errmsg for reporting
> this value.

Thanks for the patch. This LGTM [warning: I am not a maintainer], but I just
have two observations (nits?).

> diff --git a/gdb/source.c b/gdb/source.c
> index be5af180c49..158b59fd613 100644
> --- a/gdb/source.c
> +++ b/gdb/source.c
> @@ -1200,10 +1204,21 @@ open_source_file (struct symtab *s)
>   
>   	  /* Query debuginfod for the source file.  */
>   	  if (build_id != nullptr && !srcpath.empty ())
> -	    fd = debuginfod_source_query (build_id->data,
> -					  build_id->size,
> -					  srcpath.c_str (),
> -					  &fullname);
> +	    {
> +	      scoped_fd query_fd
> +		= debuginfod_source_query (build_id->data,
> +					   build_id->size,
> +					   srcpath.c_str (),
> +					   &fullname);
> +
> +	      /* Don't return a negative errno from debuginfod_source_query.
> +		 It handles the reporting of its own errors.  */

I would prefer to see a comment on this behavior with debuginfod_source_query's
description. [Repeating here is fine IMO.]

> +	      if (query_fd.get () >= 0)
> +		{
> +		  s->fullname = fullname.release ();
> +		  return query_fd;
> +		}
> +	    }
>   	}
>       }
>   
> diff --git a/gdb/utils.c b/gdb/utils.c
> index 95adbe58e4a..0a5c6a70531 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -670,6 +670,21 @@ print_sys_errmsg (const char *string, int errcode)
>     gdb_printf (gdb_stderr, "%s: %s.\n", string, err);
>   }
>   
> +/* Same as perror_with_name except that ERRCODE specifies the error
> +   instead of errno.  */
> +

Should this comment be moved to the decl in utils.h? In such
cases, we normally use "See utils.h." as a comment at the
definition.

> +void
> +throw_sys_errmsg (const char *string, int errcode)
> +{
> +  const char *err = safe_strerror (errcode);
> +  std::string combined = std::string (string) + ": " + err;
> +
> +  bfd_set_error (bfd_error_no_error);
> +  errno = 0;
> +
> +  throw_error (GENERIC_ERROR, _("%s."), combined.c_str ());
> +}
> +
>   /* Control C eventually causes this to be called, at a convenient time.  */
>   
>   void
> diff --git a/gdb/utils.h b/gdb/utils.h
> index 7865812998e..54a1bc4e476 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -278,6 +278,8 @@ extern void fprintf_symbol (struct ui_file *, const char *,
>   extern void perror_warning_with_name (const char *string);
>   
>   extern void print_sys_errmsg (const char *, int);
> +
> +extern void throw_sys_errmsg (const char *, int) ATTRIBUTE_NORETURN;
>   \f
>   /* Warnings and error messages.  */

<paranoid>FWIW, I've regression tested this on x86_64 both unix/-m64 and
native-extended-gdbserver/-m64. There were no issues.</paranoid>

Keith