Re: [PATCH v2 4/6] gdb/linux-tdep: migrate linux_info_proc to file_reader_t

[Andrew Burgess <aburgess@redhat.com>]

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

> The patch migrates the code of linux_info_proc to use file_reader_t to
> 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.
>
> Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> ---
>  gdb/linux-tdep.c | 135 +++++++++++++++++++++++++++++------------------
>  1 file changed, 84 insertions(+), 51 deletions(-)
>
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index 84614bc91a0..e2c5b2d8815 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -866,6 +866,39 @@ dump_note_entry_p (filter_flags filterflags, const smaps_data &map)
>    return true;
>  }
>  
> +/* Extract a string view from BUFFER starting at START and ending at the
> +   first occurrence of SEPARATOR.
> +   Return the extracted view together with an iterator to the beginning of
> +   the next entry, skipping any successive separators.  If no separator
> +   is found, return the remainder of BUFFER starting at START.  If there is
> +   no following entry, the returned iterator is BUFFER.end ().  */
> +
> +static std::pair<gdb::array_view<char>, gdb::array_view<char>::iterator>
> +extract_string_view_from_buffer (gdb::array_view<char> &buffer,

The gdbsupport/array-view.h header says that gdb::array_view objects
should usually be passed by value.  Is there a reason why this needs to
be passed by reference here?



> @@ -914,25 +947,23 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>  
>    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);
> +	  /* /proc/<pid>/cmdline stores the command-line arguments as a
> +	     sequence of NUL-separated strings.  */
> +	  gdb::array_view<char> cmdline = cmdline_freader.cast_view<char> ();
> +	  gdb_assert (cmdline[cmdline.size () - 1] == '\0');

We shouldn't assert on data from an outside source.  This should be
either an error, or a warning if GDB is able to handle this and push
on.



> @@ -997,12 +1025,16 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>  	  current_uiout->table_header (0, ui_left, "objfile", "File");
>  	  current_uiout->table_body ();
>  
> -	  char *saveptr;
> -	  for (const char *line = strtok_r (map.get (), "\n", &saveptr);
> -	       line != nullptr;
> -	       line = strtok_r (nullptr, "\n", &saveptr))
> +	  auto content = map_freader.view ();
> +	  for (auto it = content.begin (); it != content.end ();)
>  	    {
> -	      struct mapping m = read_mapping (line);
> +	      auto [line, next_line_begin]
> +		= extract_string_view_from_buffer (content, it, '\n');
> +	      it = next_line_begin;
> +
> +	      /* read_mapping() expects a null-terminated string.  */
> +	      *std::prev (it) = '\0';

If the buffer contains two consecutive '\n' characters then this will
overwrite the wrong one I think, e.g. "abc\n\ndef\n". IT will point at
the 'd', and this will overwrite the second '\n', not the first, which I
think is what you want.

Also, extract_string_view_from_buffer handles a missing final '\n', so
if the buffer is "abc\bdef" then IT will point to the character after
'f', and the above will overwrite 'f'.

Given that extract_string_view_from_buffer handles these cases, I think
this code should also handle them, or throw an error if we see data in a
form that you don't expect.


Thanks,
Andrew