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

[Luis <luis.machado.foss@gmail.com>]

Drive-by review.

On 07/07/2026 16:48, Matthieu Longo wrote:
> The patch migratse the code of linux_info_proc to use file_reader_t to

Typo: migratse

> 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(-)
> 
> 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);
> +}
> +
>   /* 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');

Formatting: Stray space before cmdline.size ()

> +	  /* 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)
>       {
> @@ -910,27 +926,25 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>       }
>     if (environ_f)
>       {
> -      xsnprintf (filename, sizeof filename, "/proc/%ld/environ", ptid.lwp ());
> -      gdb_byte *buffer;
> -      LONGEST len = target_fileio_read_alloc (nullptr, filename, &buffer);
> -
> -      if (len > 0)
> +      file_reader_t<gdb_byte> environ_freader
> +	(string_printf ("/proc/%ld/environ", ptid.lwp ()));
> +      if (environ_freader)
>   	{
> -	  gdb::unique_xmalloc_ptr<char> dealloc ((char *) buffer);
>   	  gdb_printf (_("Environment variables:\n\n"));
> -
> +	  gdb::array_view<char> buffer = environ_freader.cast_view<char> ();
>   	  /* Entries are separated by the null character.
>   	     Print each environment variable, line by line.  */
> -	  gdb_byte *buffer_end = buffer + len;
> -	  while (buffer < buffer_end)
> +	  for (auto it = buffer.begin (); it != buffer.end ();)
>   	    {
> -	      gdb_printf ("  %s\n", buffer);
> -	      /* +1 for the null character.  */
> -	      buffer += strlen ((char *) buffer) + 1;
> +	      auto [ntbs, next_start]
> +		= extract_string_view_from_buffer (buffer, it, '\0');
> +	      gdb_printf ("  %s\n", ntbs.data ());
> +	      it = next_start;
>   	    }
>   	}
>         else
> -	warning (_("unable to open /proc file '%s'"), filename);
> +	warning (_("unable to open /proc file '%s'"),
> +		 environ_freader.c_filepath());
>       }
>     if (exe_f)
>       {
> @@ -944,10 +958,9 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>       }
>     if (mappings_f)
>       {
> -      xsnprintf (filename, sizeof filename, "/proc/%ld/maps", ptid.lwp ());
> -      gdb::unique_xmalloc_ptr<char> map
> -	= target_fileio_read_stralloc (NULL, filename);
> -      if (map != NULL)
> +      file_reader_t<char> map_freader
> +	(string_printf ("/proc/%ld/maps", ptid.lwp ()));
> +      if (map_freader)
>   	{
>   	  gdb_printf (_("Mapped address spaces:\n\n"));
>   	  ui_out_emit_table emitter (current_uiout, 6, -1, "ProcMappings");
> @@ -961,12 +974,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;
> +

Is there a risk we will drop a final chunk of the data when the buffer 
does not end in \n here (or more generally, does not end in whatever 
separator we're looking for), comparing it with the old strtok_r behavior?

It's a corner case, but I thought I´d check.

> +	      /* read_mapping() expects a null-terminated string.  */
> +	      *std::prev (it) = '\0';
> +	      struct mapping m = read_mapping (line.data ());
>   
>   	      ui_out_emit_tuple tuple_emitter (current_uiout, nullptr);
>   	      current_uiout->field_core_addr ("start", gdbarch, m.addr);
> @@ -985,26 +1002,26 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>   	    }
>   	}
>         else
> -	warning (_("unable to open /proc file '%s'"), filename);
> +	warning (_("unable to open /proc file '%s'"),
> +		 map_freader.c_filepath ());
>       }
>     if (status_f)
>       {
> -      xsnprintf (filename, sizeof filename, "/proc/%ld/status", ptid.lwp ());
> -      gdb::unique_xmalloc_ptr<char> status
> -	= target_fileio_read_stralloc (NULL, filename);
> -      if (status)
> -	gdb_puts (status.get ());
> +      file_reader_t<char> status_freader
> +	(string_printf ("/proc/%ld/status", ptid.lwp ()));
> +      if (status_freader)
> +	gdb_puts (status_freader.data ());
>         else
> -	warning (_("unable to open /proc file '%s'"), filename);
> +	warning (_("unable to open /proc file '%s'"),
> +		 status_freader.c_filepath ());
>       }
>     if (stat_f)
>       {
> -      xsnprintf (filename, sizeof filename, "/proc/%ld/stat", ptid.lwp ());
> -      gdb::unique_xmalloc_ptr<char> statstr
> -	= target_fileio_read_stralloc (NULL, filename);
> -      if (statstr)
> +      file_reader_t<char> stat_freader
> +	(string_printf ("/proc/%ld/stat", ptid.lwp ()));
> +      if (stat_freader)
>   	{
> -	  const char *p = statstr.get ();
> +	  const char *p = stat_freader.data ();
>   
>   	  gdb_printf (_("Process: %s\n"),
>   		      pulongest (strtoulst (p, &p, 10)));
> @@ -1131,7 +1148,8 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>   #endif
>   	}
>         else
> -	warning (_("unable to open /proc file '%s'"), filename);
> +	warning (_("unable to open /proc file '%s'"),
> +		 stat_freader.c_filepath());

Formatting: Space before parens. Multiple cases.

>       }
>   }
>