Re: [PATCH v1 06/10] gdb/linux-tdep: migrate linux_info_proc to file_reader_t

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

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

> The patch migratse the code of linux_info_proc to use file_reader_t to

Typo: migrates

> read the procfs files.
> The availability of array_views allows to also simplify the logic in
> several places, where null-terminating characters are replaced by spaces,
> or where the file content is iterated line by line.
> In the last case, a new helper function, extract_string_view_from_buffer,
> encapsulates the logic for such iterations where string are separated by
> tokens.
> ---
>  gdb/linux-tdep.c | 120 +++++++++++++++++++++++++++--------------------
>  1 file changed, 69 insertions(+), 51 deletions(-)

Nice improvement, thanks!

Just a couple of nits, but:

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

> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index a12a69f03a2..a2af1586d35 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -836,6 +836,27 @@ dump_note_entry_p (filter_flags filterflags, const smaps_data &map)
>    return true;
>  }
>  
> +/* In a character buffer where entries are separated by a SEPARATOR character,
> +   extract the string view starting at START.
> +   Return the extracted view and the iterator to the next entry.  */
> +
> +static std::pair<gdb::array_view<char>, gdb::array_view<char>::iterator>
> +extract_string_view_from_buffer (gdb::array_view<char> &buffer,
> +				 gdb::array_view<char>::iterator start,
> +				 char separator = '\0')
> +{
> +  if (start < buffer.begin () || start >= buffer.end ())
> +    return std::make_pair (gdb::array_view<char> (), buffer.end ());
> +
> +  auto it = std::find (start, buffer.end (), separator);
> +  if (it == buffer.end ())
> +    return std::make_pair (gdb::array_view<char> (), buffer.end ());
> +
> +  auto next_start = std::next (it);
> +  return std::make_pair
> +    (gdb::array_view<char> (start, next_start), next_start);

This line fits 80 columns and doesn't need to be broken.

> +}
> +
>  /* Implement the "info proc" command.  */
>  
>  static void
> @@ -878,25 +899,20 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>    gdb_printf (_("process %d\n"), ptid.pid ());
>    if (cmdline_f)
>      {
> -      xsnprintf (filename, sizeof filename, "/proc/%ld/cmdline", ptid.lwp ());
> -      gdb_byte *buffer;
> -      LONGEST len = target_fileio_read_alloc (nullptr, filename, &buffer);
> -
> -      if (len > 0)
> +      file_reader_t<gdb_byte> cmdline_freader
> +	(string_printf ("/proc/%ld/cmdline", ptid.lwp ()));
> +      if (cmdline_freader)
>  	{
> -	  gdb::unique_xmalloc_ptr<char> cmdline ((char *) buffer);
> -	  ssize_t pos;
> -
> -	  for (pos = 0; pos < len - 1; pos++)
> -	    {
> -	      if (buffer[pos] == '\0')
> -		buffer[pos] = ' ';
> -	    }
> -	  buffer[len - 1] = '\0';
> -	  gdb_printf ("cmdline = '%s'\n", buffer);
> +	  gdb::array_view<char> cmdline = cmdline_freader.cast_view<char> ();
> +	  gdb_assert (cmdline[ cmdline.size () - 1] == '\0');

Extraneous space after '['.

> +	  /* Replace null characters splitting the arguments in the command
> +	     line by spaces, except for the last one.  */
> +	  gdb::replace (cmdline.slice (0, cmdline.size () - 1), '\0', ' ');
> +	  gdb_printf ("cmdline = '%s'\n", cmdline.data ());
>  	}
>        else
> -	warning (_("unable to open /proc file '%s'"), filename);
> +	warning (_("unable to open /proc file '%s'"),
> +		 cmdline_freader.c_filepath());
>      }
>    if (cwd_f)
>      {

-- 
Thiago
(he/him)