* [PATCH v2 1/6] target_fileio_read_stralloc: add an optional length parameter
2026-08-25 10:09 [PATCH v2 0/6] gdb: introduce file_reader_t to read procfs files Matthieu Longo
@ 2026-08-25 10:09 ` Matthieu Longo
2026-09-10 10:55 ` Andrew Burgess
2026-08-25 10:09 ` [PATCH v2 2/6] gdb support: add gdb::ranges::replace algorithm Matthieu Longo
` (6 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Matthieu Longo @ 2026-08-25 10:09 UTC (permalink / raw)
To: gdb-patches
Cc: Simon Marchi, Thiago Jung Bauermann, Luis Machado, Luis Machado,
Christina Joos, Kevin Buettner, Matthieu Longo
Extend target_fileio_read_stralloc with an optional output parameter
that returns the number of bytes read, excluding the terminating NUL
byte.
Most callers only need the returned NUL-terminated buffer and can
ignore the new parameter, but callers that need the number of bytes
read can now obtain it without recomputing it.
While updating the function, make a couple of minor cleanups by using
'\0' instead of 0 for character literals and clarifying the function
documentation.
Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
---
gdb/target.c | 11 ++++++++---
gdb/target.h | 20 ++++++++++++--------
2 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/gdb/target.c b/gdb/target.c
index 5d937f3ae85..ebf0f30092c 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3547,7 +3547,8 @@ target_fileio_read_alloc (struct inferior *inf, const char *filename,
/* See target.h. */
gdb::unique_xmalloc_ptr<char>
-target_fileio_read_stralloc (struct inferior *inf, const char *filename)
+target_fileio_read_stralloc (struct inferior *inf, const char *filename,
+ LONGEST *len)
{
gdb_byte *buffer;
char *bufstr;
@@ -3556,17 +3557,21 @@ target_fileio_read_stralloc (struct inferior *inf, const char *filename)
transferred = target_fileio_read_alloc_1 (inf, filename, &buffer, 1);
bufstr = (char *) buffer;
+ /* Note: on failure, target_fileio_read_alloc_1 returns -1. */
+ if (len != nullptr)
+ *len = transferred;
+
if (transferred < 0)
return gdb::unique_xmalloc_ptr<char> (nullptr);
if (transferred == 0)
return make_unique_xstrdup ("");
- bufstr[transferred] = 0;
+ bufstr[transferred] = '\0';
/* Check for embedded NUL bytes; but allow trailing NULs. */
for (i = strlen (bufstr); i < transferred; i++)
- if (bufstr[i] != 0)
+ if (bufstr[i] != '\0')
{
warning (_("target file %s "
"contained unexpected null characters"),
diff --git a/gdb/target.h b/gdb/target.h
index 6d1c21f29f6..819279c08fc 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2327,15 +2327,19 @@ extern LONGEST target_fileio_read_alloc (struct inferior *inf,
const char *filename,
gdb_byte **buf_p);
-/* Read target file FILENAME, in the filesystem as seen by INF. If
- INF is NULL, use the filesystem seen by the debugger (GDB or, for
- remote targets, the remote stub). The result is NUL-terminated and
- returned as a string, allocated using xmalloc. If an error occurs
- or the transfer is unsupported, NULL is returned. Empty objects
- are returned as allocated but empty strings. A warning is issued
- if the result contains any embedded NUL bytes. */
+/* Read the content of the target file FILENAME from the filesystem as
+ seen by INF. If INF is NULL, use the filesystem seen by the debugger
+ (GDB or, for remote targets, the remote stub).
+
+ If LEN is not NULL, store the number of bytes read, excluding the
+ terminating NUL byte.
+
+ The returned buffer is NUL-terminated and allocated using xmalloc.
+ On error, or if the transfer is unsupported, return NULL and set
+ LEN to -1. Empty files are returned as allocated but empty strings.
+ A warning is issued if the file content contains embedded NUL bytes. */
extern gdb::unique_xmalloc_ptr<char> target_fileio_read_stralloc
- (struct inferior *inf, const char *filename);
+ (struct inferior *inf, const char *filename, LONGEST *len = nullptr);
/* Invalidate the target associated with open handles that were open
on target TARG, since we're about to close (and maybe destroy) the
--
2.55.0
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 1/6] target_fileio_read_stralloc: add an optional length parameter
2026-08-25 10:09 ` [PATCH v2 1/6] target_fileio_read_stralloc: add an optional length parameter Matthieu Longo
@ 2026-09-10 10:55 ` Andrew Burgess
0 siblings, 0 replies; 19+ messages in thread
From: Andrew Burgess @ 2026-09-10 10:55 UTC (permalink / raw)
To: Matthieu Longo, gdb-patches
Cc: Simon Marchi, Thiago Jung Bauermann, Luis Machado, Luis Machado,
Christina Joos, Kevin Buettner, Matthieu Longo
Matthieu Longo <matthieu.longo@arm.com> writes:
> Extend target_fileio_read_stralloc with an optional output parameter
> that returns the number of bytes read, excluding the terminating NUL
> byte.
>
> Most callers only need the returned NUL-terminated buffer and can
> ignore the new parameter, but callers that need the number of bytes
> read can now obtain it without recomputing it.
>
> While updating the function, make a couple of minor cleanups by using
> '\0' instead of 0 for character literals and clarifying the function
> documentation.
This looks fine with on minor nit, see below...
>
> Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> ---
> gdb/target.c | 11 ++++++++---
> gdb/target.h | 20 ++++++++++++--------
> 2 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/target.c b/gdb/target.c
> index 5d937f3ae85..ebf0f30092c 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -3547,7 +3547,8 @@ target_fileio_read_alloc (struct inferior *inf, const char *filename,
> /* See target.h. */
>
> gdb::unique_xmalloc_ptr<char>
> -target_fileio_read_stralloc (struct inferior *inf, const char *filename)
> +target_fileio_read_stralloc (struct inferior *inf, const char *filename,
> + LONGEST *len)
> {
> gdb_byte *buffer;
> char *bufstr;
> @@ -3556,17 +3557,21 @@ target_fileio_read_stralloc (struct inferior *inf, const char *filename)
> transferred = target_fileio_read_alloc_1 (inf, filename, &buffer, 1);
> bufstr = (char *) buffer;
>
> + /* Note: on failure, target_fileio_read_alloc_1 returns -1. */
There is no needs for a 'Note:' prefix. By definition, comments are
things about which we should take note. Just:
/* On failure target_fileio_read_alloc_1 returns -1. */
Is fine.
With that fixed:
Approved-By: Andrew Burgess <aburgess@redhat.com>
thanks,
Andrew
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 2/6] gdb support: add gdb::ranges::replace algorithm
2026-08-25 10:09 [PATCH v2 0/6] gdb: introduce file_reader_t to read procfs files Matthieu Longo
2026-08-25 10:09 ` [PATCH v2 1/6] target_fileio_read_stralloc: add an optional length parameter Matthieu Longo
@ 2026-08-25 10:09 ` Matthieu Longo
2026-09-10 11:07 ` Andrew Burgess
2026-08-25 10:09 ` [PATCH v2 3/6] gdb: introduce helper class file_reader_t Matthieu Longo
` (5 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Matthieu Longo @ 2026-08-25 10:09 UTC (permalink / raw)
To: gdb-patches
Cc: Simon Marchi, Thiago Jung Bauermann, Luis Machado, Luis Machado,
Christina Joos, Kevin Buettner, Matthieu Longo
Provide a C++17-compatible replacement for the C++20 std::ranges::replace
algorithm, allowing callers to use a consistent interface until GDB
transitions to C++20. The helper should be removed once the C++ standard
library implementation become available.
https://en.cppreference.com/cpp/algorithm/ranges/replace
---
gdbsupport/array-view.h | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/gdbsupport/array-view.h b/gdbsupport/array-view.h
index 8431d7f5add..f9842ecff30 100644
--- a/gdbsupport/array-view.h
+++ b/gdbsupport/array-view.h
@@ -225,6 +225,22 @@ void copy (gdb::array_view<U> src, gdb::array_view<T> dest)
std::copy_backward (src.begin (), src.end (), dest.end ());
}
+namespace ranges {
+
+/* Replace all occurrences of a value in the provided range.
+
+ Note: this helper is a reimplementation of std::ranges::replace, only
+ available from C++20 onwards, and consequently, should be replaced by
+ std::ranges::replace once GDB switches to C++20. */
+
+template <class Range, typename T>
+void replace (Range r, const T &old_value, const T &new_value)
+{
+ std::replace (r.begin (), r.end (), old_value, new_value);
+}
+
+} /* namespace ranges */
+
/* Compare LHS and RHS for (deep) equality. That is, whether LHS and
RHS have the same sizes, and whether each pair of elements of LHS
and RHS at the same position compares equal. */
--
2.55.0
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 2/6] gdb support: add gdb::ranges::replace algorithm
2026-08-25 10:09 ` [PATCH v2 2/6] gdb support: add gdb::ranges::replace algorithm Matthieu Longo
@ 2026-09-10 11:07 ` Andrew Burgess
2026-09-10 11:08 ` Andrew Burgess
2026-09-16 16:03 ` Matthieu Longo
0 siblings, 2 replies; 19+ messages in thread
From: Andrew Burgess @ 2026-09-10 11:07 UTC (permalink / raw)
To: Matthieu Longo, gdb-patches
Cc: Simon Marchi, Thiago Jung Bauermann, Luis Machado, Luis Machado,
Christina Joos, Kevin Buettner, Matthieu Longo
Matthieu Longo <matthieu.longo@arm.com> writes:
> Provide a C++17-compatible replacement for the C++20 std::ranges::replace
> algorithm, allowing callers to use a consistent interface until GDB
> transitions to C++20. The helper should be removed once the C++ standard
> library implementation become available.
>
> https://en.cppreference.com/cpp/algorithm/ranges/replace
> ---
> gdbsupport/array-view.h | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/gdbsupport/array-view.h b/gdbsupport/array-view.h
> index 8431d7f5add..f9842ecff30 100644
> --- a/gdbsupport/array-view.h
> +++ b/gdbsupport/array-view.h
> @@ -225,6 +225,22 @@ void copy (gdb::array_view<U> src, gdb::array_view<T> dest)
> std::copy_backward (src.begin (), src.end (), dest.end ());
> }
>
> +namespace ranges {
> +
> +/* Replace all occurrences of a value in the provided range.
> +
> + Note: this helper is a reimplementation of std::ranges::replace, only
> + available from C++20 onwards, and consequently, should be replaced by
> + std::ranges::replace once GDB switches to C++20. */
> +
> +template <class Range, typename T>
> +void replace (Range r, const T &old_value, const T &new_value)
Looking at the linked cppreference page, the C++20 functions take R as
'Range &&r'. Doesn't your versions create a copy of the range? This
will work fine for non-owning ranges, like gdb::array_view, but will
mean replace operates on a copy of the range for something like
std::vector. Even if what you have above is intentional, I think this
difference should be highlighted and explained.
Also, as this isn't specifically tied to gdb::array_view, I wonder if
this would be better put into a new file gdbsupport/ranges.h ? I don't
think anything much is needed to create the new file other than just
adding the file, so that should be pretty easy to do.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 2/6] gdb support: add gdb::ranges::replace algorithm
2026-09-10 11:07 ` Andrew Burgess
@ 2026-09-10 11:08 ` Andrew Burgess
2026-09-16 16:03 ` Matthieu Longo
2026-09-16 16:03 ` Matthieu Longo
1 sibling, 1 reply; 19+ messages in thread
From: Andrew Burgess @ 2026-09-10 11:08 UTC (permalink / raw)
To: Matthieu Longo, gdb-patches
Cc: Simon Marchi, Thiago Jung Bauermann, Luis Machado, Luis Machado,
Christina Joos, Kevin Buettner, Matthieu Longo
Andrew Burgess <aburgess@redhat.com> writes:
> Matthieu Longo <matthieu.longo@arm.com> writes:
>
>> Provide a C++17-compatible replacement for the C++20 std::ranges::replace
>> algorithm, allowing callers to use a consistent interface until GDB
>> transitions to C++20. The helper should be removed once the C++ standard
>> library implementation become available.
>>
>> https://en.cppreference.com/cpp/algorithm/ranges/replace
>> ---
>> gdbsupport/array-view.h | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/gdbsupport/array-view.h b/gdbsupport/array-view.h
>> index 8431d7f5add..f9842ecff30 100644
>> --- a/gdbsupport/array-view.h
>> +++ b/gdbsupport/array-view.h
>> @@ -225,6 +225,22 @@ void copy (gdb::array_view<U> src, gdb::array_view<T> dest)
>> std::copy_backward (src.begin (), src.end (), dest.end ());
>> }
>>
>> +namespace ranges {
>> +
>> +/* Replace all occurrences of a value in the provided range.
>> +
>> + Note: this helper is a reimplementation of std::ranges::replace, only
>> + available from C++20 onwards, and consequently, should be replaced by
>> + std::ranges::replace once GDB switches to C++20. */
>> +
>> +template <class Range, typename T>
>> +void replace (Range r, const T &old_value, const T &new_value)
>
> Looking at the linked cppreference page, the C++20 functions take R as
> 'Range &&r'. Doesn't your versions create a copy of the range? This
> will work fine for non-owning ranges, like gdb::array_view, but will
> mean replace operates on a copy of the range for something like
> std::vector. Even if what you have above is intentional, I think this
> difference should be highlighted and explained.
>
> Also, as this isn't specifically tied to gdb::array_view, I wonder if
> this would be better put into a new file gdbsupport/ranges.h ? I don't
> think anything much is needed to create the new file other than just
> adding the file, so that should be pretty easy to do.
Just as I hit send I realised that I should also say this would benefit
from some unittests, see gdb/unittests/ for examples.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 2/6] gdb support: add gdb::ranges::replace algorithm
2026-09-10 11:08 ` Andrew Burgess
@ 2026-09-16 16:03 ` Matthieu Longo
0 siblings, 0 replies; 19+ messages in thread
From: Matthieu Longo @ 2026-09-16 16:03 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
Cc: Simon Marchi, Thiago Jung Bauermann, Luis Machado, Luis Machado,
Christina Joos, Kevin Buettner
On 10/09/2026 12:08, Andrew Burgess wrote:
> Andrew Burgess <aburgess@redhat.com> writes:
>
>> Matthieu Longo <matthieu.longo@arm.com> writes:
>>
>>> Provide a C++17-compatible replacement for the C++20 std::ranges::replace
>>> algorithm, allowing callers to use a consistent interface until GDB
>>> transitions to C++20. The helper should be removed once the C++ standard
>>> library implementation become available.
>>>
>>> https://en.cppreference.com/cpp/algorithm/ranges/replace
>>> ---
>>> gdbsupport/array-view.h | 16 ++++++++++++++++
>>> 1 file changed, 16 insertions(+)
>>>
>>> diff --git a/gdbsupport/array-view.h b/gdbsupport/array-view.h
>>> index 8431d7f5add..f9842ecff30 100644
>>> --- a/gdbsupport/array-view.h
>>> +++ b/gdbsupport/array-view.h
>>> @@ -225,6 +225,22 @@ void copy (gdb::array_view<U> src, gdb::array_view<T> dest)
>>> std::copy_backward (src.begin (), src.end (), dest.end ());
>>> }
>>>
>>> +namespace ranges {
>>> +
>>> +/* Replace all occurrences of a value in the provided range.
>>> +
>>> + Note: this helper is a reimplementation of std::ranges::replace, only
>>> + available from C++20 onwards, and consequently, should be replaced by
>>> + std::ranges::replace once GDB switches to C++20. */
>>> +
>>> +template <class Range, typename T>
>>> +void replace (Range r, const T &old_value, const T &new_value)
>>
>> Looking at the linked cppreference page, the C++20 functions take R as
>> 'Range &&r'. Doesn't your versions create a copy of the range? This
>> will work fine for non-owning ranges, like gdb::array_view, but will
>> mean replace operates on a copy of the range for something like
>> std::vector. Even if what you have above is intentional, I think this
>> difference should be highlighted and explained.
>>
>> Also, as this isn't specifically tied to gdb::array_view, I wonder if
>> this would be better put into a new file gdbsupport/ranges.h ? I don't
>> think anything much is needed to create the new file other than just
>> adding the file, so that should be pretty easy to do.
>
> Just as I hit send I realised that I should also say this would benefit
> from some unittests, see gdb/unittests/ for examples.
>
> Thanks,
> Andrew
Added the unit tests in the next revision.
Matthieu
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/6] gdb support: add gdb::ranges::replace algorithm
2026-09-10 11:07 ` Andrew Burgess
2026-09-10 11:08 ` Andrew Burgess
@ 2026-09-16 16:03 ` Matthieu Longo
1 sibling, 0 replies; 19+ messages in thread
From: Matthieu Longo @ 2026-09-16 16:03 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
Cc: Simon Marchi, Thiago Jung Bauermann, Luis Machado, Luis Machado,
Christina Joos, Kevin Buettner
On 10/09/2026 12:07, Andrew Burgess wrote:
> Matthieu Longo <matthieu.longo@arm.com> writes:
>
>> Provide a C++17-compatible replacement for the C++20 std::ranges::replace
>> algorithm, allowing callers to use a consistent interface until GDB
>> transitions to C++20. The helper should be removed once the C++ standard
>> library implementation become available.
>>
>> https://en.cppreference.com/cpp/algorithm/ranges/replace
>> ---
>> gdbsupport/array-view.h | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/gdbsupport/array-view.h b/gdbsupport/array-view.h
>> index 8431d7f5add..f9842ecff30 100644
>> --- a/gdbsupport/array-view.h
>> +++ b/gdbsupport/array-view.h
>> @@ -225,6 +225,22 @@ void copy (gdb::array_view<U> src, gdb::array_view<T> dest)
>> std::copy_backward (src.begin (), src.end (), dest.end ());
>> }
>>
>> +namespace ranges {
>> +
>> +/* Replace all occurrences of a value in the provided range.
>> +
>> + Note: this helper is a reimplementation of std::ranges::replace, only
>> + available from C++20 onwards, and consequently, should be replaced by
>> + std::ranges::replace once GDB switches to C++20. */
>> +
>> +template <class Range, typename T>
>> +void replace (Range r, const T &old_value, const T &new_value)
>
> Looking at the linked cppreference page, the C++20 functions take R as
> 'Range &&r'. Doesn't your versions create a copy of the range?
Yes, it does.
Given that I was only using it gdb::array_view, I didn't notice the issue.
The replace cannot work with vectors as you pointed out below.
> This will work fine for non-owning ranges, like gdb::array_view, but will> mean replace operates
on a copy of the range for something like
> std::vector. Even if what you have above is intentional, I think this
> difference should be highlighted and explained.
>
I changed the function signature to use 'Range &&r'.
> Also, as this isn't specifically tied to gdb::array_view, I wonder if
> this would be better put into a new file gdbsupport/ranges.h ? I don't
> think anything much is needed to create the new file other than just
> adding the file, so that should be pretty easy to do.
>
> Thanks,
> Andrew
Done.
I also added:
#if __cplusplus >= 202002L
#error "replace the helpers below by the std::ranges::* equivalent."
#endif
in the file so that we don't forget about this and get rid of it once GDB migrates to C++20.
Is this ok ? Or to avoid ?
Matthieu
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 3/6] gdb: introduce helper class file_reader_t
2026-08-25 10:09 [PATCH v2 0/6] gdb: introduce file_reader_t to read procfs files Matthieu Longo
2026-08-25 10:09 ` [PATCH v2 1/6] target_fileio_read_stralloc: add an optional length parameter Matthieu Longo
2026-08-25 10:09 ` [PATCH v2 2/6] gdb support: add gdb::ranges::replace algorithm Matthieu Longo
@ 2026-08-25 10:09 ` Matthieu Longo
2026-09-10 15:35 ` Andrew Burgess
2026-08-25 10:09 ` [PATCH v2 4/6] gdb/linux-tdep: migrate linux_info_proc to file_reader_t Matthieu Longo
` (4 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Matthieu Longo @ 2026-08-25 10:09 UTC (permalink / raw)
To: gdb-patches
Cc: Simon Marchi, Thiago Jung Bauermann, Luis Machado, Luis Machado,
Christina Joos, Kevin Buettner, Matthieu Longo
Wrap all the boilerplate code required to read a file in a new helper
class: file_reader_t. The class owns the file contents together with
the file path, and provides convenient accessors for the data, size and
typed views. It supports both null-terminated text files and binary files.
This helper eliminates repeated calls to target_fileio_read_stralloc
and target_fileio_read_alloc, remove explicit memory management with
gdb::unique_xmalloc_ptr, and simplifies the casting logic when working
with binary data.
The patch converts some of the existing Linux, AMD64, and SPARC code that
reads files from /proc to use file_reader_t. As a side effect,
amd64_linux_lam_untag_mask and linux_process_address_in_memtag_page
may now return earlier in case the file is empty.
Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Reviewed-By: Christina Joos <christina.joos@intel.com>
---
gdb/amd64-linux-tdep.c | 12 ++--
gdb/linux-tdep.c | 122 ++++++++++++++++++-----------------------
gdb/sparc64-tdep.c | 15 +++--
gdb/target.h | 78 ++++++++++++++++++++++++++
4 files changed, 143 insertions(+), 84 deletions(-)
diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index 9b23db72bbe..52f16c953d1 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -1848,14 +1848,11 @@ amd64_linux_lam_untag_mask ()
if (inf->fake_pid_p)
return DEFAULT_TAG_MASK;
- const std::string filename = string_printf ("/proc/%d/status", inf->pid);
- gdb::unique_xmalloc_ptr<char> status_file
- = target_fileio_read_stralloc (nullptr, filename.c_str ());
-
- if (status_file == nullptr)
+ file_reader_t<char> proc_status (string_printf ("/proc/%d/status", inf->pid));
+ if (!proc_status)
return DEFAULT_TAG_MASK;
- std::string_view status_file_view (status_file.get ());
+ std::string_view status_file_view (proc_status.data ());
constexpr std::string_view untag_mask_str = "untag_mask:\t";
const size_t found = status_file_view.find (untag_mask_str);
if (found != std::string::npos)
@@ -1867,7 +1864,8 @@ amd64_linux_lam_untag_mask ()
unsigned long long result = std::strtoul (start, &endptr, 0);
if (errno != 0 || endptr == start)
error (_("Failed to parse untag_mask from file %ps."),
- styled_string (file_name_style.style (), filename.c_str ()));
+ styled_string (file_name_style.style (),
+ proc_status.c_filepath ()));
return result;
}
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 588984a1ca4..84614bc91a0 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1551,7 +1551,7 @@ parse_smaps_key_value (const char *keyword, const char *line,
DATA is the contents of the smaps file. The parsed contents are stored
into the SMAPS vector. */
-static std::vector<struct smaps_data>
+static std::vector<smaps_data>
parse_smaps_data (const char *data,
const std::string &maps_filename)
{
@@ -1561,7 +1561,7 @@ parse_smaps_data (const char *data,
line = strtok_r ((char *) data, "\n", &t);
- std::vector<struct smaps_data> smaps;
+ std::vector<smaps_data> smaps;
while (line != NULL)
{
@@ -1673,7 +1673,7 @@ parse_smaps_data (const char *data,
}
}
/* Save the smaps entry to the vector. */
- struct smaps_data map;
+ smaps_data map;
map.start_address = m.addr;
map.end_address = m.endaddr;
@@ -1697,6 +1697,12 @@ 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. */
@@ -1708,17 +1714,13 @@ linux_process_address_in_memtag_page (CORE_ADDR address)
ptid_t ptid = get_process_reference_ptid ();
- std::string smaps_file = string_printf ("/proc/%ld/smaps", ptid.lwp ());
-
- gdb::unique_xmalloc_ptr<char> data
- = target_fileio_read_stralloc (NULL, smaps_file.c_str ());
-
- if (data == nullptr)
+ file_reader_t<char> smaps_freader
+ (string_printf ("/proc/%ld/smaps", ptid.lwp ()));
+ if (!smaps_freader)
return false;
/* Parse the contents of smaps into a vector. */
- std::vector<struct smaps_data> smaps
- = parse_smaps_data (data.get (), smaps_file);
+ std::vector<smaps_data> smaps = parse_smaps_data (smaps_freader);
for (const smaps_data &map : smaps)
{
@@ -1782,17 +1784,13 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
if (use_coredump_filter)
{
- std::string core_dump_filter_name
- = string_printf ("/proc/%ld/coredump_filter", ptid.lwp ());
-
- gdb::unique_xmalloc_ptr<char> coredumpfilterdata
- = target_fileio_read_stralloc (NULL, core_dump_filter_name.c_str ());
-
- if (coredumpfilterdata != NULL)
+ file_reader_t<char> coredump_filter_freader
+ (string_printf ("/proc/%ld/coredump_filter", ptid.lwp ()));
+ if (coredump_filter_freader)
{
unsigned int flags;
- sscanf (coredumpfilterdata.get (), "%x", &flags);
+ sscanf (coredump_filter_freader.data (), "%x", &flags);
filterflags = (enum filter_flag) flags;
}
}
@@ -1813,10 +1811,9 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
}
/* Parse the contents of smaps into a vector. */
- std::vector<struct smaps_data> smaps
- = parse_smaps_data (data.get (), maps_filename);
+ std::vector<smaps_data> smaps = parse_smaps_data (data.get (), maps_filename);
- for (const struct smaps_data &map : smaps)
+ for (const smaps_data &map: smaps)
{
/* Invoke the callback function to create the corefile segment. */
if (should_dump_mapping_p (filterflags, map))
@@ -2295,9 +2292,6 @@ linux_corefile_parse_exec_context (struct gdbarch *gdbarch, bfd *cbfd)
static bool
linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
{
- /* The filename which we will use to obtain some info about the process.
- We will basically use this to store the `/proc/PID/FILENAME' file. */
- char filename[100];
/* The basename of the executable. */
const char *basename;
/* Temporary buffer. */
@@ -2319,24 +2313,27 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
gdb_assert (p != nullptr);
- /* Obtaining PID and filename. */
- xsnprintf (filename, sizeof (filename), "/proc/%ld/cmdline", ptid.lwp ());
- /* The full name of the program which generated the corefile. */
- gdb_byte *buf = nullptr;
- LONGEST buf_len = target_fileio_read_alloc (nullptr, filename, &buf);
- gdb::unique_xmalloc_ptr<char> fname ((char *)buf);
+ file_reader_t<gdb_byte> cmdline_freader
+ (string_printf ("/proc/%ld/cmdline", ptid.lwp ()));
+ if (!cmdline_freader)
+ return false;
- if (buf_len < 1 || fname.get () == nullptr || fname.get ()[0] == '\0')
+ /* /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> ();
+ /* The buffer points to the full name of the program which generated the
+ corefile. */
+ if (cmdline.size () < 1 || cmdline[0] == '\0')
{
/* No program name was read, so we won't be able to retrieve more
information about the process. */
return false;
}
- if (fname.get ()[buf_len - 1] != '\0')
+ if (cmdline[cmdline.size () - 1] != '\0')
{
warning (_("target file %s "
"does not contain a trailing null character"),
- filename);
+ cmdline_freader.c_filepath ());
return false;
}
@@ -2346,27 +2343,24 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
p->pr_pid = ptid.pid ();
/* Copying the program name. Only the basename matters. */
- basename = lbasename (fname.get ());
+ basename = lbasename (cmdline.data ());
strncpy (p->pr_fname, basename, sizeof (p->pr_fname) - 1);
p->pr_fname[sizeof (p->pr_fname) - 1] = '\0';
const std::string &infargs = current_inferior ()->args ();
/* The arguments of the program. */
- std::string psargs = fname.get ();
+ std::string psargs = cmdline.data ();
if (!infargs.empty ())
psargs += ' ' + infargs;
strncpy (p->pr_psargs, psargs.c_str (), sizeof (p->pr_psargs) - 1);
p->pr_psargs[sizeof (p->pr_psargs) - 1] = '\0';
- xsnprintf (filename, sizeof (filename), "/proc/%ld/stat", ptid.lwp ());
- /* The contents of `/proc/PID/stat'. */
- gdb::unique_xmalloc_ptr<char> proc_stat_contents
- = target_fileio_read_stralloc (NULL, filename);
- char *proc_stat = proc_stat_contents.get ();
-
- if (proc_stat == NULL || *proc_stat == '\0')
+ file_reader_t<char> stat_freader
+ (string_printf ("/proc/%ld/stat", ptid.lwp ()));
+ const char *proc_stat = stat_freader.data ();
+ if (!stat_freader || *proc_stat == '\0')
{
/* Despite being unable to read more information about the
process, we return true here because at least we have its
@@ -2438,13 +2432,10 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
/* Finally, obtaining the UID and GID. For that, we read and parse the
contents of the `/proc/PID/status' file. */
- xsnprintf (filename, sizeof (filename), "/proc/%ld/status", ptid.lwp ());
- /* The contents of `/proc/PID/status'. */
- gdb::unique_xmalloc_ptr<char> proc_status_contents
- = target_fileio_read_stralloc (NULL, filename);
- char *proc_status = proc_status_contents.get ();
-
- if (proc_status == NULL || *proc_status == '\0')
+ file_reader_t<char> status_freader
+ (string_printf ("/proc/%ld/status", ptid.lwp ()));
+ char *proc_status = status_freader.data ();
+ if (!status_freader || *proc_status == '\0')
{
/* Returning true since we already have a bunch of information. */
return true;
@@ -2837,9 +2828,6 @@ linux_gdb_signal_to_target (struct gdbarch *gdbarch,
static bool
linux_vsyscall_range_raw (struct gdbarch *gdbarch, struct mem_range *range)
{
- char filename[100];
- long pid;
-
if (target_auxv_search (AT_SYSINFO_EHDR, &range->start) <= 0)
return false;
@@ -2877,7 +2865,7 @@ linux_vsyscall_range_raw (struct gdbarch *gdbarch, struct mem_range *range)
if (current_inferior ()->fake_pid_p)
return false;
- pid = current_inferior ()->pid;
+ long pid = current_inferior ()->pid;
/* Note that reading /proc/PID/task/PID/maps (1) is much faster than
reading /proc/PID/maps (2). The later identifies thread stacks
@@ -2887,15 +2875,14 @@ linux_vsyscall_range_raw (struct gdbarch *gdbarch, struct mem_range *range)
a few thousand threads, (1) takes a few milliseconds, while (2)
takes several seconds. Also note that "smaps", what we read for
determining core dump mappings, is even slower than "maps". */
- xsnprintf (filename, sizeof filename, "/proc/%ld/task/%ld/maps", pid, pid);
- gdb::unique_xmalloc_ptr<char> data
- = target_fileio_read_stralloc (NULL, filename);
- if (data != NULL)
+ file_reader_t<char> task_maps_freader
+ (string_printf ("/proc/%ld/task/%ld/maps", pid, pid));
+ if (task_maps_freader)
{
char *line;
char *saveptr = NULL;
- for (line = strtok_r (data.get (), "\n", &saveptr);
+ for (line = strtok_r (task_maps_freader.data (), "\n", &saveptr);
line != NULL;
line = strtok_r (NULL, "\n", &saveptr))
{
@@ -2913,8 +2900,9 @@ linux_vsyscall_range_raw (struct gdbarch *gdbarch, struct mem_range *range)
}
}
}
- else
- warning (_("unable to open /proc file '%s'"), filename);
+ else if (task_maps_freader.error ())
+ warning (_("unable to open /proc file '%s'"),
+ task_maps_freader.c_filepath ());
return false;
}
@@ -3241,16 +3229,12 @@ linux_address_in_shadow_stack_mem_range
ptid_t ptid = get_process_reference_ptid ();
- std::string smaps_file = string_printf ("/proc/%ld/smaps", ptid.lwp ());
-
- gdb::unique_xmalloc_ptr<char> data
- = target_fileio_read_stralloc (nullptr, smaps_file.c_str ());
-
- if (data == nullptr)
+ file_reader_t<char> smaps_freader
+ (string_printf ("/proc/%ld/smaps", ptid.lwp ()));
+ if (!smaps_freader)
return false;
- const std::vector<smaps_data> smaps
- = parse_smaps_data (data.get (), smaps_file);
+ const std::vector<smaps_data> smaps = parse_smaps_data (smaps_freader);
auto find_addr_mem_range = [&addr] (const smaps_data &map)
{
diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c
index 93db3417a2a..97f8fa72e52 100644
--- a/gdb/sparc64-tdep.c
+++ b/gdb/sparc64-tdep.c
@@ -301,18 +301,16 @@ adi_tag_fd ()
static bool
adi_is_addr_mapped (CORE_ADDR vaddr, size_t cnt)
{
- char filename[MAX_PROC_NAME_SIZE];
size_t i = 0;
pid_t pid = inferior_ptid.pid ();
- snprintf (filename, sizeof filename, "/proc/%ld/adi/maps", (long) pid);
- gdb::unique_xmalloc_ptr<char> data
- = target_fileio_read_stralloc (NULL, filename);
- if (data)
+ file_reader_t<char> adi_maps_freader
+ (string_printf ("/proc/%d/adi/maps", pid));
+ if (adi_maps_freader)
{
adi_stat_t adi_stat = get_adi_info (pid);
char *saveptr;
- for (char *line = strtok_r (data.get (), "\n", &saveptr);
+ for (char *line = strtok_r (adi_maps_freader.data (), "\n", &saveptr);
line;
line = strtok_r (NULL, "\n", &saveptr))
{
@@ -328,8 +326,9 @@ adi_is_addr_mapped (CORE_ADDR vaddr, size_t cnt)
}
}
}
- else
- warning (_("unable to open /proc file '%s'"), filename);
+ else if (adi_maps_freader.error ())
+ warning (_("unable to open /proc file '%s'"),
+ adi_maps_freader.c_filepath ());
return false;
}
diff --git a/gdb/target.h b/gdb/target.h
index 819279c08fc..017918b6582 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2341,6 +2341,84 @@ extern LONGEST target_fileio_read_alloc (struct inferior *inf,
extern gdb::unique_xmalloc_ptr<char> target_fileio_read_stralloc
(struct inferior *inf, const char *filename, LONGEST *len = nullptr);
+/* Helper class for reading the content of a file on the target. */
+template <typename T>
+class file_reader_t
+{
+ /* The filepath of the file being read. */
+ std::string m_filepath;
+ /* Smart pointer to the data. */
+ gdb::unique_xmalloc_ptr<T> m_data;
+ /* Number of bytes read. */
+ LONGEST m_size;
+
+public:
+ file_reader_t (const std::string &filepath)
+ : m_filepath (filepath)
+ , m_size (0)
+ {
+ if constexpr (std::is_same_v<T, char>)
+ m_data = target_fileio_read_stralloc (nullptr, m_filepath.c_str (),
+ &m_size);
+ else
+ {
+ gdb_byte *buf = nullptr;
+ m_size = target_fileio_read_alloc (nullptr, m_filepath.c_str (), &buf);
+ m_data = gdb::unique_xmalloc_ptr<T> (reinterpret_cast<T *>(buf));
+ }
+ }
+
+ file_reader_t (file_reader_t &&) = default;
+ file_reader_t &operator= (file_reader_t &&) = default;
+
+ DISABLE_COPY_AND_ASSIGN (file_reader_t);
+
+ /* Return true if the file was read successfully but contained no data. */
+ bool empty () const noexcept
+ { return m_data != nullptr && m_size == 0; }
+
+ /* Return true if the file could not be read. */
+ bool error () const noexcept
+ { return m_data == nullptr || m_size < 0; }
+
+ /* Return true if the file was read successfully and is non-empty. */
+ explicit operator bool () const noexcept
+ { return !(error () || empty ()); }
+
+ /* Return a pointer to the data. */
+ T *data () const noexcept
+ { return m_data.get (); }
+
+ /* Return the number of bytes read. */
+ LONGEST size () const noexcept
+ {
+ /* For char buffers, size() corresponds to the size of the read data. Some
+ null-terminator characters are possibly scattered throughout the data.
+ Consequently, strlen() might not reflect the actual size. */
+ return m_size;
+ }
+
+ /* Return a span of the data. */
+ gdb::array_view<T> view () const noexcept
+ { return gdb::array_view<T> (m_data.get (), size ()); }
+
+ /* Return a span of the data, reinterpreted as U objects. */
+ template <typename U>
+ gdb::array_view<U> cast_view () const noexcept
+ {
+ return gdb::array_view<U> (reinterpret_cast<U *> (m_data.get ()),
+ size () * sizeof (T) / sizeof (U));
+ }
+
+ /* Return the path of the file that was read. */
+ const std::string &filepath () const noexcept
+ { return m_filepath; }
+
+ /* Return the path of the file that was read as a C string. */
+ const char *c_filepath () const noexcept
+ { return m_filepath.c_str (); }
+};
+
/* Invalidate the target associated with open handles that were open
on target TARG, since we're about to close (and maybe destroy) the
target. The handles remain open from the client's perspective, but
--
2.55.0
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 3/6] gdb: introduce helper class file_reader_t
2026-08-25 10:09 ` [PATCH v2 3/6] gdb: introduce helper class file_reader_t Matthieu Longo
@ 2026-09-10 15:35 ` Andrew Burgess
0 siblings, 0 replies; 19+ messages in thread
From: Andrew Burgess @ 2026-09-10 15:35 UTC (permalink / raw)
To: Matthieu Longo, gdb-patches
Cc: Simon Marchi, Thiago Jung Bauermann, Luis Machado, Luis Machado,
Christina Joos, Kevin Buettner, Matthieu Longo
Matthieu Longo <matthieu.longo@arm.com> writes:
> Wrap all the boilerplate code required to read a file in a new helper
> class: file_reader_t. The class owns the file contents together with
> the file path, and provides convenient accessors for the data, size and
> typed views. It supports both null-terminated text files and binary files.
>
> This helper eliminates repeated calls to target_fileio_read_stralloc
> and target_fileio_read_alloc, remove explicit memory management with
s/remove/removes/
> gdb::unique_xmalloc_ptr, and simplifies the casting logic when working
> with binary data.
>
> The patch converts some of the existing Linux, AMD64, and SPARC code that
> reads files from /proc to use file_reader_t. As a side effect,
> amd64_linux_lam_untag_mask and linux_process_address_in_memtag_page
> may now return earlier in case the file is empty.
>
> Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Reviewed-By: Christina Joos <christina.joos@intel.com>
> ---
> gdb/amd64-linux-tdep.c | 12 ++--
> gdb/linux-tdep.c | 122 ++++++++++++++++++-----------------------
> gdb/sparc64-tdep.c | 15 +++--
> gdb/target.h | 78 ++++++++++++++++++++++++++
> 4 files changed, 143 insertions(+), 84 deletions(-)
>
> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index 9b23db72bbe..52f16c953d1 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -1848,14 +1848,11 @@ amd64_linux_lam_untag_mask ()
> if (inf->fake_pid_p)
> return DEFAULT_TAG_MASK;
>
> - const std::string filename = string_printf ("/proc/%d/status", inf->pid);
> - gdb::unique_xmalloc_ptr<char> status_file
> - = target_fileio_read_stralloc (nullptr, filename.c_str ());
> -
> - if (status_file == nullptr)
> + file_reader_t<char> proc_status (string_printf ("/proc/%d/status", inf->pid));
> + if (!proc_status)
> return DEFAULT_TAG_MASK;
>
> - std::string_view status_file_view (status_file.get ());
> + std::string_view status_file_view (proc_status.data ());
> constexpr std::string_view untag_mask_str = "untag_mask:\t";
> const size_t found = status_file_view.find (untag_mask_str);
> if (found != std::string::npos)
> @@ -1867,7 +1864,8 @@ amd64_linux_lam_untag_mask ()
> unsigned long long result = std::strtoul (start, &endptr, 0);
> if (errno != 0 || endptr == start)
> error (_("Failed to parse untag_mask from file %ps."),
> - styled_string (file_name_style.style (), filename.c_str ()));
> + styled_string (file_name_style.style (),
> + proc_status.c_filepath ()));
>
> return result;
> }
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index 588984a1ca4..84614bc91a0 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -1551,7 +1551,7 @@ parse_smaps_key_value (const char *keyword, const char *line,
> DATA is the contents of the smaps file. The parsed contents are stored
> into the SMAPS vector. */
>
> -static std::vector<struct smaps_data>
> +static std::vector<smaps_data>
Throughout this patch there's a bunch of places where you've done
nothing but delete the 'struct' prefix. It's OK to do this in code that
you're touching anyway as part of this patch, but any, like this, that
are in code that you'd not otherwise touch, are unrelated changes and
should be moved into a separate patch.
I think you should either drop these, or have a first patch which does a
"remove some struct prefixes" cleanup, your choice.
> @@ -2346,27 +2343,24 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
> p->pr_pid = ptid.pid ();
>
> /* Copying the program name. Only the basename matters. */
> - basename = lbasename (fname.get ());
> + basename = lbasename (cmdline.data ());
> strncpy (p->pr_fname, basename, sizeof (p->pr_fname) - 1);
> p->pr_fname[sizeof (p->pr_fname) - 1] = '\0';
>
> const std::string &infargs = current_inferior ()->args ();
>
> /* The arguments of the program. */
> - std::string psargs = fname.get ();
> + std::string psargs = cmdline.data ();
> if (!infargs.empty ())
> psargs += ' ' + infargs;
>
> strncpy (p->pr_psargs, psargs.c_str (), sizeof (p->pr_psargs) - 1);
> p->pr_psargs[sizeof (p->pr_psargs) - 1] = '\0';
>
> - xsnprintf (filename, sizeof (filename), "/proc/%ld/stat", ptid.lwp ());
> - /* The contents of `/proc/PID/stat'. */
> - gdb::unique_xmalloc_ptr<char> proc_stat_contents
> - = target_fileio_read_stralloc (NULL, filename);
> - char *proc_stat = proc_stat_contents.get ();
> -
> - if (proc_stat == NULL || *proc_stat == '\0')
> + file_reader_t<char> stat_freader
> + (string_printf ("/proc/%ld/stat", ptid.lwp ()));
> + const char *proc_stat = stat_freader.data ();
> + if (!stat_freader || *proc_stat == '\0')
We access the data here before checking if the read was successful.
This works fine, but doesn't seem ideal. Later on I suggest that maybe
file_reader_t::data should assert that we're no in the error state, and
this is what I was looking at when I started thinking about that.
If you really think we should support reading data when in an error
state, then the data method should document what the return value is
when the file_reader_t is in the error state.
> diff --git a/gdb/target.h b/gdb/target.h
> index 819279c08fc..017918b6582 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -2341,6 +2341,84 @@ extern LONGEST target_fileio_read_alloc (struct inferior *inf,
> extern gdb::unique_xmalloc_ptr<char> target_fileio_read_stralloc
> (struct inferior *inf, const char *filename, LONGEST *len = nullptr);
>
> +/* Helper class for reading the content of a file on the target. */
> +template <typename T>
> +class file_reader_t
I'm pretty sure that types ending with _t are reserved by ... some
spec. We should avoid this and ideally, pick a name that better
describes what the class does, e.g. target_file_reader.
> +{
> + /* The filepath of the file being read. */
> + std::string m_filepath;
The ship has mostly sailed already, but "path" should be used for lists
of locations, like the $PATH variable. It would be better to just use
filename, m_filename, etc. I am fully aware that 'path' is used
throughout GDB in place of filename, but we might as well avoid adding
another here.
This should be fixed throughout this class.
> + /* Smart pointer to the data. */
> + gdb::unique_xmalloc_ptr<T> m_data;
> + /* Number of bytes read. */
> + LONGEST m_size;
GDB style usually puts a space between member variables, e.g.:
/* The filepath of the file being read. */
std::string m_filepath;
/* Smart pointer to the data. */
gdb::unique_xmalloc_ptr<T> m_data;
/* Number of bytes read. */
LONGEST m_size;
> +
> +public:
> + file_reader_t (const std::string &filepath)
> + : m_filepath (filepath)
> + , m_size (0)
> + {
> + if constexpr (std::is_same_v<T, char>)
> + m_data = target_fileio_read_stralloc (nullptr, m_filepath.c_str (),
> + &m_size);
> + else
> + {
> + gdb_byte *buf = nullptr;
> + m_size = target_fileio_read_alloc (nullptr, m_filepath.c_str (), &buf);
> + m_data = gdb::unique_xmalloc_ptr<T> (reinterpret_cast<T *>(buf));
> + }
> + }
There's a bug hiding in here when T is not 'char'. If the file being
read is empty then target_fileio_read_alloc returns 0 but leaves *BUF
unchanged, i.e. as nullptr.
Given that, despite successfully reading the empty file, empty() will
return false and error() will return true.
Also, given this is being written as a general helper class, it might be
a good idea to define how the inferior is passed in, rather than leaving
that for future users to do.
> +
> + file_reader_t (file_reader_t &&) = default;
> + file_reader_t &operator= (file_reader_t &&) = default;
> +
> + DISABLE_COPY_AND_ASSIGN (file_reader_t);
> +
> + /* Return true if the file was read successfully but contained no data. */
> + bool empty () const noexcept
> + { return m_data != nullptr && m_size == 0; }
> +
> + /* Return true if the file could not be read. */
> + bool error () const noexcept
> + { return m_data == nullptr || m_size < 0; }
> +
> + /* Return true if the file was read successfully and is non-empty. */
> + explicit operator bool () const noexcept
> + { return !(error () || empty ()); }
I'm really not a fan of this API. Consider this code from earlier in
this patch:
file_reader_t<char> proc_status (string_printf ("/proc/%d/status", inf->pid));
if (!proc_status)
return DEFAULT_TAG_MASK;
I don't think it's obvious that !proc_status means error or empty. I
think a much less error prone API would be to just add a new member
function:
bool empty_or_error () const noexcept
{ return this->empty () || this->error (); }
And then use that. It's more typing for sure, but it's also crystal
clear what's going on.
> +
> + /* Return a pointer to the data. */
> + T *data () const noexcept
> + { return m_data.get (); }
Might be a good idea to assert that we're not in the error state.
> +
> + /* Return the number of bytes read. */
> + LONGEST size () const noexcept
> + {
> + /* For char buffers, size() corresponds to the size of the read data. Some
> + null-terminator characters are possibly scattered throughout the data.
> + Consequently, strlen() might not reflect the actual size. */
> + return m_size;
> + }
Again, maybe assert that we're not in the error state. I think there's
only one user of this right now, and it already checks for errors before
calling size.
> +
> + /* Return a span of the data. */
> + gdb::array_view<T> view () const noexcept
> + { return gdb::array_view<T> (m_data.get (), size ()); }
> +
> + /* Return a span of the data, reinterpreted as U objects. */
> + template <typename U>
> + gdb::array_view<U> cast_view () const noexcept
> + {
> + return gdb::array_view<U> (reinterpret_cast<U *> (m_data.get ()),
> + size () * sizeof (T) / sizeof (U));
> + }
It would be a good idea to say in the comment what happens if the file
size is not a multiple of 'sizeof (U)'. Or do we even want to support
this case?
Thanks,
Andrew
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 4/6] gdb/linux-tdep: migrate linux_info_proc to file_reader_t
2026-08-25 10:09 [PATCH v2 0/6] gdb: introduce file_reader_t to read procfs files Matthieu Longo
` (2 preceding siblings ...)
2026-08-25 10:09 ` [PATCH v2 3/6] gdb: introduce helper class file_reader_t Matthieu Longo
@ 2026-08-25 10:09 ` Matthieu Longo
2026-09-10 16:31 ` Andrew Burgess
2026-08-25 10:09 ` [PATCH v2 5/6] gdb/linux-tdep: migrate linux_find_memory_regions_full " Matthieu Longo
` (3 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Matthieu Longo @ 2026-08-25 10:09 UTC (permalink / raw)
To: gdb-patches
Cc: Simon Marchi, Thiago Jung Bauermann, Luis Machado, Luis Machado,
Christina Joos, Kevin Buettner, Matthieu Longo
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,
+ gdb::array_view<char>::iterator start,
+ char separator = '\0')
+{
+ auto next_start = buffer.end ();
+
+ /* Reject a START iterator that does not point into BUFFER. */
+ if (start < buffer.begin () || start >= buffer.end ())
+ return {gdb::array_view<char> (), next_start};
+
+ auto it = std::find (start, buffer.end (), separator);
+
+ /* If no separator is found, the remainder of BUFFER is the final string. */
+ if (it != buffer.end ())
+ {
+ /* Otherwise, skip successive separators so that NEXT_START points to
+ the beginning of the next string, if any. */
+ for (next_start = std::next (it);
+ next_start != buffer.end () && *next_start == separator;
+ next_start = std::next (next_start));
+ }
+
+ return {gdb::array_view<char> (start, it), next_start};
+}
+
/* Implement the "info proc" command. */
static void
@@ -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');
+ /* Replace null characters splitting the arguments in the command
+ line by spaces, except for the last one. */
+ gdb::ranges::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)
{
@@ -946,27 +977,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)
{
@@ -980,10 +1009,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");
@@ -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';
+ struct mapping m = read_mapping (line.data ());
ui_out_emit_tuple tuple_emitter (current_uiout);
current_uiout->field_core_addr ("start", gdbarch, m.addr);
@@ -1021,26 +1053,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)));
@@ -1167,7 +1199,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 ());
}
}
--
2.55.0
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 4/6] gdb/linux-tdep: migrate linux_info_proc to file_reader_t
2026-08-25 10:09 ` [PATCH v2 4/6] gdb/linux-tdep: migrate linux_info_proc to file_reader_t Matthieu Longo
@ 2026-09-10 16:31 ` Andrew Burgess
0 siblings, 0 replies; 19+ messages in thread
From: Andrew Burgess @ 2026-09-10 16:31 UTC (permalink / raw)
To: Matthieu Longo, gdb-patches
Cc: Simon Marchi, Thiago Jung Bauermann, Luis Machado, Luis Machado,
Christina Joos, Kevin Buettner, Matthieu Longo
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 5/6] gdb/linux-tdep: migrate linux_find_memory_regions_full to file_reader_t
2026-08-25 10:09 [PATCH v2 0/6] gdb: introduce file_reader_t to read procfs files Matthieu Longo
` (3 preceding siblings ...)
2026-08-25 10:09 ` [PATCH v2 4/6] gdb/linux-tdep: migrate linux_info_proc to file_reader_t Matthieu Longo
@ 2026-08-25 10:09 ` Matthieu Longo
2026-09-11 8:34 ` Andrew Burgess
2026-08-25 10:09 ` [PATCH v2 6/6] gdb/linux-tdep: remove legacy parse_smaps_data overload Matthieu Longo
` (2 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Matthieu Longo @ 2026-08-25 10:09 UTC (permalink / raw)
To: gdb-patches
Cc: Simon Marchi, Thiago Jung Bauermann, Luis Machado, Luis Machado,
Christina Joos, Kevin Buettner, Matthieu Longo
The previous implementation of linux_find_memory_regions_full could
still return success even when none of the /proc/PID/[s]maps files
existed, or all reads returned 0 bytes (this last case can happen on
Linux when the thread-group leader has exited).
As a result, the function could incorrectly succeed, allowing core
dump generation via the gcore command.
This logical defect was allowing, by chance, the function to return
success and hence, allowing fortuitously the coredump generation via
gcore command (see gcore-stale-thread test for more details).
A previous patch in this patch series fixed this issue by using the
first LWP ID of the current inferior instead of relying on the PID.
This patch adapts the code to use file_reader_t and makes the function
return an error if reading any of the procfs files fails.
Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
---
gdb/linux-tdep.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index e2c5b2d8815..2833d8a4bb6 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1828,24 +1828,22 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
}
}
- std::string maps_filename = string_printf ("/proc/%ld/smaps", ptid.lwp ());
-
- gdb::unique_xmalloc_ptr<char> data
- = target_fileio_read_stralloc (NULL, maps_filename.c_str ());
+ std::vector<smaps_data> smaps;
- if (data == NULL)
+ file_reader_t<char> smaps_freader
+ (string_printf ("/proc/%ld/smaps", ptid.lwp ()));
+ if (smaps_freader)
+ smaps = parse_smaps_data (smaps_freader);
+ else
{
/* Older Linux kernels did not support /proc/PID/smaps. */
- maps_filename = string_printf ("/proc/%ld/maps", ptid.lwp ());
- data = target_fileio_read_stralloc (NULL, maps_filename.c_str ());
-
- if (data == nullptr)
+ file_reader_t<char> maps_freader
+ (string_printf ("/proc/%ld/maps", ptid.lwp ()));
+ if (!maps_freader)
return false;
+ smaps = parse_smaps_data (maps_freader);
}
- /* Parse the contents of smaps into a vector. */
- std::vector<smaps_data> smaps = parse_smaps_data (data.get (), maps_filename);
-
for (const smaps_data &map: smaps)
{
/* Invoke the callback function to create the corefile segment. */
--
2.55.0
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 5/6] gdb/linux-tdep: migrate linux_find_memory_regions_full to file_reader_t
2026-08-25 10:09 ` [PATCH v2 5/6] gdb/linux-tdep: migrate linux_find_memory_regions_full " Matthieu Longo
@ 2026-09-11 8:34 ` Andrew Burgess
0 siblings, 0 replies; 19+ messages in thread
From: Andrew Burgess @ 2026-09-11 8:34 UTC (permalink / raw)
To: Matthieu Longo, gdb-patches
Cc: Simon Marchi, Thiago Jung Bauermann, Luis Machado, Luis Machado,
Christina Joos, Kevin Buettner, Matthieu Longo
Matthieu Longo <matthieu.longo@arm.com> writes:
> The previous implementation of linux_find_memory_regions_full could
> still return success even when none of the /proc/PID/[s]maps files
> existed, or all reads returned 0 bytes (this last case can happen on
> Linux when the thread-group leader has exited).
> As a result, the function could incorrectly succeed, allowing core
> dump generation via the gcore command.
> This logical defect was allowing, by chance, the function to return
> success and hence, allowing fortuitously the coredump generation via
> gcore command (see gcore-stale-thread test for more details).
>
> A previous patch in this patch series fixed this issue by using the
> first LWP ID of the current inferior instead of relying on the PID.
> This patch adapts the code to use file_reader_t and makes the function
> return an error if reading any of the procfs files fails.
This commit message seems out of date. As far as I can tell the issue
you are describing here isn't fixed yet.
Further, I took a look at gdb.threads/gcore-stale-thread.exp and
couldn't find any text talking about this issue, so saying "see
gcore-stale-thread test for more details" isn't super helpful.
It doesn't sound like this patch was actually aiming to fix the bug you
described anyway, just to convert to the file_reader_t class, so
honestly, I'd just drop all discussion of the previously fixed bug.
But if this patch is fixing the bug then I'd expect either a new test to
be added, or if gcore-stale-thread.exp does hit this bug, I would have
expected the test to be updated now that the bug is fixed.
Thanks,
Andrew
>
> Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> ---
> gdb/linux-tdep.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index e2c5b2d8815..2833d8a4bb6 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -1828,24 +1828,22 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
> }
> }
>
> - std::string maps_filename = string_printf ("/proc/%ld/smaps", ptid.lwp ());
> -
> - gdb::unique_xmalloc_ptr<char> data
> - = target_fileio_read_stralloc (NULL, maps_filename.c_str ());
> + std::vector<smaps_data> smaps;
>
> - if (data == NULL)
> + file_reader_t<char> smaps_freader
> + (string_printf ("/proc/%ld/smaps", ptid.lwp ()));
> + if (smaps_freader)
> + smaps = parse_smaps_data (smaps_freader);
> + else
> {
> /* Older Linux kernels did not support /proc/PID/smaps. */
> - maps_filename = string_printf ("/proc/%ld/maps", ptid.lwp ());
> - data = target_fileio_read_stralloc (NULL, maps_filename.c_str ());
> -
> - if (data == nullptr)
> + file_reader_t<char> maps_freader
> + (string_printf ("/proc/%ld/maps", ptid.lwp ()));
> + if (!maps_freader)
> return false;
> + smaps = parse_smaps_data (maps_freader);
> }
>
> - /* Parse the contents of smaps into a vector. */
> - std::vector<smaps_data> smaps = parse_smaps_data (data.get (), maps_filename);
> -
> for (const smaps_data &map: smaps)
> {
> /* Invoke the callback function to create the corefile segment. */
> --
> 2.55.0
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 6/6] gdb/linux-tdep: remove legacy parse_smaps_data overload
2026-08-25 10:09 [PATCH v2 0/6] gdb: introduce file_reader_t to read procfs files Matthieu Longo
` (4 preceding siblings ...)
2026-08-25 10:09 ` [PATCH v2 5/6] gdb/linux-tdep: migrate linux_find_memory_regions_full " Matthieu Longo
@ 2026-08-25 10:09 ` Matthieu Longo
2026-09-11 8:41 ` Andrew Burgess
2026-09-03 22:58 ` [PATCH v2 0/6] gdb: introduce file_reader_t to read procfs files Matthieu Longo
2026-09-08 10:27 ` Matthieu Longo
7 siblings, 1 reply; 19+ messages in thread
From: Matthieu Longo @ 2026-08-25 10:09 UTC (permalink / raw)
To: gdb-patches
Cc: Simon Marchi, Thiago Jung Bauermann, Luis Machado, Luis Machado,
Christina Joos, Kevin Buettner, Matthieu Longo
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. */
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. */
--
2.55.0
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 6/6] gdb/linux-tdep: remove legacy parse_smaps_data overload
2026-08-25 10:09 ` [PATCH v2 6/6] gdb/linux-tdep: remove legacy parse_smaps_data overload Matthieu Longo
@ 2026-09-11 8:41 ` Andrew Burgess
2026-09-16 10:55 ` Matthieu Longo
0 siblings, 1 reply; 19+ messages in thread
From: Andrew Burgess @ 2026-09-11 8:41 UTC (permalink / raw)
To: Matthieu Longo, gdb-patches
Cc: Simon Marchi, Thiago Jung Bauermann, Luis Machado, Luis Machado,
Christina Joos, Kevin Buettner, Matthieu Longo
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 6/6] gdb/linux-tdep: remove legacy parse_smaps_data overload
2026-09-11 8:41 ` Andrew Burgess
@ 2026-09-16 10:55 ` Matthieu Longo
0 siblings, 0 replies; 19+ messages in thread
From: Matthieu Longo @ 2026-09-16 10:55 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
Cc: Simon Marchi, Thiago Jung Bauermann, Luis Machado, Luis Machado,
Christina Joos, Kevin Buettner
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/6] gdb: introduce file_reader_t to read procfs files
2026-08-25 10:09 [PATCH v2 0/6] gdb: introduce file_reader_t to read procfs files Matthieu Longo
` (5 preceding siblings ...)
2026-08-25 10:09 ` [PATCH v2 6/6] gdb/linux-tdep: remove legacy parse_smaps_data overload Matthieu Longo
@ 2026-09-03 22:58 ` Matthieu Longo
2026-09-08 10:27 ` Matthieu Longo
7 siblings, 0 replies; 19+ messages in thread
From: Matthieu Longo @ 2026-09-03 22:58 UTC (permalink / raw)
To: gdb-patches
Cc: Simon Marchi, Thiago Jung Bauermann, Luis Machado, Luis Machado,
Christina Joos, Kevin Buettner
On 25/08/2026 11:09, Matthieu Longo wrote:
> Those patches were extracted from a previous patch series [1].
> Patches 1 and 2 are prerequisites to the next patches.
> Patch 3 introduces class file_reader_t, and patches 4, 5, and 5 migrates existing code to use this new class.
>
> Already reviewed but pending on a maintainer's approval: 1, 3, 4, 5
> Pending on review and approval: 2
> Already reviewed and approved: 6
>
> v1: https://inbox.sourceware.org/gdb-patches/20260728151700.253720-1-matthieu.longo@arm.com/
> Changes v1 -> v2:
> - in patch 3/6,
> - amended the commit message as requested by Christina to mention the behavioral change.
> - fix mistake in condition of adi_is_addr_mapped ()
> - made move constructor, and move assignment operator of file_reader_t use =default.
> - disabled copy constructor and copy assignment operator of file_reader_t.
>
> Changes diff against [1]:
> - remove reimplementation of std::replace, and use std::replace for gdb::ranges::replace.
> - addressed comments from Christina Schimpe and Chirstina Joos regarding the interface of class file_reader_t.
> - fix the implementation of extract_string_view_from_buffer after Luis found bugs in the previous implementation.
>
> This patch series depends on [2].
>
> [1]: https://inbox.sourceware.org/gdb-patches/20260707154900.94542-1-matthieu.longo@arm.com/
> [2]: https://inbox.sourceware.org/gdb-patches/20260728143317.245389-1-matthieu.longo@arm.com/
>
> Regards,
> Matthieu
>
>
> Matthieu Longo (6):
> target_fileio_read_stralloc: add an optional length parameter
> gdb support: add gdb::ranges::replace algorithm
> gdb: introduce helper class file_reader_t
> gdb/linux-tdep: migrate linux_info_proc to file_reader_t
> gdb/linux-tdep: migrate linux_find_memory_regions_full to file_reader_t
> gdb/linux-tdep: remove legacy parse_smaps_data overload
>
> gdb/amd64-linux-tdep.c | 12 +-
> gdb/linux-tdep.c | 295 +++++++++++++++++++++-------------------
> gdb/sparc64-tdep.c | 15 +-
> gdb/target.c | 11 +-
> gdb/target.h | 98 +++++++++++--
> gdbsupport/array-view.h | 16 +++
> 6 files changed, 278 insertions(+), 169 deletions(-)
>
Ping
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 0/6] gdb: introduce file_reader_t to read procfs files
2026-08-25 10:09 [PATCH v2 0/6] gdb: introduce file_reader_t to read procfs files Matthieu Longo
` (6 preceding siblings ...)
2026-09-03 22:58 ` [PATCH v2 0/6] gdb: introduce file_reader_t to read procfs files Matthieu Longo
@ 2026-09-08 10:27 ` Matthieu Longo
7 siblings, 0 replies; 19+ messages in thread
From: Matthieu Longo @ 2026-09-08 10:27 UTC (permalink / raw)
To: gdb-patches
Cc: Simon Marchi, Thiago Jung Bauermann, Luis Machado, Luis Machado,
Christina Joos, Kevin Buettner
On 25/08/2026 11:09, Matthieu Longo wrote:
> Those patches were extracted from a previous patch series [1].
> Patches 1 and 2 are prerequisites to the next patches.
> Patch 3 introduces class file_reader_t, and patches 4, 5, and 5 migrates existing code to use this new class.
>
> Already reviewed but pending on a maintainer's approval: 1, 3, 4, 5
> Pending on review and approval: 2
> Already reviewed and approved: 6
>
> v1: https://inbox.sourceware.org/gdb-patches/20260728151700.253720-1-matthieu.longo@arm.com/
> Changes v1 -> v2:
> - in patch 3/6,
> - amended the commit message as requested by Christina to mention the behavioral change.
> - fix mistake in condition of adi_is_addr_mapped ()
> - made move constructor, and move assignment operator of file_reader_t use =default.
> - disabled copy constructor and copy assignment operator of file_reader_t.
>
> Changes diff against [1]:
> - remove reimplementation of std::replace, and use std::replace for gdb::ranges::replace.
> - addressed comments from Christina Schimpe and Chirstina Joos regarding the interface of class file_reader_t.
> - fix the implementation of extract_string_view_from_buffer after Luis found bugs in the previous implementation.
>
> This patch series depends on [2].
>
> [1]: https://inbox.sourceware.org/gdb-patches/20260707154900.94542-1-matthieu.longo@arm.com/
> [2]: https://inbox.sourceware.org/gdb-patches/20260728143317.245389-1-matthieu.longo@arm.com/
>
> Regards,
> Matthieu
>
>
> Matthieu Longo (6):
> target_fileio_read_stralloc: add an optional length parameter
> gdb support: add gdb::ranges::replace algorithm
> gdb: introduce helper class file_reader_t
> gdb/linux-tdep: migrate linux_info_proc to file_reader_t
> gdb/linux-tdep: migrate linux_find_memory_regions_full to file_reader_t
> gdb/linux-tdep: remove legacy parse_smaps_data overload
>
> gdb/amd64-linux-tdep.c | 12 +-
> gdb/linux-tdep.c | 295 +++++++++++++++++++++-------------------
> gdb/sparc64-tdep.c | 15 +-
> gdb/target.c | 11 +-
> gdb/target.h | 98 +++++++++++--
> gdbsupport/array-view.h | 16 +++
> 6 files changed, 278 insertions(+), 169 deletions(-)
>
Correction, this patch series depends on
https://inbox.sourceware.org/gdb-patches/20260824162155.467233-1-matthieu.longo@arm.com/
[2] was the old revision, I forgot to update the link.
Matthieu
^ permalink raw reply [flat|nested] 19+ messages in thread