Re: [PATCH v1 05/10] gdb: introduce helper class file_reader_t

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Matthieu Longo <matthieu.longo@arm.com> writes:

> Wrap all the boilerplate code required to read a file in a new helper
> class: file_reader_t. The class owns the file contents together with
> the file path, and provides convenient accessors for the data, size and
> typed views. It supports both null-terminated text files and binary files.
>
> This helper eliminates repeated calls to target_fileio_read_stralloc
> and target_fileio_read_alloc, remove explicit memory management with
> gdb::unique_xmalloc_ptr, and simplifies the casting logic when working
> with binary data.
>
> The patch converts some of the existing Linux, AMD64, and SPARC code that
> reads files from /proc to use file_reader_t.
> ---
>  gdb/amd64-linux-tdep.c |  12 ++---
>  gdb/linux-tdep.c       | 102 +++++++++++++++++------------------------
>  gdb/sparc64-tdep.c     |  12 ++---
>  gdb/target.h           |  65 ++++++++++++++++++++++++++
>  4 files changed, 117 insertions(+), 74 deletions(-)

With the changes I suggested below:

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

> @@ -2285,23 +2280,24 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
>  
>    /* Obtaining PID and filename.  */
>    pid = inferior_ptid.pid ();
> -  xsnprintf (filename, sizeof (filename), "/proc/%d/cmdline", (int) pid);
> -  /* The full name of the program which generated the corefile.  */
> -  gdb_byte *buf = nullptr;
> -  LONGEST buf_len = target_fileio_read_alloc (nullptr, filename, &buf);
> -  gdb::unique_xmalloc_ptr<char> fname ((char *)buf);
> +  file_reader_t<gdb_byte> cmdline_freader
> +    (string_printf ("/proc/%d/cmdline", pid));

I was scratching my head for a bit trying to understand why not use
file_reader_t<char> here and avoid having to use cast_view<char>
below. I think it's worth a comment mentioning that it's because cmdline
has '\0' separating the arguments.

> +  if (!cmdline_freader)
> +    return false;
>  
> -  if (buf_len < 1 || fname.get () == nullptr || fname.get ()[0] == '\0')
> +  /* The full name of the program which generated the corefile.  */
> +  gdb::array_view<char> cmdline = cmdline_freader.cast_view<char> ();
> +  if (cmdline.size () < 1 || cmdline[0] == '\0')
>      {
>        /* No program name was read, so we won't be able to retrieve more
>  	 information about the process.  */
>        return false;
>      }
> -  if (fname.get ()[buf_len - 1] != '\0')
> +  if (cmdline[cmdline.size () - 1] != '\0')
>      {
>        warning (_("target file %s "
>  		 "does not contain a trailing null character"),
> -	       filename);
> +	       cmdline_freader.c_filepath ());
>        return false;
>      }
>  
⋮
> diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c
> index 93db3417a2a..811b76f27d8 100644
> --- a/gdb/sparc64-tdep.c
> +++ b/gdb/sparc64-tdep.c
> @@ -305,14 +305,13 @@ adi_is_addr_mapped (CORE_ADDR vaddr, size_t cnt)
>    size_t i = 0;
>  
>    pid_t pid = inferior_ptid.pid ();
> -  snprintf (filename, sizeof filename, "/proc/%ld/adi/maps", (long) pid);
> -  gdb::unique_xmalloc_ptr<char> data
> -    = target_fileio_read_stralloc (NULL, filename);

This change leaves filename unused and I get a build error when using
"configure --enable-targets=all":

  CXX    sparc64-tdep.o
/home/bauermann/src/binutils-gdb-wt-2/gdb/sparc64-tdep.c: In function 'bool adi_is_addr_mapped(CORE_ADDR, size_t)':
/home/bauermann/src/binutils-gdb-wt-2/gdb/sparc64-tdep.c:304:8: error: unused variable 'filename' [-Werror=unused-variable]
  304 |   char filename[MAX_PROC_NAME_SIZE];
      |        ^~~~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [Makefile:2100: sparc64-tdep.o] Error 1

> -  if (data)
> +  file_reader_t<char> adi_maps_freader
> +    (string_printf ("/proc/%d/adi/maps", pid));

I'll just note that this patch changes the printf pattern from "%ld" and
"(long) pid" to "%d" and "pid". I think the change is for the better and
I can't think of why it was done the other way before.

> +  if (adi_maps_freader)
>      {
>        adi_stat_t adi_stat = get_adi_info (pid);
>        char *saveptr;
> -      for (char *line = strtok_r (data.get (), "\n", &saveptr);
> +      for (char *line = strtok_r (adi_maps_freader.data (), "\n", &saveptr);
>  	   line;
>  	   line = strtok_r (NULL, "\n", &saveptr))
>  	{

-- 
Thiago
(he/him)