diff mbox series

[v2] gdbserver: Check r_version < 1 for Linux debugger interface

Message ID YRrK+nOJmShP932/@gmail.com
State Not Applicable
Headers show
Series [v2] gdbserver: Check r_version < 1 for Linux debugger interface | expand

Checks

Context Check Description
dj/TryBot-apply_patch fail Patch failed to apply to master at the time it was sent
dj/TryBot-32bit fail Patch series failed to apply

Commit Message

H.J. Lu Aug. 16, 2021, 8:30 p.m. UTC
Update gdbserver to check r_version < 1 instead of r_version != 1 so
that r_version can be bumped for a new field in the glibc debugger
interface to support multiple namespaces.  Since so far, the gdbserver
only reads fields defined for r_version == 1, it is compatible with
r_version >= 1.

All future glibc debugger interface changes will be backward compatible.
If there is ever the need for backward incompatible change to the glibc
debugger interface, a new DT_XXX element will be provided to access the
new incompatible interface.

	PR gdb/11839
	* linux-low.cc (linux_process_target::qxfer_libraries_svr4):
	Check r_version < 1 instead of r_version != 1.
---
 gdbserver/linux-low.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

H.J. Lu Aug. 16, 2021, 10:02 p.m. UTC | #1
On Mon, Aug 16, 2021 at 1:30 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> Update gdbserver to check r_version < 1 instead of r_version != 1 so
> that r_version can be bumped for a new field in the glibc debugger
> interface to support multiple namespaces.  Since so far, the gdbserver
> only reads fields defined for r_version == 1, it is compatible with
> r_version >= 1.
>
> All future glibc debugger interface changes will be backward compatible.
> If there is ever the need for backward incompatible change to the glibc
> debugger interface, a new DT_XXX element will be provided to access the
> new incompatible interface.
>
>         PR gdb/11839
>         * linux-low.cc (linux_process_target::qxfer_libraries_svr4):
>         Check r_version < 1 instead of r_version != 1.
> ---
>  gdbserver/linux-low.cc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index 5c6191d941c..fc7a995351d 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -6845,7 +6845,7 @@ linux_process_target::qxfer_libraries_svr4 (const char *annex,
>           if (linux_read_memory (priv->r_debug + lmo->r_version_offset,
>                                  (unsigned char *) &r_version,
>                                  sizeof (r_version)) != 0
> -             || r_version != 1)
> +             || r_version < 1)
>             {
>               warning ("unexpected r_debug version %d", r_version);
>             }
> --
> 2.31.1
>

Set r_version == 2 breaks GDB due to

static CORE_ADDR
solib_svr4_r_ldsomap (struct svr4_info *info)
{
  struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
  struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
  enum bfd_endian byte_order = type_byte_order (ptr_type);
  ULONGEST version = 0;

  try
    {
      /* Check version, and return zero if `struct r_debug' doesn't have
         the r_ldsomap member.  */
      version
        = read_memory_unsigned_integer (info->debug_base +
lmo->r_version_offset,
                                        lmo->r_version_size, byte_order);
    }
  catch (const gdb_exception_error &ex)
    {
      exception_print (gdb_stderr, ex);
    }

  if (version < 2 || lmo->r_ldsomap_offset == -1)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

glibc doesn't have r_ldsomap.  But r_ldsomap_offset is set
unconditionally.   Shouldn't it be set only if the target debugger
interface has it?

    return 0;

  return read_memory_typed_address (info->debug_base + lmo->r_ldsomap_offset,
                                    ptr_type);
}
H.J. Lu Aug. 16, 2021, 10:08 p.m. UTC | #2
On Mon, Aug 16, 2021 at 3:02 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Mon, Aug 16, 2021 at 1:30 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > Update gdbserver to check r_version < 1 instead of r_version != 1 so
> > that r_version can be bumped for a new field in the glibc debugger
> > interface to support multiple namespaces.  Since so far, the gdbserver
> > only reads fields defined for r_version == 1, it is compatible with
> > r_version >= 1.
> >
> > All future glibc debugger interface changes will be backward compatible.
> > If there is ever the need for backward incompatible change to the glibc
> > debugger interface, a new DT_XXX element will be provided to access the
> > new incompatible interface.
> >
> >         PR gdb/11839
> >         * linux-low.cc (linux_process_target::qxfer_libraries_svr4):
> >         Check r_version < 1 instead of r_version != 1.
> > ---
> >  gdbserver/linux-low.cc | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> > index 5c6191d941c..fc7a995351d 100644
> > --- a/gdbserver/linux-low.cc
> > +++ b/gdbserver/linux-low.cc
> > @@ -6845,7 +6845,7 @@ linux_process_target::qxfer_libraries_svr4 (const char *annex,
> >           if (linux_read_memory (priv->r_debug + lmo->r_version_offset,
> >                                  (unsigned char *) &r_version,
> >                                  sizeof (r_version)) != 0
> > -             || r_version != 1)
> > +             || r_version < 1)
> >             {
> >               warning ("unexpected r_debug version %d", r_version);
> >             }
> > --
> > 2.31.1
> >
>
> Set r_version == 2 breaks GDB due to
>
> static CORE_ADDR
> solib_svr4_r_ldsomap (struct svr4_info *info)
> {
>   struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
>   struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
>   enum bfd_endian byte_order = type_byte_order (ptr_type);
>   ULONGEST version = 0;
>
>   try
>     {
>       /* Check version, and return zero if `struct r_debug' doesn't have
>          the r_ldsomap member.  */
>       version
>         = read_memory_unsigned_integer (info->debug_base +
> lmo->r_version_offset,
>                                         lmo->r_version_size, byte_order);
>     }
>   catch (const gdb_exception_error &ex)
>     {
>       exception_print (gdb_stderr, ex);
>     }
>
>   if (version < 2 || lmo->r_ldsomap_offset == -1)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> glibc doesn't have r_ldsomap.  But r_ldsomap_offset is set
> unconditionally.   Shouldn't it be set only if the target debugger
> interface has it?
>
>     return 0;
>
>   return read_memory_typed_address (info->debug_base + lmo->r_ldsomap_offset,
>                                     ptr_type);
> }
>

