diff mbox series

[v2,6/6] gdb/linux-tdep: remove legacy parse_smaps_data overload

Message ID 20260825100912.514232-7-matthieu.longo@arm.com
State New
Headers
Series gdb: introduce file_reader_t to read procfs files |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 fail Patch failed to apply

Commit Message

Matthieu Longo Aug. 25, 2026, 10:09 a.m. UTC 
  Now that all callers use the file_reader_t overload of parse_smaps_data,
the legacy interface taking a raw buffer and filename separately is no
longer needed.

Fold its implementation into the file_reader_t version and remove the
obsolete wrapper.  This also simplifies the implementation by using the
file_reader_t accessors directly.

Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Reviewed-By: Luis Machado <luis.machado.foss@gmail.com>
---
 gdb/linux-tdep.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

Comments

Andrew Burgess Sept. 11, 2026, 8:41 a.m. UTC | #1 
Matthieu Longo <matthieu.longo@arm.com> writes:

> Now that all callers use the file_reader_t overload of parse_smaps_data,
> the legacy interface taking a raw buffer and filename separately is no
> longer needed.
>
> Fold its implementation into the file_reader_t version and remove the
> obsolete wrapper.  This also simplifies the implementation by using the
> file_reader_t accessors directly.
>
> Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Reviewed-By: Luis Machado <luis.machado.foss@gmail.com>
> ---
>  gdb/linux-tdep.c | 30 ++++++++++++------------------
>  1 file changed, 12 insertions(+), 18 deletions(-)
>
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index 2833d8a4bb6..4cdb02b9c09 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -1581,18 +1581,17 @@ parse_smaps_key_value (const char *keyword, const char *line,
>  /* Helper function to parse the contents of /proc/<pid>/smaps into a data
>     structure, for easy access.
>  
> -   DATA is the contents of the smaps file.  The parsed contents are stored
> -   into the SMAPS vector.  */
> +   FREADER is a wrapper around the contents of the smaps file.
> +   The parsed contents are stored into the SMAPS vector.  */

As you are editing this comment anyway, could you fix it too please.
The parsed contents are not "stored into the SMAPS vector", but are
returned as a vector.

>  
>  static std::vector<smaps_data>
> -parse_smaps_data (const char *data,
> -		  const std::string &maps_filename)
> +parse_smaps_data (const file_reader_t<char> &freader)
>  {
>    char *line, *t;
>  
> -  gdb_assert (data != nullptr);
> +  gdb_assert (freader);
>  
> -  line = strtok_r ((char *) data, "\n", &t);
> +  line = strtok_r (freader.data (), "\n", &t);
>  
>    std::vector<smaps_data> smaps;
>  
> @@ -1648,8 +1647,8 @@ parse_smaps_data (const char *data,
>  
>  	  if (sscanf (line, "%64s", keyword) != 1)
>  	    {
> -	      warning (_("Error parsing {s,}maps file '%s'"),
> -		       maps_filename.c_str ());
> +	      warning (_("Error parsing keyword in {s,}maps file '%s'"),
> +		       freader.c_filepath ());

Also, while you're passing, warning message should start with a lower
case letter (as they are printed "warning: ..."), could you fix this
please. And could you add filename styling too:

  warning (_("error parsing keyword in {s,}maps file '%ps'"),
           styled_string (file_name_style.style (),
                          freader.c_filepath ()));

There's another place that would benefit from this cleanup below.

Thanks,
Andrew

>  	      break;
>  	    }
>  
> @@ -1663,12 +1662,12 @@ parse_smaps_data (const char *data,
>  	    decode_vmflags (line, &v);
>  
>  	  if (parse_smaps_key_value (keyword, line, "Rss:",
> -				     maps_filename,
> +				     freader.filepath (),
>  				     &rss))
>  	    continue;
>  
>  	  if (parse_smaps_key_value (keyword, line, "Swap:",
> -				     maps_filename,
> +				     freader.filepath (),
>  				     &swap))
>  	    continue;
>  
> @@ -1679,8 +1678,9 @@ parse_smaps_data (const char *data,
>  
>  	      if (sscanf (line, "%*s%lu", &number) != 1)
>  		{
> -		  warning (_("Error parsing {s,}maps file '%s' number"),
> -			   maps_filename.c_str ());
> +		  warning (_("Error parsing numeric value associated with "
> +			     "key '%s' in {s,}maps file '%s'"),
> +			   keyword, freader.c_filepath ());
>  		  break;
>  		}
>  	      if (number > 0)
> @@ -1730,12 +1730,6 @@ parse_smaps_data (const char *data,
>    return smaps;
>  }
>  
> -static std::vector<smaps_data>
> -parse_smaps_data (const file_reader_t<char> &freader)
> -{
> -  return parse_smaps_data (freader.data (), freader.filepath ());
> -}
> -
>  /* Helper that checks if an address is in a memory tag page for a live
>     process.  */
>  
> -- 
> 2.55.0
Matthieu Longo Sept. 16, 2026, 10:55 a.m. UTC | #2 
On 11/09/2026 09:41, Andrew Burgess wrote:
> Matthieu Longo <matthieu.longo@arm.com> writes:
> 
>> Now that all callers use the file_reader_t overload of parse_smaps_data,
>> the legacy interface taking a raw buffer and filename separately is no
>> longer needed.
>>
>> Fold its implementation into the file_reader_t version and remove the
>> obsolete wrapper.  This also simplifies the implementation by using the
>> file_reader_t accessors directly.
>>
>> Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
>> Reviewed-By: Luis Machado <luis.machado.foss@gmail.com>
>> ---
>>  gdb/linux-tdep.c | 30 ++++++++++++------------------
>>  1 file changed, 12 insertions(+), 18 deletions(-)
>>
>> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
>> index 2833d8a4bb6..4cdb02b9c09 100644
>> --- a/gdb/linux-tdep.c
>> +++ b/gdb/linux-tdep.c
>> @@ -1581,18 +1581,17 @@ parse_smaps_key_value (const char *keyword, const char *line,
>>  /* Helper function to parse the contents of /proc/<pid>/smaps into a data
>>     structure, for easy access.
>>  
>> -   DATA is the contents of the smaps file.  The parsed contents are stored
>> -   into the SMAPS vector.  */
>> +   FREADER is a wrapper around the contents of the smaps file.
>> +   The parsed contents are stored into the SMAPS vector.  */
> 
> As you are editing this comment anyway, could you fix it too please.
> The parsed contents are not "stored into the SMAPS vector", but are
> returned as a vector.
> 

Fixed in the next revision.

>>  
>>  static std::vector<smaps_data>
>> -parse_smaps_data (const char *data,
>> -		  const std::string &maps_filename)
>> +parse_smaps_data (const file_reader_t<char> &freader)
>>  {
>>    char *line, *t;
>>  
>> -  gdb_assert (data != nullptr);
>> +  gdb_assert (freader);
>>  
>> -  line = strtok_r ((char *) data, "\n", &t);
>> +  line = strtok_r (freader.data (), "\n", &t);
>>  
>>    std::vector<smaps_data> smaps;
>>  
>> @@ -1648,8 +1647,8 @@ parse_smaps_data (const char *data,
>>  
>>  	  if (sscanf (line, "%64s", keyword) != 1)
>>  	    {
>> -	      warning (_("Error parsing {s,}maps file '%s'"),
>> -		       maps_filename.c_str ());
>> +	      warning (_("Error parsing keyword in {s,}maps file '%s'"),
>> +		       freader.c_filepath ());
> 
> Also, while you're passing, warning message should start with a lower
> case letter (as they are printed "warning: ..."), could you fix this
> please. And could you add filename styling too:
> 
>   warning (_("error parsing keyword in {s,}maps file '%ps'"),
>            styled_string (file_name_style.style (),
>                           freader.c_filepath ()));
> 
> There's another place that would benefit from this cleanup below.
> 
> Thanks,
> Andrew
> 

Fixed here...

>>  	      break;
>>  	    }
>>  
>> @@ -1663,12 +1662,12 @@ parse_smaps_data (const char *data,
>>  	    decode_vmflags (line, &v);
>>  
>>  	  if (parse_smaps_key_value (keyword, line, "Rss:",
>> -				     maps_filename,
>> +				     freader.filepath (),
>>  				     &rss))
>>  	    continue;
>>  
>>  	  if (parse_smaps_key_value (keyword, line, "Swap:",
>> -				     maps_filename,
>> +				     freader.filepath (),
>>  				     &swap))
>>  	    continue;
>>  
>> @@ -1679,8 +1678,9 @@ parse_smaps_data (const char *data,
>>  
>>  	      if (sscanf (line, "%*s%lu", &number) != 1)
>>  		{
>> -		  warning (_("Error parsing {s,}maps file '%s' number"),
>> -			   maps_filename.c_str ());
>> +		  warning (_("Error parsing numeric value associated with "
>> +			     "key '%s' in {s,}maps file '%s'"),
>> +			   keyword, freader.c_filepath ());

and here in the next revision.

>>  		  break;
>>  		}
>>  	      if (number > 0)
>> @@ -1730,12 +1730,6 @@ parse_smaps_data (const char *data,
>>    return smaps;
>>  }
>>  
>> -static std::vector<smaps_data>
>> -parse_smaps_data (const file_reader_t<char> &freader)
>> -{
>> -  return parse_smaps_data (freader.data (), freader.filepath ());
>> -}
>> -
>>  /* Helper that checks if an address is in a memory tag page for a live
>>     process.  */
>>  
>> -- 
>> 2.55.0
> 

Matthieu
diff mbox series

Patch

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 2833d8a4bb6..4cdb02b9c09 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1581,18 +1581,17 @@  parse_smaps_key_value (const char *keyword, const char *line,
 /* Helper function to parse the contents of /proc/<pid>/smaps into a data
    structure, for easy access.
 
-   DATA is the contents of the smaps file.  The parsed contents are stored
-   into the SMAPS vector.  */
+   FREADER is a wrapper around the contents of the smaps file.
+   The parsed contents are stored into the SMAPS vector.  */
 
 static std::vector<smaps_data>
-parse_smaps_data (const char *data,
-		  const std::string &maps_filename)
+parse_smaps_data (const file_reader_t<char> &freader)
 {
   char *line, *t;
 
-  gdb_assert (data != nullptr);
+  gdb_assert (freader);
 
-  line = strtok_r ((char *) data, "\n", &t);
+  line = strtok_r (freader.data (), "\n", &t);
 
   std::vector<smaps_data> smaps;
 
@@ -1648,8 +1647,8 @@  parse_smaps_data (const char *data,
 
 	  if (sscanf (line, "%64s", keyword) != 1)
 	    {
-	      warning (_("Error parsing {s,}maps file '%s'"),
-		       maps_filename.c_str ());
+	      warning (_("Error parsing keyword in {s,}maps file '%s'"),
+		       freader.c_filepath ());
 	      break;
 	    }
 
@@ -1663,12 +1662,12 @@  parse_smaps_data (const char *data,
 	    decode_vmflags (line, &v);
 
 	  if (parse_smaps_key_value (keyword, line, "Rss:",
-				     maps_filename,
+				     freader.filepath (),
 				     &rss))
 	    continue;
 
 	  if (parse_smaps_key_value (keyword, line, "Swap:",
-				     maps_filename,
+				     freader.filepath (),
 				     &swap))
 	    continue;
 
@@ -1679,8 +1678,9 @@  parse_smaps_data (const char *data,
 
 	      if (sscanf (line, "%*s%lu", &number) != 1)
 		{
-		  warning (_("Error parsing {s,}maps file '%s' number"),
-			   maps_filename.c_str ());
+		  warning (_("Error parsing numeric value associated with "
+			     "key '%s' in {s,}maps file '%s'"),
+			   keyword, freader.c_filepath ());
 		  break;
 		}
 	      if (number > 0)
@@ -1730,12 +1730,6 @@  parse_smaps_data (const char *data,
   return smaps;
 }
 
-static std::vector<smaps_data>
-parse_smaps_data (const file_reader_t<char> &freader)
-{
-  return parse_smaps_data (freader.data (), freader.filepath ());
-}
-
 /* Helper that checks if an address is in a memory tag page for a live
    process.  */