[PATCHv5,05/11] gdbserver/x86: move no-xml code earlier in x86_linux_read_description

  This commit is part of a series that aims to share more of the x86
target description reading/generation code between GDB and gdbserver.

There are a huge number of similarities between the code in
gdbserver's x86_linux_read_description function and GDB's
x86_linux_nat_target::read_description function, and it is this
similarity that I plan, in a later commit, to share between GDB and
gdbserver.

However, one thing that is different in x86_linux_read_description is
the code inside the '!use_xml' block.  This is the code that handles
the case where gdbserver is not allowed to send an XML target
description back to GDB.  In this case gdbserver uses some predefined,
fixed, target descriptions.

First, it's worth noting that I suspect this code is not tested any
more.  I couldn't find anything in the testsuite that tries to disable
XML target description support.  And the idea of having a single
"fixed" target description really doesn't work well when we think
about all the various x86 extensions that exist.  Part of me would
like to rip out the no-xml support in gdbserver (at least for x86),
and if a GDB connects that doesn't support XML target descriptions,
gdbserver can just give an error and drop the connection.  GDB has
supported XML target descriptions for 16 years now, I think it would
be reasonable for our shipped gdbserver to drop support for the old
way of doing things.

Anyway.... this commit doesn't do that.

What I did notice was that, over time, the '!use_xml' block appears to
have "drifted" within the x86_linux_read_description function; it's
now not the first check we do.  Instead we make some ptrace calls and
return a target description generated based on the result of these
ptrace calls.  Surely it only makes sense to generate variable target
descriptions if we can send these back to GDB?

So in this commit I propose to move the '!use_xml' block earlier in
the x86_linux_read_description function.

The benefit of this is that this leaves the later half of
x86_linux_read_description much more similar to the GDB function
x86_linux_nat_target::read_description and sets us up for potentially
sharing code between GDB and gdbserver in a later commit.

Approved-By: John Baldwin <jhb@FreeBSD.org>
---
 gdbserver/linux-x86-low.cc | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)
  

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Freitag, 26. April 2024 17:02
> To: gdb-patches@sourceware.org
> Cc: Andrew Burgess <aburgess@redhat.com>; Willgerodt, Felix
> <felix.willgerodt@intel.com>; John Baldwin <jhb@FreeBSD.org>
> Subject: [PATCHv5 05/11] gdbserver/x86: move no-xml code earlier in
> x86_linux_read_description
> 
> This commit is part of a series that aims to share more of the x86
> target description reading/generation code between GDB and gdbserver.
> 
> There are a huge number of similarities between the code in
> gdbserver's x86_linux_read_description function and GDB's
> x86_linux_nat_target::read_description function, and it is this
> similarity that I plan, in a later commit, to share between GDB and
> gdbserver.
> 
> However, one thing that is different in x86_linux_read_description is
> the code inside the '!use_xml' block.  This is the code that handles
> the case where gdbserver is not allowed to send an XML target
> description back to GDB.  In this case gdbserver uses some predefined,
> fixed, target descriptions.
> 
> First, it's worth noting that I suspect this code is not tested any
> more.  I couldn't find anything in the testsuite that tries to disable
> XML target description support.  And the idea of having a single
> "fixed" target description really doesn't work well when we think
> about all the various x86 extensions that exist.  Part of me would
> like to rip out the no-xml support in gdbserver (at least for x86),
> and if a GDB connects that doesn't support XML target descriptions,
> gdbserver can just give an error and drop the connection.  GDB has
> supported XML target descriptions for 16 years now, I think it would
> be reasonable for our shipped gdbserver to drop support for the old
> way of doing things.

