* [PATCH] gdb/tdesc: Don't assign custom-group tdesc registers to 'general' @ 2023-09-07 17:07 Ciaran Woodward 2023-09-07 17:47 ` Luis Machado ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Ciaran Woodward @ 2023-09-07 17:07 UTC (permalink / raw) To: gdb-patches; +Cc: Ciaran Woodward The 'Target Description' mechanism in GDB enables the target to supply the full set of registers available on the system to gdb in an XML format. This format enables setting the 'group' of each register, such that they can be queried using the 'info registers <group>' mechanism. However prior to this change, even if a register was explicitly assigned to a group, it would still show up in the 'info registers general' report. This is unexpected, and also disagrees with the comment above the tdesc_register_in_reggroup_p function, which says that '-1' should be returned if the register group is not-known, not the register group is known, but differs. There was a previous change that did address this issue in aa66aac47b4dd38f9524ddb5546c08cc09930d37 but it also caused registers with *no* group in the target description to be removed from 'general', so it was reverted in 440cf44eb0f70830b8d8ac35289f84129c7a35c1 as that behaviour was used by some targets. The change in this commit enhances the usefulness of the tdesc 'group' attribute for adding system configuration registers, of which there may be hundreds - very inconvenient to request and print on every 'info registers' call. --- Email archive link to discussion of previously referenced patch: https://inbox.sourceware.org/gdb-patches/20200120155315.30333-1-shahab.vahedi@gmail.com/T/#m0ce2983153b2f482b66461ed6b97c4b287b09a89 gdb/target-descriptions.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c index cdedf88c793..c1bb03c3adf 100644 --- a/gdb/target-descriptions.c +++ b/gdb/target-descriptions.c @@ -954,14 +954,13 @@ tdesc_register_in_reggroup_p (struct gdbarch *gdbarch, int regno, { struct tdesc_reg *reg = tdesc_find_register (gdbarch, regno); - if (reg != NULL && !reg->group.empty () - && (reg->group == reggroup->name ())) - return 1; - if (reg != NULL && (reggroup == save_reggroup || reggroup == restore_reggroup)) return reg->save_restore; + if (reg != NULL && !reg->group.empty ()) + return (reg->group == reggroup->name ()); + return -1; } -- 2.25.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gdb/tdesc: Don't assign custom-group tdesc registers to 'general' 2023-09-07 17:07 [PATCH] gdb/tdesc: Don't assign custom-group tdesc registers to 'general' Ciaran Woodward @ 2023-09-07 17:47 ` Luis Machado 2023-09-11 10:31 ` Andrew Burgess 2023-09-12 13:48 ` Andrew Burgess 2023-09-11 10:52 ` Andrew Burgess 2023-09-13 11:41 ` [PATCH v2] " Ciaran Woodward 2 siblings, 2 replies; 13+ messages in thread From: Luis Machado @ 2023-09-07 17:47 UTC (permalink / raw) To: Ciaran Woodward, gdb-patches Hi, On 9/7/23 18:07, Ciaran Woodward wrote: > The 'Target Description' mechanism in GDB enables the target to > supply the full set of registers available on the system to gdb > in an XML format. > > This format enables setting the 'group' of each register, such > that they can be queried using the 'info registers <group>' > mechanism. > > However prior to this change, even if a register was explicitly > assigned to a group, it would still show up in the > 'info registers general' report. This is unexpected, and also > disagrees with the comment above the tdesc_register_in_reggroup_p > function, which says that '-1' should be returned if the register > group is not-known, not the register group is known, but differs. > > There was a previous change that did address this issue in > aa66aac47b4dd38f9524ddb5546c08cc09930d37 > but it also caused registers with *no* group in the target > description to be removed from 'general', so it was reverted in > 440cf44eb0f70830b8d8ac35289f84129c7a35c1 > as that behaviour was used by some targets. > > The change in this commit enhances the usefulness of the tdesc > 'group' attribute for adding system configuration registers, > of which there may be hundreds - very inconvenient to request > and print on every 'info registers' call. > --- > > Email archive link to discussion of previously referenced patch: > https://inbox.sourceware.org/gdb-patches/20200120155315.30333-1-shahab.vahedi@gmail.com/T/#m0ce2983153b2f482b66461ed6b97c4b287b09a89 > > gdb/target-descriptions.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c > index cdedf88c793..c1bb03c3adf 100644 > --- a/gdb/target-descriptions.c > +++ b/gdb/target-descriptions.c > @@ -954,14 +954,13 @@ tdesc_register_in_reggroup_p (struct gdbarch *gdbarch, int regno, > { > struct tdesc_reg *reg = tdesc_find_register (gdbarch, regno); > > - if (reg != NULL && !reg->group.empty () > - && (reg->group == reggroup->name ())) > - return 1; > - > if (reg != NULL > && (reggroup == save_reggroup || reggroup == restore_reggroup)) > return reg->save_restore; > > + if (reg != NULL && !reg->group.empty ()) > + return (reg->group == reggroup->name ()); > + > return -1; > } > Yeah, this is a hard one. Unfortunately the outcome then was that we have a number of debugging stubs out in the open that advertise xml's with less-than-correct group information. I think even gdb, to this day, is guilty of that. 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gdb/tdesc: Don't assign custom-group tdesc registers to 'general' 2023-09-07 17:47 ` Luis Machado @ 2023-09-11 10:31 ` Andrew Burgess 2023-09-11 18:41 ` Ciaran Woodward 2023-09-12 13:48 ` Andrew Burgess 1 sibling, 1 reply; 13+ messages in thread From: Andrew Burgess @ 2023-09-11 10:31 UTC (permalink / raw) To: Luis Machado, Ciaran Woodward, gdb-patches Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes: > Hi, > > On 9/7/23 18:07, Ciaran Woodward wrote: >> The 'Target Description' mechanism in GDB enables the target to >> supply the full set of registers available on the system to gdb >> in an XML format. >> >> This format enables setting the 'group' of each register, such >> that they can be queried using the 'info registers <group>' >> mechanism. >> >> However prior to this change, even if a register was explicitly >> assigned to a group, it would still show up in the >> 'info registers general' report. This is unexpected, and also >> disagrees with the comment above the tdesc_register_in_reggroup_p >> function, which says that '-1' should be returned if the register >> group is not-known, not the register group is known, but differs. >> >> There was a previous change that did address this issue in >> aa66aac47b4dd38f9524ddb5546c08cc09930d37 >> but it also caused registers with *no* group in the target >> description to be removed from 'general', so it was reverted in >> 440cf44eb0f70830b8d8ac35289f84129c7a35c1 >> as that behaviour was used by some targets. >> >> The change in this commit enhances the usefulness of the tdesc >> 'group' attribute for adding system configuration registers, >> of which there may be hundreds - very inconvenient to request >> and print on every 'info registers' call. >> --- >> >> Email archive link to discussion of previously referenced patch: >> https://inbox.sourceware.org/gdb-patches/20200120155315.30333-1-shahab.vahedi@gmail.com/T/#m0ce2983153b2f482b66461ed6b97c4b287b09a89 >> >> gdb/target-descriptions.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c >> index cdedf88c793..c1bb03c3adf 100644 >> --- a/gdb/target-descriptions.c >> +++ b/gdb/target-descriptions.c >> @@ -954,14 +954,13 @@ tdesc_register_in_reggroup_p (struct gdbarch *gdbarch, int regno, >> { >> struct tdesc_reg *reg = tdesc_find_register (gdbarch, regno); >> >> - if (reg != NULL && !reg->group.empty () >> - && (reg->group == reggroup->name ())) >> - return 1; >> - >> if (reg != NULL >> && (reggroup == save_reggroup || reggroup == restore_reggroup)) >> return reg->save_restore; >> >> + if (reg != NULL && !reg->group.empty ()) >> + return (reg->group == reggroup->name ()); >> + >> return -1; >> } >> > > Yeah, this is a hard one. Unfortunately the outcome then was that we have a number of debugging stubs out in the open that advertise xml's with less-than-correct group information. > > I think even gdb, to this day, is guilty of that. > > 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. 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. Thanks, Andrew ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] gdb/tdesc: Don't assign custom-group tdesc registers to 'general' 2023-09-11 10:31 ` Andrew Burgess @ 2023-09-11 18:41 ` Ciaran Woodward 0 siblings, 0 replies; 13+ messages in thread From: Ciaran Woodward @ 2023-09-11 18:41 UTC (permalink / raw) To: Andrew Burgess, Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gdb/tdesc: Don't assign custom-group tdesc registers to 'general' 2023-09-07 17:47 ` Luis Machado 2023-09-11 10:31 ` Andrew Burgess @ 2023-09-12 13:48 ` Andrew Burgess 2023-09-12 15:10 ` Luis Machado 1 sibling, 1 reply; 13+ messages in thread From: Andrew Burgess @ 2023-09-12 13:48 UTC (permalink / raw) To: Luis Machado, Ciaran Woodward, gdb-patches Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes: > Hi, > > On 9/7/23 18:07, Ciaran Woodward wrote: >> The 'Target Description' mechanism in GDB enables the target to >> supply the full set of registers available on the system to gdb >> in an XML format. >> >> This format enables setting the 'group' of each register, such >> that they can be queried using the 'info registers <group>' >> mechanism. >> >> However prior to this change, even if a register was explicitly >> assigned to a group, it would still show up in the >> 'info registers general' report. This is unexpected, and also >> disagrees with the comment above the tdesc_register_in_reggroup_p >> function, which says that '-1' should be returned if the register >> group is not-known, not the register group is known, but differs. >> >> There was a previous change that did address this issue in >> aa66aac47b4dd38f9524ddb5546c08cc09930d37 >> but it also caused registers with *no* group in the target >> description to be removed from 'general', so it was reverted in >> 440cf44eb0f70830b8d8ac35289f84129c7a35c1 >> as that behaviour was used by some targets. >> >> The change in this commit enhances the usefulness of the tdesc >> 'group' attribute for adding system configuration registers, >> of which there may be hundreds - very inconvenient to request >> and print on every 'info registers' call. >> --- >> >> Email archive link to discussion of previously referenced patch: >> https://inbox.sourceware.org/gdb-patches/20200120155315.30333-1-shahab.vahedi@gmail.com/T/#m0ce2983153b2f482b66461ed6b97c4b287b09a89 >> >> gdb/target-descriptions.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c >> index cdedf88c793..c1bb03c3adf 100644 >> --- a/gdb/target-descriptions.c >> +++ b/gdb/target-descriptions.c >> @@ -954,14 +954,13 @@ tdesc_register_in_reggroup_p (struct gdbarch *gdbarch, int regno, >> { >> struct tdesc_reg *reg = tdesc_find_register (gdbarch, regno); >> >> - if (reg != NULL && !reg->group.empty () >> - && (reg->group == reggroup->name ())) >> - return 1; >> - >> if (reg != NULL >> && (reggroup == save_reggroup || reggroup == restore_reggroup)) >> return reg->save_restore; >> >> + if (reg != NULL && !reg->group.empty ()) >> + return (reg->group == reggroup->name ()); >> + >> return -1; >> } >> > > Yeah, this is a hard one. Unfortunately the outcome then was that we > have a number of debugging stubs out in the open that advertise xml's > with less-than-correct group information. > > I think even gdb, to this day, is guilty of that. > > 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. I took another look at this today. When you talk about the 'fpscr' register, in features/arm/arm-vfpv{2,3}.xml the register of type 'int', so would be in the general group, but is placed into the 'float' group with a line like: <reg name="fpscr" bitsize="32" type="int" group="float"/> So, to me, fpscr should show up for: info registers all info registers float But not for: info registers info registers general When you mention existing stubs in the wild that "advertise xml's with less-than-correct group information." -- are these stubs sending something different to the above? Or are they just copying that above? If we did want to retain the above functionality, then we can just set_gdbarch_register_reggroup_p to point to this (untested) function: static int arm_register_reggroup_p (struct gdbarch *gdbarch, int regnum, const struct reggroup *reggroup) { if (regnum == ARM_FPSCR_REGNUM && reggroup == general_reggroup) reggroup = float_reggroup; int ret = tdesc_register_in_reggroup_p (gdbarch, regnum, reggroup); if (ret != -1) return ret; return default_register_reggroup_p (gdbarch, regno, reggroup); } Thanks, Andrew ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gdb/tdesc: Don't assign custom-group tdesc registers to 'general' 2023-09-12 13:48 ` Andrew Burgess @ 2023-09-12 15:10 ` Luis Machado 0 siblings, 0 replies; 13+ messages in thread From: Luis Machado @ 2023-09-12 15:10 UTC (permalink / raw) To: Andrew Burgess, Ciaran Woodward, gdb-patches On 9/12/23 14:48, Andrew Burgess wrote: > Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes: > >> Hi, >> >> On 9/7/23 18:07, Ciaran Woodward wrote: >>> The 'Target Description' mechanism in GDB enables the target to >>> supply the full set of registers available on the system to gdb >>> in an XML format. >>> >>> This format enables setting the 'group' of each register, such >>> that they can be queried using the 'info registers <group>' >>> mechanism. >>> >>> However prior to this change, even if a register was explicitly >>> assigned to a group, it would still show up in the >>> 'info registers general' report. This is unexpected, and also >>> disagrees with the comment above the tdesc_register_in_reggroup_p >>> function, which says that '-1' should be returned if the register >>> group is not-known, not the register group is known, but differs. >>> >>> There was a previous change that did address this issue in >>> aa66aac47b4dd38f9524ddb5546c08cc09930d37 >>> but it also caused registers with *no* group in the target >>> description to be removed from 'general', so it was reverted in >>> 440cf44eb0f70830b8d8ac35289f84129c7a35c1 >>> as that behaviour was used by some targets. >>> >>> The change in this commit enhances the usefulness of the tdesc >>> 'group' attribute for adding system configuration registers, >>> of which there may be hundreds - very inconvenient to request >>> and print on every 'info registers' call. >>> --- >>> >>> Email archive link to discussion of previously referenced patch: >>> https://inbox.sourceware.org/gdb-patches/20200120155315.30333-1-shahab.vahedi@gmail.com/T/#m0ce2983153b2f482b66461ed6b97c4b287b09a89 >>> >>> gdb/target-descriptions.c | 7 +++---- >>> 1 file changed, 3 insertions(+), 4 deletions(-) >>> >>> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c >>> index cdedf88c793..c1bb03c3adf 100644 >>> --- a/gdb/target-descriptions.c >>> +++ b/gdb/target-descriptions.c >>> @@ -954,14 +954,13 @@ tdesc_register_in_reggroup_p (struct gdbarch *gdbarch, int regno, >>> { >>> struct tdesc_reg *reg = tdesc_find_register (gdbarch, regno); >>> >>> - if (reg != NULL && !reg->group.empty () >>> - && (reg->group == reggroup->name ())) >>> - return 1; >>> - >>> if (reg != NULL >>> && (reggroup == save_reggroup || reggroup == restore_reggroup)) >>> return reg->save_restore; >>> >>> + if (reg != NULL && !reg->group.empty ()) >>> + return (reg->group == reggroup->name ()); >>> + >>> return -1; >>> } >>> >> >> Yeah, this is a hard one. Unfortunately the outcome then was that we >> have a number of debugging stubs out in the open that advertise xml's >> with less-than-correct group information. >> >> I think even gdb, to this day, is guilty of that. >> >> 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. > > I took another look at this today. When you talk about the 'fpscr' > register, in features/arm/arm-vfpv{2,3}.xml the register of type 'int', > so would be in the general group, but is placed into the 'float' group > with a line like: > > <reg name="fpscr" bitsize="32" type="int" group="float"/> > > So, to me, fpscr should show up for: > > info registers all > info registers float > > But not for: > > info registers > info registers general > Yeah. I'm not entirely sure how that came to be, but I suppose it gets displayed in "info registers" because of its type, even though it is in the float group. In any case, I don't think it is a big deal, and we could change/fix this. > When you mention existing stubs in the wild that "advertise xml's with > less-than-correct group information." -- are these stubs sending > something different to the above? Or are they just copying that above? > For 32-bit Arm I think there are both cases (the same as gdb and others somewhat different). But it is difficult to get our hands into all of those examples. > If we did want to retain the above functionality, then we can just > set_gdbarch_register_reggroup_p to point to this (untested) function: > > static int > arm_register_reggroup_p (struct gdbarch *gdbarch, int regnum, > const struct reggroup *reggroup) > { > if (regnum == ARM_FPSCR_REGNUM && reggroup == general_reggroup) > reggroup = float_reggroup; > > int ret = tdesc_register_in_reggroup_p (gdbarch, regnum, reggroup); > if (ret != -1) > return ret; > return default_register_reggroup_p (gdbarch, regno, reggroup); > } I suppose that'd work. To me, the fact fpscr is showing up in the general display looks like an oversight. This might also be a good opportunity to make the register group information in the xml's more accurate. > > Thanks, > Andrew > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gdb/tdesc: Don't assign custom-group tdesc registers to 'general' 2023-09-07 17:07 [PATCH] gdb/tdesc: Don't assign custom-group tdesc registers to 'general' Ciaran Woodward 2023-09-07 17:47 ` Luis Machado @ 2023-09-11 10:52 ` Andrew Burgess 2023-09-11 18:33 ` Ciaran Woodward 2023-09-13 11:41 ` [PATCH v2] " Ciaran Woodward 2 siblings, 1 reply; 13+ messages in thread From: Andrew Burgess @ 2023-09-11 10:52 UTC (permalink / raw) To: Ciaran Woodward, gdb-patches Ciaran Woodward <ciaranwoodward@xmos.com> writes: > The 'Target Description' mechanism in GDB enables the target to > supply the full set of registers available on the system to gdb > in an XML format. > > This format enables setting the 'group' of each register, such > that they can be queried using the 'info registers <group>' > mechanism. > > However prior to this change, even if a register was explicitly > assigned to a group, it would still show up in the > 'info registers general' report. This is unexpected, and also > disagrees with the comment above the tdesc_register_in_reggroup_p > function, which says that '-1' should be returned if the register > group is not-known, not the register group is known, but differs. > > There was a previous change that did address this issue in > aa66aac47b4dd38f9524ddb5546c08cc09930d37 > but it also caused registers with *no* group in the target > description to be removed from 'general', so it was reverted in > 440cf44eb0f70830b8d8ac35289f84129c7a35c1 > as that behaviour was used by some targets. > > The change in this commit enhances the usefulness of the tdesc > 'group' attribute for adding system configuration registers, > of which there may be hundreds - very inconvenient to request > and print on every 'info registers' call. > --- > > Email archive link to discussion of previously referenced patch: > https://inbox.sourceware.org/gdb-patches/20200120155315.30333-1-shahab.vahedi@gmail.com/T/#m0ce2983153b2f482b66461ed6b97c4b287b09a89 > > gdb/target-descriptions.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c > index cdedf88c793..c1bb03c3adf 100644 > --- a/gdb/target-descriptions.c > +++ b/gdb/target-descriptions.c > @@ -954,14 +954,13 @@ tdesc_register_in_reggroup_p (struct gdbarch *gdbarch, int regno, > { > struct tdesc_reg *reg = tdesc_find_register (gdbarch, regno); > > - if (reg != NULL && !reg->group.empty () > - && (reg->group == reggroup->name ())) > - return 1; > - > if (reg != NULL > && (reggroup == save_reggroup || reggroup == restore_reggroup)) > return reg->save_restore; > > + if (reg != NULL && !reg->group.empty ()) > + return (reg->group == reggroup->name ()); > + > return -1; > } I haven't tested this, but looking at the series you referenced, I don't think the above change can be correct. If reggroup is all_reggroup, and a register has a group set, then this is going to return 0, right? If we then look at tdesc_register_reggroup_p, which calls the above function, we can see that it relies on the function you changed to return -1 in this case, at which point we fall through to default_register_reggroup_p, where all_reggroup is handled. Of course, this all depends on which target you're using. Some targets implement their own set_gdbarch_register_reggroup_p callback, and handle all_reggroup before calling tdesc_register_in_reggroup_p, but others (e.g. MIPS) don't do it this way, so I think your change will break them. Thanks, Andrew ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] gdb/tdesc: Don't assign custom-group tdesc registers to 'general' 2023-09-11 10:52 ` Andrew Burgess @ 2023-09-11 18:33 ` Ciaran Woodward 2023-09-12 13:33 ` Andrew Burgess 0 siblings, 1 reply; 13+ messages in thread From: Ciaran Woodward @ 2023-09-11 18:33 UTC (permalink / raw) To: Andrew Burgess, gdb-patches Hi Andrew, > I haven't tested this, but looking at the series you referenced, I don't > think the above change can be correct. If reggroup is all_reggroup, and > a register has a group set, then this is going to return 0, right? > > If we then look at tdesc_register_reggroup_p, which calls the above > function, we can see that it relies on the function you changed to > return -1 in this case, at which point we fall through to > default_register_reggroup_p, where all_reggroup is handled. > > Of course, this all depends on which target you're using. Some targets > implement their own set_gdbarch_register_reggroup_p callback, and handle > all_reggroup before calling tdesc_register_in_reggroup_p, but others > (e.g. MIPS) don't do it this way, so I think your change will break > them. Yes, you're right - good catch. I suppose the best route in a v2 patch would be to return -1 if the group is reggroup_all, so that the caller can handle it as it does currently. Thanks, Ciaran ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] gdb/tdesc: Don't assign custom-group tdesc registers to 'general' 2023-09-11 18:33 ` Ciaran Woodward @ 2023-09-12 13:33 ` Andrew Burgess 0 siblings, 0 replies; 13+ messages in thread From: Andrew Burgess @ 2023-09-12 13:33 UTC (permalink / raw) To: Ciaran Woodward, gdb-patches Ciaran Woodward <ciaranwoodward@xmos.com> writes: > Hi Andrew, > >> I haven't tested this, but looking at the series you referenced, I don't >> think the above change can be correct. If reggroup is all_reggroup, and >> a register has a group set, then this is going to return 0, right? >> >> If we then look at tdesc_register_reggroup_p, which calls the above >> function, we can see that it relies on the function you changed to >> return -1 in this case, at which point we fall through to >> default_register_reggroup_p, where all_reggroup is handled. >> >> Of course, this all depends on which target you're using. Some targets >> implement their own set_gdbarch_register_reggroup_p callback, and handle >> all_reggroup before calling tdesc_register_in_reggroup_p, but others >> (e.g. MIPS) don't do it this way, so I think your change will break >> them. > > Yes, you're right - good catch. I suppose the best route in a v2 patch > would be to return -1 if the group is reggroup_all, so that the caller > can handle it as it does currently. That would be fine, but you could also just return 1. The original patch that you referenced (aa66aac47b4dd38) changed the function tdesc_register_in_reggroup_p to this: int tdesc_register_in_reggroup_p (struct gdbarch *gdbarch, int regno, const struct reggroup *reggroup) { struct tdesc_reg *reg = tdesc_find_register (gdbarch, regno); if (reg != NULL) { if (reggroup == all_reggroup) return 1; else if (reggroup == save_reggroup || reggroup == restore_reggroup) return reg->save_restore; else return (int) (reg->group == reggroup->name ()); } return -1; } I think the only mistake here is the final 'else', which should have been: else if (!reg->group.empty ()) return (int) (reg->group == reggroup->name ()); With that fixed, I think this is pretty much what your new patch is going to look like. From there, I agree with your conclusions in [1], any problems where targets want registers to show up in extra groups, or to follow non-standard rules, should be solved by overriding the gdbarch_register_reggroup_p hook. [1] https://inbox.sourceware.org/gdb-patches/PAXPR09MB55834D4B7236002FCFCD4197B9F2A@PAXPR09MB5583.eurprd09.prod.outlook.com/T/#m6575b4be85f8f15751e7d419e0c3df9148cbb53a Thanks, Andrew ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] gdb/tdesc: Don't assign custom-group tdesc registers to 'general' 2023-09-07 17:07 [PATCH] gdb/tdesc: Don't assign custom-group tdesc registers to 'general' Ciaran Woodward 2023-09-07 17:47 ` Luis Machado 2023-09-11 10:52 ` Andrew Burgess @ 2023-09-13 11:41 ` Ciaran Woodward 2023-11-20 13:25 ` [PING][PATCH " Ciaran Woodward 2 siblings, 1 reply; 13+ messages in thread From: Ciaran Woodward @ 2023-09-13 11:41 UTC (permalink / raw) To: gdb-patches; +Cc: luis.machado, aburgess, Ciaran Woodward The 'Target Description' mechanism in GDB enables the target to supply the full set of registers available on the system to gdb in an XML format. This format enables setting the 'group' of each register, such that they can be queried using the 'info registers <group>' mechanism. However prior to this change, even if a register was explicitly assigned to a group, it would still show up in the 'info registers general' report. This is unexpected, and also disagrees with the comment above the tdesc_register_in_reggroup_p function, which says that '-1' should be returned if the register group is not-known, not the register group is known, but differs. There was a previous change that did address this issue in aa66aac47b4dd38f9524ddb5546c08cc09930d37 but it also caused registers with *no* group in the target description to be removed from 'general', so it was reverted in 440cf44eb0f70830b8d8ac35289f84129c7a35c1 as that behaviour was used by some targets. The change in this commit enhances the usefulness of the tdesc 'group' attribute for adding system configuration registers, of which there may be hundreds - very inconvenient to request and print on every 'info registers' call. --- gdb/target-descriptions.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c index cdedf88c793..6eb626b9c5a 100644 --- a/gdb/target-descriptions.c +++ b/gdb/target-descriptions.c @@ -954,13 +954,17 @@ tdesc_register_in_reggroup_p (struct gdbarch *gdbarch, int regno, { struct tdesc_reg *reg = tdesc_find_register (gdbarch, regno); - if (reg != NULL && !reg->group.empty () - && (reg->group == reggroup->name ())) + if (reg != NULL) + { + if (reggroup == all_reggroup) return 1; - if (reg != NULL - && (reggroup == save_reggroup || reggroup == restore_reggroup)) - return reg->save_restore; + if (reggroup == save_reggroup || reggroup == restore_reggroup) + return reg->save_restore; + + if (reg != NULL && !reg->group.empty ()) + return (reg->group == reggroup->name ()); + } return -1; } -- 2.25.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PING][PATCH v2] gdb/tdesc: Don't assign custom-group tdesc registers to 'general' 2023-09-13 11:41 ` [PATCH v2] " Ciaran Woodward @ 2023-11-20 13:25 ` Ciaran Woodward 2023-11-21 13:57 ` Luis Machado 0 siblings, 1 reply; 13+ messages in thread From: Ciaran Woodward @ 2023-11-20 13:25 UTC (permalink / raw) To: gdb-patches; +Cc: luis.machado, aburgess Ping Discussion of the V1 can be found here: https://patchwork.sourceware.org/project/gdb/patch/20230907170752.28639-1-ciaranwoodward@xmos.com/ From what I see, this change is complete & useful as-is, but opens the door for further useful work in future - to improve the tdesc XMLs. > -----Original Message----- > From: Ciaran Woodward > Sent: Wednesday, September 13, 2023 12:42 PM > To: gdb-patches@sourceware.org > Cc: luis.machado@arm.com; aburgess@redhat.com; Ciaran Woodward > <ciaranwoodward@xmos.com> > Subject: [PATCH v2] gdb/tdesc: Don't assign custom-group tdesc registers > to 'general' > > The 'Target Description' mechanism in GDB enables the target to > supply the full set of registers available on the system to gdb > in an XML format. > > This format enables setting the 'group' of each register, such > that they can be queried using the 'info registers <group>' > mechanism. > > However prior to this change, even if a register was explicitly > assigned to a group, it would still show up in the > 'info registers general' report. This is unexpected, and also > disagrees with the comment above the tdesc_register_in_reggroup_p > function, which says that '-1' should be returned if the register > group is not-known, not the register group is known, but differs. > > There was a previous change that did address this issue in > aa66aac47b4dd38f9524ddb5546c08cc09930d37 > but it also caused registers with *no* group in the target > description to be removed from 'general', so it was reverted in > 440cf44eb0f70830b8d8ac35289f84129c7a35c1 > as that behaviour was used by some targets. > > The change in this commit enhances the usefulness of the tdesc > 'group' attribute for adding system configuration registers, > of which there may be hundreds - very inconvenient to request > and print on every 'info registers' call. > --- > gdb/target-descriptions.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c > index cdedf88c793..6eb626b9c5a 100644 > --- a/gdb/target-descriptions.c > +++ b/gdb/target-descriptions.c > @@ -954,13 +954,17 @@ tdesc_register_in_reggroup_p (struct gdbarch > *gdbarch, int regno, > { > struct tdesc_reg *reg = tdesc_find_register (gdbarch, regno); > > - if (reg != NULL && !reg->group.empty () > - && (reg->group == reggroup->name ())) > + if (reg != NULL) > + { > + if (reggroup == all_reggroup) > return 1; > > - if (reg != NULL > - && (reggroup == save_reggroup || reggroup == restore_reggroup)) > - return reg->save_restore; > + if (reggroup == save_reggroup || reggroup == restore_reggroup) > + return reg->save_restore; > + > + if (reg != NULL && !reg->group.empty ()) > + return (reg->group == reggroup->name ()); > + } > > return -1; > } > -- > 2.25.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PING][PATCH v2] gdb/tdesc: Don't assign custom-group tdesc registers to 'general' 2023-11-20 13:25 ` [PING][PATCH " Ciaran Woodward @ 2023-11-21 13:57 ` Luis Machado 2023-11-21 18:17 ` Ciaran Woodward 0 siblings, 1 reply; 13+ messages in thread From: Luis Machado @ 2023-11-21 13:57 UTC (permalink / raw) To: Ciaran Woodward, gdb-patches; +Cc: aburgess On 11/20/23 13:25, Ciaran Woodward wrote: > Ping > > Discussion of the V1 can be found here: https://patchwork.sourceware.org/project/gdb/patch/20230907170752.28639-1-ciaranwoodward@xmos.com/ > > From what I see, this change is complete & useful as-is, but opens the door for further useful work in future - to improve the tdesc XMLs. > Options 1 and 3 from the v1 discussion look reasonable to me. For 3 it would be nice to establish some kind of versioning info so gdb can handle the xml the right way, and enforce group information. Say, if group information is missing for a "v2" of the XML format, warn/error out. For 1, I think it would be nice to cleanup gdb's target descriptions and add group information explicitly, to prevent this from happening. in the future. >> -----Original Message----- >> From: Ciaran Woodward >> Sent: Wednesday, September 13, 2023 12:42 PM >> To: gdb-patches@sourceware.org >> Cc: luis.machado@arm.com; aburgess@redhat.com; Ciaran Woodward >> <ciaranwoodward@xmos.com> >> Subject: [PATCH v2] gdb/tdesc: Don't assign custom-group tdesc registers >> to 'general' >> >> The 'Target Description' mechanism in GDB enables the target to >> supply the full set of registers available on the system to gdb >> in an XML format. >> >> This format enables setting the 'group' of each register, such >> that they can be queried using the 'info registers <group>' >> mechanism. >> >> However prior to this change, even if a register was explicitly >> assigned to a group, it would still show up in the >> 'info registers general' report. This is unexpected, and also >> disagrees with the comment above the tdesc_register_in_reggroup_p >> function, which says that '-1' should be returned if the register >> group is not-known, not the register group is known, but differs. >> >> There was a previous change that did address this issue in >> aa66aac47b4dd38f9524ddb5546c08cc09930d37 >> but it also caused registers with *no* group in the target >> description to be removed from 'general', so it was reverted in >> 440cf44eb0f70830b8d8ac35289f84129c7a35c1 >> as that behaviour was used by some targets. >> >> The change in this commit enhances the usefulness of the tdesc >> 'group' attribute for adding system configuration registers, >> of which there may be hundreds - very inconvenient to request >> and print on every 'info registers' call. >> --- >> gdb/target-descriptions.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c >> index cdedf88c793..6eb626b9c5a 100644 >> --- a/gdb/target-descriptions.c >> +++ b/gdb/target-descriptions.c >> @@ -954,13 +954,17 @@ tdesc_register_in_reggroup_p (struct gdbarch >> *gdbarch, int regno, >> { >> struct tdesc_reg *reg = tdesc_find_register (gdbarch, regno); >> >> - if (reg != NULL && !reg->group.empty () >> - && (reg->group == reggroup->name ())) >> + if (reg != NULL) >> + { >> + if (reggroup == all_reggroup) >> return 1; >> >> - if (reg != NULL >> - && (reggroup == save_reggroup || reggroup == restore_reggroup)) >> - return reg->save_restore; >> + if (reggroup == save_reggroup || reggroup == restore_reggroup) >> + return reg->save_restore; >> + >> + if (reg != NULL && !reg->group.empty ()) >> + return (reg->group == reggroup->name ()); >> + } >> >> return -1; >> } >> -- >> 2.25.1 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PING][PATCH v2] gdb/tdesc: Don't assign custom-group tdesc registers to 'general' 2023-11-21 13:57 ` Luis Machado @ 2023-11-21 18:17 ` Ciaran Woodward 0 siblings, 0 replies; 13+ messages in thread From: Ciaran Woodward @ 2023-11-21 18:17 UTC (permalink / raw) To: Luis Machado, gdb-patches; +Cc: aburgess Hi, Thanks for your reply > > Discussion of the V1 can be found here: > https://patchwork.sourceware.org/project/gdb/patch/20230907170752.28639-1- > ciaranwoodward@xmos.com/ > > > > Options 1 and 3 from the v1 discussion look reasonable to me. For 3 it > would be nice to establish some kind of > versioning info so gdb can handle the xml the right way, and enforce group > information. Say, if group information > is missing for a "v2" of the XML format, warn/error out. I'm leaning towards 1, but it isn't clear to me which of the listed registers are "quirks of the previous output which we don't care about" and which are "important groupings we want to maintain". Going back to Andrew's example, if we look only at ARM's fpscr, which is defined: <reg name="fpscr" bitsize="32" type="int" group="float"/> Both pre- and post- patch, it would show in: info registers all info registers float Pre-patch, but not post-patch, it would show in: info registers info registers general To me, the post-patch behaviour seems like an improvement, but I'm not close enough with the architecture to know that for sure. There are a bunch of similar situations in other architectures (identified in an earlier email). If you think I should maintain the pre-patch behaviour, I'm happy to go add the fix to all the individual architectures. (it also raises the question of whether we should allow multi-grouping for custom groups too) > For 1, I think it would be nice to cleanup gdb's target descriptions and > add group information explicitly, to > prevent this from happening. in the future. Post-patch, the behaviour can be expressed as: If the group is explicitly stated in the target-description, add the register to that group. If the group is not stated in the target-description, add the register to a group based on its type. It's the "If the group is not stated in the target-description" which is added by this patch. Which I think already makes sense - the rationale being that there is never a reason to add a register to no group, so having a default is reasonable. Therefore I don't think any change to the existing XMLs is necessary. Pre-patch, even when you _did_ specify an explicit group, gdb would add it to the group based on type anyway. Please let me know if I'm misunderstanding the rationale for your request. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-11-21 18:17 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-09-07 17:07 [PATCH] gdb/tdesc: Don't assign custom-group tdesc registers to 'general' Ciaran Woodward 2023-09-07 17:47 ` Luis Machado 2023-09-11 10:31 ` Andrew Burgess 2023-09-11 18:41 ` Ciaran Woodward 2023-09-12 13:48 ` Andrew Burgess 2023-09-12 15:10 ` Luis Machado 2023-09-11 10:52 ` Andrew Burgess 2023-09-11 18:33 ` Ciaran Woodward 2023-09-12 13:33 ` Andrew Burgess 2023-09-13 11:41 ` [PATCH v2] " Ciaran Woodward 2023-11-20 13:25 ` [PING][PATCH " Ciaran Woodward 2023-11-21 13:57 ` Luis Machado 2023-11-21 18:17 ` Ciaran Woodward
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).