Re: [PATCH v1 03/10] target_fileio_read_stralloc: add an optional length parameter

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

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

> On 24/07/2026 03:50, Thiago Jung Bauermann wrote:
>> Matthieu Longo <matthieu.longo@arm.com> writes:
>> 
>>> On 09/07/2026 07:30, Thiago Jung Bauermann wrote:
>>>> Matthieu Longo <matthieu.longo@arm.com> writes:
>>>>
>>>>> diff --git a/gdb/target.h b/gdb/target.h
>>>>> index 22653138491..4215553033c 100644
>>>>> --- a/gdb/target.h
>>>>> +++ b/gdb/target.h
>>>>> @@ -2336,7 +2336,7 @@ extern LONGEST target_fileio_read_alloc (struct inferior *inf,
>>>>>     are returned as allocated but empty strings.  A warning is issued
>>>>>     if the result contains any 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, size_t *len = nullptr);
>>>>
>>>> It's worth updating the documentation comment to mention the new parameter.
>>>
>>> See the updated diff in gdb/target.h
>>>
>>> diff --git a/gdb/target.h b/gdb/target.h
>>> index 22653138491..0df5a654f75 100644
>>> --- a/gdb/target.h
>>> +++ b/gdb/target.h
>>> @@ -2328,15 +2328,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.  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, size_t *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
>> 
>> Looks great, thanks!
>> 
>> My only suggestion is to take the opportunity to do s/NULL/nullptr/ in
>> the comment.
>> 
>
> I don't remember having seen usages of nullptr inside the documentation comments.
> Have you a previous example for this ?

Yes, there are a number of examples:

gdb/block.h:

/* Return the compunit over whose static or global block the iterator currently
   iterates.  Return nullptr if the iteration is finished.  */
  struct compunit_symtab *compunit_symtab () const;

gdb/breakpoint.h:

  /* Reevaluate a breakpoint.  This is necessary after symbols change
     (e.g., an executable or DSO was loaded, or the inferior just
     started).

     If not nullptr, then FILTER_PSPACE is the program space in which
     symbols may have changed, we only need to add new locations in
     FILTER_PSPACE.

     If FILTER_PSPACE is nullptr then all program spaces may have changed,
     new locations need to be searched for in every program space.

     This is pure virtual as, at a minimum, each sub-class must recompute
     any cached condition expressions based off of the cond_string member
     variable.  */
  virtual void re_set (program_space *filter_pspace) = 0;

gdb/gdbcore.h:

  /* Constructor.  BUILD_ID is not nullptr, and is the build-id for the
     mapped file.  FILENAME is the location of the file that GDB loaded to
     provide the mapped file.  This might be different from the name of the
     mapped file mentioned in the core file, e.g. if GDB downloads a file
     from debuginfod then FILENAME would point into the debuginfod client
     cache.  The FILENAME can be the empty string if GDB was unable to find
     a file to provide the mapped file.  */

  core_target_mapped_file_info (const bfd_build_id *build_id,
				const std::string filename)

gdb/language.h:

  /* Set the default boolean type to be TYPE.  If NAME is not nullptr then
     before using TYPE a symbol called NAME will be looked up, and the type
     of this symbol will be used instead.  Should only be called once when
     performing setup for a particular language in combination with a
     particular gdbarch.  */
  void set_bool_type (struct type *type, const char *name = nullptr)

And others as well.

-- 
Thiago
(he/him)