Interesting. I for one would +1 this. Slightly related:
I wonder if anyone really builds GDB without libexpat anymore and
if we couldn't even think of making it mandatory. (Which doesn't
mean dropping the ball on supporting stubs without XML.)

 
> Anyway.... this commit doesn't do that.
>
> What I did notice was that, over time, the '!use_xml' block appears to
> have "drifted" within the x86_linux_read_description function; it's
> now not the first check we do.  Instead we make some ptrace calls and
> return a target description generated based on the result of these
> ptrace calls.  Surely it only makes sense to generate variable target
> descriptions if we can send these back to GDB?
> 
> So in this commit I propose to move the '!use_xml' block earlier in
> the x86_linux_read_description function.
> 
> The benefit of this is that this leaves the later half of
> x86_linux_read_description much more similar to the GDB function
> x86_linux_nat_target::read_description and sets us up for potentially
> sharing code between GDB and gdbserver in a later commit.
> 
> Approved-By: John Baldwin <jhb@FreeBSD.org>
> ---
>  gdbserver/linux-x86-low.cc | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 

Looks fine to me.

Approved-By: Felix Willgerodt <felix.willgerodt@intel.com>

Thanks,
Felix

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

On 4/29/24 15:34, Willgerodt, Felix wrote:
>> -----Original Message-----
>> From: Andrew Burgess <aburgess@redhat.com>
>> Sent: Freitag, 26. April 2024 17:02
>> To: gdb-patches@sourceware.org
>> Cc: Andrew Burgess <aburgess@redhat.com>; Willgerodt, Felix
>> <felix.willgerodt@intel.com>; John Baldwin <jhb@FreeBSD.org>
>> Subject: [PATCHv5 05/11] gdbserver/x86: move no-xml code earlier in
>> x86_linux_read_description
>>
>> This commit is part of a series that aims to share more of the x86
>> target description reading/generation code between GDB and gdbserver.
>>
>> There are a huge number of similarities between the code in
>> gdbserver's x86_linux_read_description function and GDB's
>> x86_linux_nat_target::read_description function, and it is this
>> similarity that I plan, in a later commit, to share between GDB and
>> gdbserver.
>>
>> However, one thing that is different in x86_linux_read_description is
>> the code inside the '!use_xml' block.  This is the code that handles
>> the case where gdbserver is not allowed to send an XML target
>> description back to GDB.  In this case gdbserver uses some predefined,
>> fixed, target descriptions.
>>
>> First, it's worth noting that I suspect this code is not tested any
>> more.  I couldn't find anything in the testsuite that tries to disable
>> XML target description support.  And the idea of having a single
>> "fixed" target description really doesn't work well when we think
>> about all the various x86 extensions that exist.  Part of me would
>> like to rip out the no-xml support in gdbserver (at least for x86),
>> and if a GDB connects that doesn't support XML target descriptions,
>> gdbserver can just give an error and drop the connection.  GDB has
>> supported XML target descriptions for 16 years now, I think it would
>> be reasonable for our shipped gdbserver to drop support for the old
>> way of doing things.
> 
> Interesting. I for one would +1 this. Slightly related:
> I wonder if anyone really builds GDB without libexpat anymore and
> if we couldn't even think of making it mandatory. (Which doesn't
> mean dropping the ball on supporting stubs without XML.)

I'd support making xml descriptions mandatory for some targets. AArch64 and
32-bit Arm both rely heavily on xml description for feature discovery.

Off the top of my head, I think very old probes and the Linux Kernel's kgdb don't
support xml. I'm not sure how often kgdb gets used though, but falling back to a
default description should work for AArch64 and 32-bit Arm.
  

Luis Machado <luis.machado@arm.com> writes:

> On 4/29/24 15:34, Willgerodt, Felix wrote:
>>> -----Original Message-----
>>> From: Andrew Burgess <aburgess@redhat.com>
>>> Sent: Freitag, 26. April 2024 17:02
>>> To: gdb-patches@sourceware.org
>>> Cc: Andrew Burgess <aburgess@redhat.com>; Willgerodt, Felix
>>> <felix.willgerodt@intel.com>; John Baldwin <jhb@FreeBSD.org>
>>> Subject: [PATCHv5 05/11] gdbserver/x86: move no-xml code earlier in
>>> x86_linux_read_description
>>>
>>> This commit is part of a series that aims to share more of the x86
>>> target description reading/generation code between GDB and gdbserver.
>>>
>>> There are a huge number of similarities between the code in
>>> gdbserver's x86_linux_read_description function and GDB's
>>> x86_linux_nat_target::read_description function, and it is this
>>> similarity that I plan, in a later commit, to share between GDB and
>>> gdbserver.
>>>
>>> However, one thing that is different in x86_linux_read_description is
>>> the code inside the '!use_xml' block.  This is the code that handles
>>> the case where gdbserver is not allowed to send an XML target
>>> description back to GDB.  In this case gdbserver uses some predefined,
>>> fixed, target descriptions.
>>>
>>> First, it's worth noting that I suspect this code is not tested any
>>> more.  I couldn't find anything in the testsuite that tries to disable
>>> XML target description support.  And the idea of having a single
>>> "fixed" target description really doesn't work well when we think
>>> about all the various x86 extensions that exist.  Part of me would
>>> like to rip out the no-xml support in gdbserver (at least for x86),
>>> and if a GDB connects that doesn't support XML target descriptions,
>>> gdbserver can just give an error and drop the connection.  GDB has
>>> supported XML target descriptions for 16 years now, I think it would
>>> be reasonable for our shipped gdbserver to drop support for the old
>>> way of doing things.
>> 
>> Interesting. I for one would +1 this. Slightly related:
>> I wonder if anyone really builds GDB without libexpat anymore and
>> if we couldn't even think of making it mandatory. (Which doesn't
>> mean dropping the ball on supporting stubs without XML.)
>
> I'd support making xml descriptions mandatory for some targets. AArch64 and
> 32-bit Arm both rely heavily on xml description for feature discovery.

I might be wrong, but I don't think Felix was even suggesting we need to
go that far.  I think the suggestion was just that GDB itself should
require libexpat, which would mean we have the potential to read XML
target descriptions.

This wouldn't mean that targets would be required to send XML target
descriptions.

My comment was specifically that x86 gdbserver should drop its no XML
support, and if a user tries to connect with a no XML GDB then gdbserver
would reject the connection.

Moving to make libexpat a requirement would help in this case as a user
would no longer be able to build a GDB which couldn't connect to an x86
gdbserver.

> Off the top of my head, I think very old probes and the Linux Kernel's kgdb don't
> support xml. I'm not sure how often kgdb gets used though, but falling back to a
> default description should work for AArch64 and 32-bit Arm.

I don't believe building GDB with libexpat support breaks the ability to
connect to a target which doesn't support XML descriptions (if it does
then that's a bug), so hopefully nobody else would be
inconvenienced... except for those folk who are building GDB without
libexpat support.

I've added making these changes to my todo list, but I don't want to add
them to this series.

Thanks,
Andrew
  