I opened:

https://sourceware.org/bugzilla/show_bug.cgi?id=28236
Simon Marchi Aug. 17, 2021, 1:13 p.m. UTC | #3
On 2021-08-16 4:30 p.m., H.J. Lu wrote:
> Update gdbserver to check r_version < 1 instead of r_version != 1 so
> that r_version can be bumped for a new field in the glibc debugger
> interface to support multiple namespaces.  Since so far, the gdbserver
> only reads fields defined for r_version == 1, it is compatible with
> r_version >= 1.
> 
> All future glibc debugger interface changes will be backward compatible.
> If there is ever the need for backward incompatible change to the glibc
> debugger interface, a new DT_XXX element will be provided to access the
> new incompatible interface.
> 
> 	PR gdb/11839
> 	* linux-low.cc (linux_process_target::qxfer_libraries_svr4):
> 	Check r_version < 1 instead of r_version != 1.

Note that GDB doesn't use ChangeLogs.  You can use

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=11839

To link to the Bugzilla entry.

> ---
>  gdbserver/linux-low.cc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index 5c6191d941c..fc7a995351d 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -6845,7 +6845,7 @@ linux_process_target::qxfer_libraries_svr4 (const char *annex,
>  	  if (linux_read_memory (priv->r_debug + lmo->r_version_offset,
>  				 (unsigned char *) &r_version,
>  				 sizeof (r_version)) != 0
> -	      || r_version != 1)
> +	      || r_version < 1)

This patch LGTM.  As you noted, things break when bumping r_version to 2
because of r_ldsomap, but since there isn't a glibc out there using
r_version == 2, I think that this patch can safely go in right now.

