Message ID | YRrK+nOJmShP932/@gmail.com |
---|---|
State | New |
Headers | show |
Series | [v2] gdbserver: Check r_version < 1 for Linux debugger interface | expand |
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); }
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
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
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
* 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
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 --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); }