On 5/7/24 16:43, Andrew Burgess wrote:
> Luis Machado <luis.machado@arm.com> writes:
> 
>> On 4/29/24 15:34, Willgerodt, Felix wrote:
>>>> -----Original Message-----
>>>> From: Andrew Burgess <aburgess@redhat.com>
>>>> Sent: Freitag, 26. April 2024 17:02
>>>> To: gdb-patches@sourceware.org
>>>> Cc: Andrew Burgess <aburgess@redhat.com>; Willgerodt, Felix
>>>> <felix.willgerodt@intel.com>; John Baldwin <jhb@FreeBSD.org>
>>>> Subject: [PATCHv5 05/11] gdbserver/x86: move no-xml code earlier in
>>>> x86_linux_read_description
>>>>
>>>> This commit is part of a series that aims to share more of the x86
>>>> target description reading/generation code between GDB and gdbserver.
>>>>
>>>> There are a huge number of similarities between the code in
>>>> gdbserver's x86_linux_read_description function and GDB's
>>>> x86_linux_nat_target::read_description function, and it is this
>>>> similarity that I plan, in a later commit, to share between GDB and
>>>> gdbserver.
>>>>
>>>> However, one thing that is different in x86_linux_read_description is
>>>> the code inside the '!use_xml' block.  This is the code that handles
>>>> the case where gdbserver is not allowed to send an XML target
>>>> description back to GDB.  In this case gdbserver uses some predefined,
>>>> fixed, target descriptions.
>>>>
>>>> First, it's worth noting that I suspect this code is not tested any
>>>> more.  I couldn't find anything in the testsuite that tries to disable
>>>> XML target description support.  And the idea of having a single
>>>> "fixed" target description really doesn't work well when we think
>>>> about all the various x86 extensions that exist.  Part of me would
>>>> like to rip out the no-xml support in gdbserver (at least for x86),
>>>> and if a GDB connects that doesn't support XML target descriptions,
>>>> gdbserver can just give an error and drop the connection.  GDB has
>>>> supported XML target descriptions for 16 years now, I think it would
>>>> be reasonable for our shipped gdbserver to drop support for the old
>>>> way of doing things.
>>>
>>> Interesting. I for one would +1 this. Slightly related:
>>> I wonder if anyone really builds GDB without libexpat anymore and
>>> if we couldn't even think of making it mandatory. (Which doesn't
>>> mean dropping the ball on supporting stubs without XML.)
>>
>> I'd support making xml descriptions mandatory for some targets. AArch64 and
>> 32-bit Arm both rely heavily on xml description for feature discovery.
> 
> I might be wrong, but I don't think Felix was even suggesting we need to
> go that far.  I think the suggestion was just that GDB itself should
> require libexpat, which would mean we have the potential to read XML
> target descriptions.

Sorry, I might've read too much into it. But making libexpat a requirement for gdb
would be a good move in my opinion, nonetheless. Cross-building gdb sometimes yields a gdb
with no libexpat and that is not very useful for AArch64/Arm debugging for instance.

> 
> This wouldn't mean that targets would be required to send XML target
> descriptions.> 
> My comment was specifically that x86 gdbserver should drop its no XML
> support, and if a user tries to connect with a no XML GDB then gdbserver
> would reject the connection.

Understood.

> 
> Moving to make libexpat a requirement would help in this case as a user
> would no longer be able to build a GDB which couldn't connect to an x86
> gdbserver.
> 
>> Off the top of my head, I think very old probes and the Linux Kernel's kgdb don't
>> support xml. I'm not sure how often kgdb gets used though, but falling back to a
>> default description should work for AArch64 and 32-bit Arm.
> 
> I don't believe building GDB with libexpat support breaks the ability to
> connect to a target which doesn't support XML descriptions (if it does
> then that's a bug), so hopefully nobody else would be
> inconvenienced... except for those folk who are building GDB without
> libexpat support.

Makes sense. Thanks for clarifying.