Simon
Simon Marchi Aug. 17, 2021, 1:17 p.m. UTC | #4
On 2021-08-16 6:02 p.m., H.J. Lu wrote:
> On Mon, Aug 16, 2021 at 1:30 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> Update gdbserver to check r_version < 1 instead of r_version != 1 so
>> that r_version can be bumped for a new field in the glibc debugger
>> interface to support multiple namespaces.  Since so far, the gdbserver
>> only reads fields defined for r_version == 1, it is compatible with
>> r_version >= 1.
>>
>> All future glibc debugger interface changes will be backward compatible.
>> If there is ever the need for backward incompatible change to the glibc
>> debugger interface, a new DT_XXX element will be provided to access the
>> new incompatible interface.
>>
>>         PR gdb/11839
>>         * linux-low.cc (linux_process_target::qxfer_libraries_svr4):
>>         Check r_version < 1 instead of r_version != 1.
>> ---
>>  gdbserver/linux-low.cc | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
>> index 5c6191d941c..fc7a995351d 100644
>> --- a/gdbserver/linux-low.cc
>> +++ b/gdbserver/linux-low.cc
>> @@ -6845,7 +6845,7 @@ linux_process_target::qxfer_libraries_svr4 (const char *annex,
>>           if (linux_read_memory (priv->r_debug + lmo->r_version_offset,
>>                                  (unsigned char *) &r_version,
>>                                  sizeof (r_version)) != 0
>> -             || r_version != 1)
>> +             || r_version < 1)
>>             {
>>               warning ("unexpected r_debug version %d", r_version);
>>             }
>> --
>> 2.31.1
>>
> 
> Set r_version == 2 breaks GDB due to
> 
> static CORE_ADDR
> solib_svr4_r_ldsomap (struct svr4_info *info)
> {
>   struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
>   struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
>   enum bfd_endian byte_order = type_byte_order (ptr_type);
>   ULONGEST version = 0;
> 
>   try
>     {
>       /* Check version, and return zero if `struct r_debug' doesn't have
>          the r_ldsomap member.  */
>       version
>         = read_memory_unsigned_integer (info->debug_base +
> lmo->r_version_offset,
>                                         lmo->r_version_size, byte_order);
>     }
>   catch (const gdb_exception_error &ex)
>     {
>       exception_print (gdb_stderr, ex);
>     }
> 
>   if (version < 2 || lmo->r_ldsomap_offset == -1)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> glibc doesn't have r_ldsomap.  But r_ldsomap_offset is set
> unconditionally.   Shouldn't it be set only if the target debugger
> interface has it?

Ideally, I think that Solaris support would implement its own
"solaris_{ilp32,lp64}_fetch_link_map_offsets", where it would set
r_ldsomap_offset.  And either there wouldn't be a generic
svr4_{ilp32,lp64}_fetch_link_map_offsets (each OS support would provide
its own, because things vary so much between OSes) or
svr4_{ilp32,lp64}_fetch_link_map_offsets would just specify what's
really the common denominator between all of them.

I think that would allow remove that "version < 2" check that is a bit
ugly.

Simon
Florian Weimer Aug. 17, 2021, 1:48 p.m. UTC | #5
* H. J. Lu via Gdb-patches:

> Set r_version == 2 breaks GDB due to
>
> static CORE_ADDR
> solib_svr4_r_ldsomap (struct svr4_info *info)
> {
>   struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
>   struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
>   enum bfd_endian byte_order = type_byte_order (ptr_type);
>   ULONGEST version = 0;
>
>   try
>     {
>       /* Check version, and return zero if `struct r_debug' doesn't have
>          the r_ldsomap member.  */
>       version
>         = read_memory_unsigned_integer (info->debug_base +
> lmo->r_version_offset,
>                                         lmo->r_version_size, byte_order);
>     }
>   catch (const gdb_exception_error &ex)
>     {
>       exception_print (gdb_stderr, ex);
>     }
>
>   if (version < 2 || lmo->r_ldsomap_offset == -1)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> glibc doesn't have r_ldsomap.  But r_ldsomap_offset is set
> unconditionally.   Shouldn't it be set only if the target debugger
> interface has it?

glibc should add r_ldsomap_offset and switch its own version number to
3, I think.

Thanks,
Florian
H.J. Lu Aug. 17, 2021, 2:04 p.m. UTC | #6
On Tue, Aug 17, 2021 at 6:48 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu via Gdb-patches:
>
> > Set r_version == 2 breaks GDB due to
> >
> > static CORE_ADDR
> > solib_svr4_r_ldsomap (struct svr4_info *info)
> > {
> >   struct link_map_offsets *lmo = svr4_fetch_link_map_offsets ();
> >   struct type *ptr_type = builtin_type (target_gdbarch ())->builtin_data_ptr;
> >   enum bfd_endian byte_order = type_byte_order (ptr_type);
> >   ULONGEST version = 0;
> >
> >   try
> >     {
> >       /* Check version, and return zero if `struct r_debug' doesn't have
> >          the r_ldsomap member.  */
> >       version
> >         = read_memory_unsigned_integer (info->debug_base +
> > lmo->r_version_offset,
> >                                         lmo->r_version_size, byte_order);
> >     }
> >   catch (const gdb_exception_error &ex)
> >     {
> >       exception_print (gdb_stderr, ex);
> >     }
> >
> >   if (version < 2 || lmo->r_ldsomap_offset == -1)
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > glibc doesn't have r_ldsomap.  But r_ldsomap_offset is set
> > unconditionally.   Shouldn't it be set only if the target debugger
> > interface has it?
>
> glibc should add r_ldsomap_offset and switch its own version number to
> 3, I think.

We can add an OS flavor field to struct link_map_offsets for this.
diff mbox series

Patch

diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 5c6191d941c..fc7a995351d 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -6845,7 +6845,7 @@  linux_process_target::qxfer_libraries_svr4 (const char *annex,
 	  if (linux_read_memory (priv->r_debug + lmo->r_version_offset,
 				 (unsigned char *) &r_version,
 				 sizeof (r_version)) != 0
-	      || r_version != 1)
+	      || r_version < 1)
 	    {
 	      warning ("unexpected r_debug version %d", r_version);
 	    }