Re: [PATCH] [Arm] Remove dead FPA code

[Luis Machado <luis.machado@arm.com>]

On 10/13/22 09:44, Pedro Alves wrote:
> On 2022-10-13 8:18 a.m., Luis Machado wrote:
>> On 10/10/22 15:56, Pedro Alves wrote:
> 
>>>> More recently we've added two new guesses for MVE and M-profile system registers, but it doesn't
>>>> make sense to do so, as these features are advertised as XML already.
>>>
>>> What was the justification for adding them back then, then?
>>>
>>
>> It was meant to support other register sets under a situation where no XML is sent back.
> 
> Well, that's the reason for all the guesses existing.  The question was more about -- why were _those_
> particular guesses needed / added?  Maybe because such stubs already existed in the wild, and
> people were using some downstream patched version of GDB to work with them?
> 

They weren't needed at all. mve is reported through XML by gdbserver and qemu. The system registers are only currently
supported by STLink, and that also uses XML.

>> Though valid, my feeling is that
>> it was done mostly as copy/pasting what was already being done for the GPR's/FPA. The documentation isn't exactly clear
>> on whether one should do it or not. Only a few targets use these guesses (Arm, Mips and Microblaze).
> 
> It's strictly a backwards compatibility feature.  It's explained in the comment:
> 

Right, but since this is an old port and the background is fairly convoluted, it might be easy to miss if you
don't know some of its history. I consider the mve/system bits of the guesses dead code. Though we could argue now someone may come up with
a stub that uses mve and the system registers without XML, and that we will need to support that going forward.

I don't think that's realistic though. I hope you'll agree.

> /* For backward-compatibility we allow two 'g' packet lengths with
>     the remote protocol depending on whether FPA registers are
>     supplied.  M-profile targets do not have FPA registers, but some
>     stubs already exist in the wild which use a 'g' packet which
>     supplies them albeit with dummy values.  The packet format which
>     includes FPA registers should be considered deprecated for
>     M-profile targets.  */
> 
> static void
> arm_register_g_packet_guesses (struct gdbarch *gdbarch)
> {
> 
> Is there something that could be improved in that comment to make it clearer?

Yes, we should put a big warning stating we should no longer update this code, until it can be dropped without
upsetting other software.

> 
>> Removing the guesses doesn't buy us much on its own, but there is opportunity to get targets to move
>> to XML descriptions, which is something easier to work with, more flexible and less error-prone. So it looks
>> like a step in the right direction.
> 
> The thing is that the old/existing stubs that don't send an XML don't need the flexibility.  They just need
> to continue working as they do today.  Removing the guesses and changing the default g/G packet layout
> causes pain to _users_ foremost.  IMO, removing backwards compatibility just for the sake of it doesn't
> really help anyone.

Although that's true, it is also true that a recent-enough GDB could be used to debug those very old targets after
the compatibility layer is dropped in the future.

> 
> For new registers/features, XML is how to get the flexibility.  So I think the angle should be that
> we, as maintainers, should not accept adding support for more/new registers without XML tdescs.
Yes, and that's already the case.