> 
> I've added making these changes to my todo list, but I don't want to add
> them to this series.
> 
> Thanks,
> Andrew
>
  

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Dienstag, 7. Mai 2024 17:44
> To: Luis Machado <luis.machado@arm.com>; Willgerodt, Felix
> <felix.willgerodt@intel.com>; gdb-patches@sourceware.org
> Cc: John Baldwin <jhb@FreeBSD.org>
> Subject: Re: [PATCHv5 05/11] gdbserver/x86: move no-xml code earlier in
> x86_linux_read_description
> 
> Luis Machado <luis.machado@arm.com> writes:
> 
> > On 4/29/24 15:34, Willgerodt, Felix wrote:
> >>> -----Original Message-----
> >>> From: Andrew Burgess <aburgess@redhat.com>
> >>> Sent: Freitag, 26. April 2024 17:02
> >>> To: gdb-patches@sourceware.org
> >>> Cc: Andrew Burgess <aburgess@redhat.com>; Willgerodt, Felix
> >>> <felix.willgerodt@intel.com>; John Baldwin <jhb@FreeBSD.org>
> >>> Subject: [PATCHv5 05/11] gdbserver/x86: move no-xml code earlier in
> >>> x86_linux_read_description
> >>>
> >>> This commit is part of a series that aims to share more of the x86
> >>> target description reading/generation code between GDB and gdbserver.
> >>>
> >>> There are a huge number of similarities between the code in
> >>> gdbserver's x86_linux_read_description function and GDB's
> >>> x86_linux_nat_target::read_description function, and it is this
> >>> similarity that I plan, in a later commit, to share between GDB and
> >>> gdbserver.
> >>>
> >>> However, one thing that is different in x86_linux_read_description is
> >>> the code inside the '!use_xml' block.  This is the code that handles
> >>> the case where gdbserver is not allowed to send an XML target
> >>> description back to GDB.  In this case gdbserver uses some predefined,
> >>> fixed, target descriptions.
> >>>
> >>> First, it's worth noting that I suspect this code is not tested any
> >>> more.  I couldn't find anything in the testsuite that tries to disable
> >>> XML target description support.  And the idea of having a single
> >>> "fixed" target description really doesn't work well when we think
> >>> about all the various x86 extensions that exist.  Part of me would
> >>> like to rip out the no-xml support in gdbserver (at least for x86),
> >>> and if a GDB connects that doesn't support XML target descriptions,
> >>> gdbserver can just give an error and drop the connection.  GDB has
> >>> supported XML target descriptions for 16 years now, I think it would
> >>> be reasonable for our shipped gdbserver to drop support for the old
> >>> way of doing things.
> >>
> >> Interesting. I for one would +1 this. Slightly related:
> >> I wonder if anyone really builds GDB without libexpat anymore and
> >> if we couldn't even think of making it mandatory. (Which doesn't
> >> mean dropping the ball on supporting stubs without XML.)
> >
> > I'd support making xml descriptions mandatory for some targets. AArch64 and
> > 32-bit Arm both rely heavily on xml description for feature discovery.
> 
> I might be wrong, but I don't think Felix was even suggesting we need to
> go that far. 

You are correct.

> I think the suggestion was just that GDB itself should
> require libexpat, which would mean we have the potential to read XML
> target descriptions.
> 
> This wouldn't mean that targets would be required to send XML target
> descriptions.
> 
> My comment was specifically that x86 gdbserver should drop its no XML
> support, and if a user tries to connect with a no XML GDB then gdbserver
> would reject the connection.
> 
> Moving to make libexpat a requirement would help in this case as a user
> would no longer be able to build a GDB which couldn't connect to an x86
> gdbserver.
> 
> > Off the top of my head, I think very old probes and the Linux Kernel's kgdb don't
> > support xml. I'm not sure how often kgdb gets used though, but falling back to a
> > default description should work for AArch64 and 32-bit Arm.
> 
> I don't believe building GDB with libexpat support breaks the ability to
> connect to a target which doesn't support XML descriptions (if it does
> then that's a bug), so hopefully nobody else would be
> inconvenienced... except for those folk who are building GDB without
> libexpat support.

One immediate concern in my mind would be testability for no-xml stubs
(without having looked at the current testing situation). Anyway, this is not
really a topic for this series, just an interesting side discussion.

Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

"Willgerodt, Felix" <felix.willgerodt@intel.com> writes:

