Message ID | YRqD+iTeKfkn94Nn@gmail.com |
---|---|
State | New |
Headers | show |
Series | gdbserver: Check r_version < 1 for Linux debugger interface | expand |
On 2021-08-16 11:27 a.m., H.J. Lu via Gdb-patches 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. > > 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); > } > I don't understand how this change on its own is useful. If r_version gets bumped from 1 to 2, it's presumably because there are backwards incompatible changes done to the interface. Without the corresponding changes to adjust to that new interface, then we just risk having a gdbserver trying to read a library list with r_version == 2 without actually knowing how to read a library list with r_version == 2. So it will potentially interpret the data in a wrong way and behave badly. Simon
On Mon, Aug 16, 2021 at 9:26 AM Simon Marchi <simon.marchi@polymtl.ca> wrote: > > On 2021-08-16 11:27 a.m., H.J. Lu via Gdb-patches 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. > > > > 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); > > } > > > > I don't understand how this change on its own is useful. If r_version > gets bumped from 1 to 2, it's presumably because there are backwards No. The all glibc debugger interface changes will be backward compatible. > incompatible changes done to the interface. Without the corresponding > changes to adjust to that new interface, then we just risk having a > gdbserver trying to read a library list with r_version == 2 without > actually knowing how to read a library list with r_version == 2. So Since all future interface changes will be backward compatible, gdbserver just needs to check r_version for incompatible implementation. Since the current gdbserver only reads fields defined for r_version == 1, it is compatible with r_version >= 1. > it will potentially interpret the data in a wrong way and behave badly. > > Simon
On 2021-08-16 2:02 p.m., H.J. Lu wrote: > On Mon, Aug 16, 2021 at 9:26 AM Simon Marchi <simon.marchi@polymtl.ca> wrote: >> >> On 2021-08-16 11:27 a.m., H.J. Lu via Gdb-patches 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. >>> >>> 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); >>> } >>> >> >> I don't understand how this change on its own is useful. If r_version >> gets bumped from 1 to 2, it's presumably because there are backwards > > No. The all glibc debugger interface changes will be backward compatible. Ok, this is an important piece of information, please add it to the commit message. >> incompatible changes done to the interface. Without the corresponding >> changes to adjust to that new interface, then we just risk having a >> gdbserver trying to read a library list with r_version == 2 without >> actually knowing how to read a library list with r_version == 2. So > > Since all future interface changes will be backward compatible, gdbserver > just needs to check r_version for incompatible implementation. Since the > current gdbserver only reads fields defined for r_version == 1, it is compatible > with r_version >= 1. I'm just curious, if there is ever the need to do a backwards incompatible change, how would that be handled? Simon
On Mon, Aug 16, 2021 at 11:08 AM Simon Marchi <simon.marchi@polymtl.ca> wrote: > > On 2021-08-16 2:02 p.m., H.J. Lu wrote: > > On Mon, Aug 16, 2021 at 9:26 AM Simon Marchi <simon.marchi@polymtl.ca> wrote: > >> > >> On 2021-08-16 11:27 a.m., H.J. Lu via Gdb-patches 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. > >>> > >>> 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); > >>> } > >>> > >> > >> I don't understand how this change on its own is useful. If r_version > >> gets bumped from 1 to 2, it's presumably because there are backwards > > > > No. The all glibc debugger interface changes will be backward compatible. > > Ok, this is an important piece of information, please add it to the > commit message. Will do. > >> incompatible changes done to the interface. Without the corresponding > >> changes to adjust to that new interface, then we just risk having a > >> gdbserver trying to read a library list with r_version == 2 without > >> actually knowing how to read a library list with r_version == 2. So > > > > Since all future interface changes will be backward compatible, gdbserver > > just needs to check r_version for incompatible implementation. Since the > > current gdbserver only reads fields defined for r_version == 1, it is compatible > > with r_version >= 1. > > I'm just curious, if there is ever the need to do a backwards > incompatible change, how would that be handled? If it does happen, we will provide its access via a new DT_XXX element.
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); }