RE: [PATCH] gdb/tdesc: Don't assign custom-group tdesc registers to 'general'

[Ciaran Woodward <ciaranwoodward@xmos.com>]

Hi,

Luis:
> Though the patch doesn't regress things as much as before, it still
> makes the 32-bit Arm gdb drop the fpscr register from the general display
> (info registers).
>
> Depending on what others think, and the benefit of not having a bunch
> of system registers show up every time, we could cope with this
> particular regression.

The most severely affected target I believe is the s390, which puts
many registers into 'lower' and 'upper' groups in the tdesc. This
seemingly distinguishes the upper and lower 32-bits of 64-bit registers. 

However, the s390 already implements proper 64 bit versions of
these registers as 'pseudo' registers, which are added to the general set.
With the current master behaviour, 'info registers' will print the
pseudo (64-bit) versions, the upper half and the lower half separately,
which is not particularly useful. After this patch, only the pseudo
versions will be printed - which I feel is better behaviour.

I would be interested to hear if anyone thinks the new behaviour is a
'harmful' regression, because we could avoid the regression, its just
more work, and provides arguably worse user experience.

(I did a sweep of other potentially affected platforms below)

Andrew:
> I wonder if the right solution would be to work around existing "broken"
> stubs in the *-tdep.c code?
> 
> We could add a function to target-descriptions.c which sets a register's
> group name ONLY when the group name is empty.
> 
> Then from the *-tdep.c code we can fill in groups which for important
> registers, which are known to possibly be empty.
> 
> Or we could extend tdesc_numbered_register and maybe
> tdesc_unnumbered_register to take an optional "default" group, the
> default would be applied if the register is found, and doesn't already
> have a group set.

Unless I'm misunderstanding you, the patch as currently shown will
already work as you specify and fall back to the default group handling
(adding to general, vector or float depending on type - using
default_register_reggroup_p). It will do this for all cases where the
tdesc group is empty.

I assumed there is no reason to actually be able to add a register to
'no group' - since tdesc can specify an arbitrary group name, it is
always possible to specify a nonstandard group to put excess registers
that shouldn't be shown under normal 'info registers'.

The legacy behaviour we might want to replicate was that registers were
added to multiple groups: the one they were added to in the tdesc, and
the one they get added to by type. So the 'fpscr' register Luis mentioned
would be displayed in *both* 'info registers general' and 'info registers float'.

If we do want to maintain the legacy behaviour, I see two ways to
re-implement this for stubs which send legacy target descriptions:

1. A tdep-override approach (similar to what you suggest), implementing
a gdbarch_register_reggroup_p for the target which calls into
tdesc_register_in_reggroup_p but ignores 0 (NOT_IN_GROUP) returns
for known-bad registers. This would be a reasonable approach if we
think we know the full set of affected registers, and it would avoid the
argument/attribute extra bloat that option 2 would entail.

2. A different tdep-override, which just uses exactly the legacy
behaviour for the architecture, by calling a different version of
tdesc_use_registers.

3. Add some boolean attribute to the register to trigger the 'new'
behaviour, and default it to the old behaviour if not specified. Something
like a defaultGroups='false' in the XML, which would exclude the register
from the legacy behaviour, and exclusively add it to the groups specified
by 'groups' and 'save-restore'. Would need an equivilent argument to the
tdesc_create_reg function.

Personally I would lean towards 1, since the number of places where this
is a harmful regression are probably minimal. In most cases, I think the
new behaviour is preferable, such as showing integer float/vector registers
alongside their 'info registers float/vector' friends, and not in "general"
at all. I identified affected registers by checking 'tdesc_create_reg'
calls:

s390, these would have been "general" also:
pswm, pswa: "psw"
acr0-15: "access"
fpc: "float"
orig_r2, last_break, system_call: "system"
tdb0, tac, tct, atia, tr0-15: "tdb"
gds, gddm, gsepla, bc_gsd, bc_gssm, bc_gsepla: "gs"
("upper" & "lower" groups would also have been shown in general, but shouldn't be imo)

mips, these would have been "general" also:
fcsr, fir: "float"
restart: "system"

aarch64, this would have been "general" also:
tag_ctl: "system"

rs6000, this would have been "general" also:
fpscr: "float"

powerpc, these would have been "general" also:
(NOTE: I don't actually see this architecture calling tdesc_use_registers, so does it even use the tdesc functionality?)
fpscr, cfpscr: "float"
vscr, vrsave, cvsr, cvrsave: "vector"

loongarch, these would have been "general" also:
fcc0-7, fcsr: "float"

i386, these would be "general" also:
fctrl, fstat, ftag, fiseg, fioff, foseg, fooff, fop: "float"
mxcsr: "vector"

arm:
fpscr: "float"
wCSSF, wCASF, wCGR0-3: "vector"

Thanks,
Ciaran