>> -----Original Message-----
>> From: Andrew Burgess <aburgess@redhat.com>
>> Sent: Dienstag, 7. Mai 2024 17:44
>> To: Luis Machado <luis.machado@arm.com>; Willgerodt, Felix
>> <felix.willgerodt@intel.com>; gdb-patches@sourceware.org
>> Cc: John Baldwin <jhb@FreeBSD.org>
>> Subject: Re: [PATCHv5 05/11] gdbserver/x86: move no-xml code earlier in
>> x86_linux_read_description
>> 
>> Luis Machado <luis.machado@arm.com> writes:
>> 
>> > On 4/29/24 15:34, Willgerodt, Felix wrote:
>> >>> -----Original Message-----
>> >>> From: Andrew Burgess <aburgess@redhat.com>
>> >>> Sent: Freitag, 26. April 2024 17:02
>> >>> To: gdb-patches@sourceware.org
>> >>> Cc: Andrew Burgess <aburgess@redhat.com>; Willgerodt, Felix
>> >>> <felix.willgerodt@intel.com>; John Baldwin <jhb@FreeBSD.org>
>> >>> Subject: [PATCHv5 05/11] gdbserver/x86: move no-xml code earlier in
>> >>> x86_linux_read_description
>> >>>
>> >>> This commit is part of a series that aims to share more of the x86
>> >>> target description reading/generation code between GDB and gdbserver.
>> >>>
>> >>> There are a huge number of similarities between the code in
>> >>> gdbserver's x86_linux_read_description function and GDB's
>> >>> x86_linux_nat_target::read_description function, and it is this
>> >>> similarity that I plan, in a later commit, to share between GDB and
>> >>> gdbserver.
>> >>>
>> >>> However, one thing that is different in x86_linux_read_description is
>> >>> the code inside the '!use_xml' block.  This is the code that handles
>> >>> the case where gdbserver is not allowed to send an XML target
>> >>> description back to GDB.  In this case gdbserver uses some predefined,
>> >>> fixed, target descriptions.
>> >>>
>> >>> First, it's worth noting that I suspect this code is not tested any
>> >>> more.  I couldn't find anything in the testsuite that tries to disable
>> >>> XML target description support.  And the idea of having a single
>> >>> "fixed" target description really doesn't work well when we think
>> >>> about all the various x86 extensions that exist.  Part of me would
>> >>> like to rip out the no-xml support in gdbserver (at least for x86),
>> >>> and if a GDB connects that doesn't support XML target descriptions,
>> >>> gdbserver can just give an error and drop the connection.  GDB has
>> >>> supported XML target descriptions for 16 years now, I think it would
>> >>> be reasonable for our shipped gdbserver to drop support for the old
>> >>> way of doing things.
>> >>
>> >> Interesting. I for one would +1 this. Slightly related:
>> >> I wonder if anyone really builds GDB without libexpat anymore and
>> >> if we couldn't even think of making it mandatory. (Which doesn't
>> >> mean dropping the ball on supporting stubs without XML.)
>> >
>> > I'd support making xml descriptions mandatory for some targets. AArch64 and
>> > 32-bit Arm both rely heavily on xml description for feature discovery.
>> 
>> I might be wrong, but I don't think Felix was even suggesting we need to
>> go that far. 
>
> You are correct.
>
>> I think the suggestion was just that GDB itself should
>> require libexpat, which would mean we have the potential to read XML
>> target descriptions.
>> 
>> This wouldn't mean that targets would be required to send XML target
>> descriptions.
>> 
>> My comment was specifically that x86 gdbserver should drop its no XML
>> support, and if a user tries to connect with a no XML GDB then gdbserver
>> would reject the connection.
>> 
>> Moving to make libexpat a requirement would help in this case as a user
>> would no longer be able to build a GDB which couldn't connect to an x86
>> gdbserver.
>> 
>> > Off the top of my head, I think very old probes and the Linux Kernel's kgdb don't
>> > support xml. I'm not sure how often kgdb gets used though, but falling back to a
>> > default description should work for AArch64 and 32-bit Arm.
>> 
>> I don't believe building GDB with libexpat support breaks the ability to
>> connect to a target which doesn't support XML descriptions (if it does
>> then that's a bug), so hopefully nobody else would be
>> inconvenienced... except for those folk who are building GDB without
>> libexpat support.
>
> One immediate concern in my mind would be testability for no-xml stubs
> (without having looked at the current testing situation). Anyway, this is not
> really a topic for this series, just an interesting side discussion.

This don't think there would be a problem.  Or, if there is, then that
would be a legitimate bug that needs fixing.

Right now I would not expect a user to have to build a no-xml GDB in
order to talk to a no-xml target.  I'd expect the user to fire up there
standard XML supporting GDB, connect to the target, and have GDB "just
work".

If there are architectures supported by gdbserver that don't support
sending back an XML description (I haven't checked) then they should
work just as they do today, even if GDB supports reading XML.

Thanks,
Andrew