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

[Matthieu Longo <matthieu.longo@arm.com>]

On 10/09/2026 17:31, Andrew Burgess wrote:
> 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?
> 

No good reason.
Fixed.

> 
>> @@ -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.
> 

Replaced assert by:

	  if (cmdline[cmdline.size () - 1] != '\0')
	    warning (_("malformed '%ps', missing null-terminating character"),
		     styled_string (file_name_style.style (), path.c_str ()));

with 'path' defined a bit before:
  std::string path = string_printf ("/proc/%ld/cmdline", ptid.lwp ());

>> @@ -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.
> 

I completely missed this issue. Thanks for noticing it.

> 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.
> 

extract_string_view_from_buffer is not supposed to change the value inside the buffer.

I adapted read_mapping and parse_smaps_data to use a gdb::array_view<char>. This removes the need to
set '\0' inside extract_string_view_from_buffer or before passing it to read_mapping.
read_mapping unfortunately needs to keep its view writable because it needs to set '\0' for
mapping.filename. To remove this need, a solution would consists in changing mapping.filename to a
std::string_view but then, it would make the printing harder. So I preferred to keep it this way for
now.

Those changes were moved to different patches, to keep the migration patch more focused on the
migration.

> 
> Thanks,
> Andrew

Matthieu