| Message ID | 20250319182255.3096731-1-alex.bennee@linaro.org |
|---|---|
| Headers | show |
| Series | gdbstub: conversion to runtime endianess helpers | expand |
On 3/19/25 11:22, Alex Bennée wrote: > The aim of this work is to get rid of the endian aware helpers in > gdbstub/helpers.h which due to their use of tswap() mean target > gdbstubs need to be built multiple times. While this series doesn't > actually build each stub once it introduces a new helper - > gdb_get_register_value() which takes a MemOp which can describe the > current endian state of the system. This will be a lot easier to > dynamically feed from a helper function. > > The most complex example is PPC which has a helper called > ppc_maybe_bswap_register() which was doing this. > > This is still an RFC so I'm interested in feedback: > > - is the API sane > - can we avoid lots of (uint8_t *) casting? Even though the series has a good intent, the fact we make everything "generic" makes that we lose all guarantees we could get by relying on static typing, and that we had possibility of mistakes when passing size (which happened in patch 4 if I'm correct). And explicit casting comes as a *strong* warning about that. By patch 7, I was really feeling it's not a win vs explicit functions per size. If the goal of the series is to get rid of endian aware helpers, well, this can be fixed in the helpers themselves, without needing to introduce a "generic" size helper. Maybe we are trying to solve two different problems here? > - should we have a reverse helper for setting registers > > If this seems like the right approach I can have a go at more of the > frontends later. > > There are a few other misc clean-ups I did on the way which might be > worth cherry picking for 10.0 but I'll leave that up to maintainers. > > Alex. > > Alex Bennée (10): > include/gdbstub: fix include guard in commands.h > gdbstub: introduce target independent gdb register helper > target/arm: convert 32 bit gdbstub to new helper > target/arm: convert 64 bit gdbstub to new helper > target/ppc: expand comment on FP/VMX/VSX access functions > target/ppc: make ppc_maybe_bswap_register static > target/ppc: convert gdbstub to new helper (!hacky) > gdbstub: assert earlier in handle_read_all_regs > include/exec: fix assert in size_memop > target/microblaze: convert gdbstub to new helper > > include/exec/memop.h | 4 +- > include/gdbstub/commands.h | 2 +- > include/gdbstub/registers.h | 30 ++++++ > target/ppc/cpu.h | 8 +- > gdbstub/gdbstub.c | 24 ++++- > target/arm/gdbstub.c | 57 +++++++---- > target/arm/gdbstub64.c | 53 ++++++---- > target/microblaze/gdbstub.c | 44 ++++---- > target/ppc/gdbstub.c | 194 ++++++++++++++++++++---------------- > 9 files changed, 257 insertions(+), 159 deletions(-) > create mode 100644 include/gdbstub/registers.h >
On 3/20/25 12:52, Pierrick Bouvier wrote: > On 3/19/25 11:22, Alex Bennée wrote: >> The aim of this work is to get rid of the endian aware helpers in >> gdbstub/helpers.h which due to their use of tswap() mean target >> gdbstubs need to be built multiple times. While this series doesn't >> actually build each stub once it introduces a new helper - >> gdb_get_register_value() which takes a MemOp which can describe the >> current endian state of the system. This will be a lot easier to >> dynamically feed from a helper function. >> >> The most complex example is PPC which has a helper called >> ppc_maybe_bswap_register() which was doing this. >> >> This is still an RFC so I'm interested in feedback: >> >> - is the API sane >> - can we avoid lots of (uint8_t *) casting? > > Even though the series has a good intent, the fact we make everything > "generic" makes that we lose all guarantees we could get by relying on > static typing, and that we had possibility of mistakes when passing size > (which happened in patch 4 if I'm correct). And explicit casting comes > as a *strong* warning about that. > > By patch 7, I was really feeling it's not a win vs explicit functions > per size. > > If the goal of the series is to get rid of endian aware helpers, well, > this can be fixed in the helpers themselves, without needing to > introduce a "generic" size helper. Maybe we are trying to solve two > different problems here? > >> - should we have a reverse helper for setting registers >> >> If this seems like the right approach I can have a go at more of the >> frontends later. >> Looking at include/gdbstub/helpers.h, gdb_get_reg128 can be solved by using target_words_bigendian() instead of TARGET_BIG_ENDIAN, which is already what tswap primitives are doing. For gdb_get_regl, I would get rid of it completely, by editing all targets gdbstub.c, and replacing with gdb_get_reg32 or gdb_get_reg64 explicit calls. ppc is a very naughty boy, because registers are defined as target_ulong, while other arch use fixed types. The solution might be as simple as changing ppc registers definition to uint64_t. If it's too complicated, you can postpone the problem by leaving gdb_get_regl defined only in ppc gdbstub, and clean up all other arch. Thanks to static typing, it will be easy to spot a wrong gdb_get_regl conversion, so it's a no-risk operaton. For ldtul_p, ldtul_le_p, and ldtul_be_p, it's a similar game. It's harder because only return type will differ, and you might miss occurences. A safe way could be to replace ldtul_p by call to a function taking return value through pointer in parameter. This way, you can replace easily with l and q variants, without any risk off implicit conversion. And for the one left depending on target_ulong/target_long, leave that for now, and move ldtul*_p to a target-helper.h included only for archs needing this. >> There are a few other misc clean-ups I did on the way which might be >> worth cherry picking for 10.0 but I'll leave that up to maintainers. >> >> Alex. >> >> Alex Bennée (10): >> include/gdbstub: fix include guard in commands.h >> gdbstub: introduce target independent gdb register helper >> target/arm: convert 32 bit gdbstub to new helper >> target/arm: convert 64 bit gdbstub to new helper >> target/ppc: expand comment on FP/VMX/VSX access functions >> target/ppc: make ppc_maybe_bswap_register static >> target/ppc: convert gdbstub to new helper (!hacky) >> gdbstub: assert earlier in handle_read_all_regs >> include/exec: fix assert in size_memop >> target/microblaze: convert gdbstub to new helper >> >> include/exec/memop.h | 4 +- >> include/gdbstub/commands.h | 2 +- >> include/gdbstub/registers.h | 30 ++++++ >> target/ppc/cpu.h | 8 +- >> gdbstub/gdbstub.c | 24 ++++- >> target/arm/gdbstub.c | 57 +++++++---- >> target/arm/gdbstub64.c | 53 ++++++---- >> target/microblaze/gdbstub.c | 44 ++++---- >> target/ppc/gdbstub.c | 194 ++++++++++++++++++++---------------- >> 9 files changed, 257 insertions(+), 159 deletions(-) >> create mode 100644 include/gdbstub/registers.h >> >
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: > On 3/19/25 11:22, Alex Bennée wrote: >> The aim of this work is to get rid of the endian aware helpers in >> gdbstub/helpers.h which due to their use of tswap() mean target >> gdbstubs need to be built multiple times. While this series doesn't >> actually build each stub once it introduces a new helper - >> gdb_get_register_value() which takes a MemOp which can describe the >> current endian state of the system. This will be a lot easier to >> dynamically feed from a helper function. >> The most complex example is PPC which has a helper called >> ppc_maybe_bswap_register() which was doing this. >> This is still an RFC so I'm interested in feedback: >> - is the API sane >> - can we avoid lots of (uint8_t *) casting? > > Even though the series has a good intent, the fact we make everything > "generic" makes that we lose all guarantees we could get by relying on > static typing, and that we had possibility of mistakes when passing > size (which happened in patch 4 if I'm correct). And explicit casting > comes as a *strong* warning about that. > > By patch 7, I was really feeling it's not a win vs explicit functions > per size. > > If the goal of the series is to get rid of endian aware helpers, well, > this can be fixed in the helpers themselves, without needing to > introduce a "generic" size helper. Maybe we are trying to solve two > different problems here? It did seem natural that if you were defining a MemOp you would use all of it rather than only its endian definition. But you are right we could introduce the same helpers with a bool flag for endianess. Maybe we should have fully formed mops and just assert in the helper: gdb_get_reg32(MemOp op, GByteArray *buf, uint32_t val) { g_assert(op & MO_SIZE == MO_32); gdb_get_register_value(op, buf, &val); } I was also trying to avoid over boilerplating the code. > >> - should we have a reverse helper for setting registers >> If this seems like the right approach I can have a go at more of the >> frontends later. >> There are a few other misc clean-ups I did on the way which might be >> worth cherry picking for 10.0 but I'll leave that up to maintainers. >> Alex. >> Alex Bennée (10): >> include/gdbstub: fix include guard in commands.h >> gdbstub: introduce target independent gdb register helper >> target/arm: convert 32 bit gdbstub to new helper >> target/arm: convert 64 bit gdbstub to new helper >> target/ppc: expand comment on FP/VMX/VSX access functions >> target/ppc: make ppc_maybe_bswap_register static >> target/ppc: convert gdbstub to new helper (!hacky) >> gdbstub: assert earlier in handle_read_all_regs >> include/exec: fix assert in size_memop >> target/microblaze: convert gdbstub to new helper >> include/exec/memop.h | 4 +- >> include/gdbstub/commands.h | 2 +- >> include/gdbstub/registers.h | 30 ++++++ >> target/ppc/cpu.h | 8 +- >> gdbstub/gdbstub.c | 24 ++++- >> target/arm/gdbstub.c | 57 +++++++---- >> target/arm/gdbstub64.c | 53 ++++++---- >> target/microblaze/gdbstub.c | 44 ++++---- >> target/ppc/gdbstub.c | 194 ++++++++++++++++++++---------------- >> 9 files changed, 257 insertions(+), 159 deletions(-) >> create mode 100644 include/gdbstub/registers.h >>
On 20/3/25 21:16, Pierrick Bouvier wrote: > On 3/20/25 12:52, Pierrick Bouvier wrote: >> On 3/19/25 11:22, Alex Bennée wrote: >>> The aim of this work is to get rid of the endian aware helpers in >>> gdbstub/helpers.h which due to their use of tswap() mean target >>> gdbstubs need to be built multiple times. While this series doesn't >>> actually build each stub once it introduces a new helper - >>> gdb_get_register_value() which takes a MemOp which can describe the >>> current endian state of the system. This will be a lot easier to >>> dynamically feed from a helper function. >>> >>> The most complex example is PPC which has a helper called >>> ppc_maybe_bswap_register() which was doing this. >>> >>> This is still an RFC so I'm interested in feedback: >>> >>> - is the API sane >>> - can we avoid lots of (uint8_t *) casting? >> >> Even though the series has a good intent, the fact we make everything >> "generic" makes that we lose all guarantees we could get by relying on >> static typing, and that we had possibility of mistakes when passing size >> (which happened in patch 4 if I'm correct). And explicit casting comes >> as a *strong* warning about that. >> >> By patch 7, I was really feeling it's not a win vs explicit functions >> per size. >> >> If the goal of the series is to get rid of endian aware helpers, well, >> this can be fixed in the helpers themselves, without needing to >> introduce a "generic" size helper. Maybe we are trying to solve two >> different problems here? >> >>> - should we have a reverse helper for setting registers >>> >>> If this seems like the right approach I can have a go at more of the >>> frontends later. >>> > > Looking at include/gdbstub/helpers.h, gdb_get_reg128 can be solved by > using target_words_bigendian() instead of TARGET_BIG_ENDIAN, which is > already what tswap primitives are doing. We'll need to eventually remove target_words_bigendian(), so that'd just be postponing that.
On 3/21/25 06:02, Philippe Mathieu-Daudé wrote: > On 20/3/25 21:16, Pierrick Bouvier wrote: >> On 3/20/25 12:52, Pierrick Bouvier wrote: >>> On 3/19/25 11:22, Alex Bennée wrote: >>>> The aim of this work is to get rid of the endian aware helpers in >>>> gdbstub/helpers.h which due to their use of tswap() mean target >>>> gdbstubs need to be built multiple times. While this series doesn't >>>> actually build each stub once it introduces a new helper - >>>> gdb_get_register_value() which takes a MemOp which can describe the >>>> current endian state of the system. This will be a lot easier to >>>> dynamically feed from a helper function. >>>> >>>> The most complex example is PPC which has a helper called >>>> ppc_maybe_bswap_register() which was doing this. >>>> >>>> This is still an RFC so I'm interested in feedback: >>>> >>>> - is the API sane >>>> - can we avoid lots of (uint8_t *) casting? >>> >>> Even though the series has a good intent, the fact we make everything >>> "generic" makes that we lose all guarantees we could get by relying on >>> static typing, and that we had possibility of mistakes when passing size >>> (which happened in patch 4 if I'm correct). And explicit casting comes >>> as a *strong* warning about that. >>> >>> By patch 7, I was really feeling it's not a win vs explicit functions >>> per size. >>> >>> If the goal of the series is to get rid of endian aware helpers, well, >>> this can be fixed in the helpers themselves, without needing to >>> introduce a "generic" size helper. Maybe we are trying to solve two >>> different problems here? >>> >>>> - should we have a reverse helper for setting registers >>>> >>>> If this seems like the right approach I can have a go at more of the >>>> frontends later. >>>> >> >> Looking at include/gdbstub/helpers.h, gdb_get_reg128 can be solved by >> using target_words_bigendian() instead of TARGET_BIG_ENDIAN, which is >> already what tswap primitives are doing. > > We'll need to eventually remove target_words_bigendian(), so that'd just > be postponing that. > It seemed to me that one of the goal of current shared work on single binary is to be able to check those kind of things at runtime. So I don't see how we can plan to remove target_words_bigendian(), or an equivalent. If you mean we'll replace this call with another name/api, I don't see it as a problem to use it for now. It's upstream and works.
On 3/21/25 04:46, Alex Bennée wrote: > Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: > >> On 3/19/25 11:22, Alex Bennée wrote: >>> The aim of this work is to get rid of the endian aware helpers in >>> gdbstub/helpers.h which due to their use of tswap() mean target >>> gdbstubs need to be built multiple times. While this series doesn't >>> actually build each stub once it introduces a new helper - >>> gdb_get_register_value() which takes a MemOp which can describe the >>> current endian state of the system. This will be a lot easier to >>> dynamically feed from a helper function. >>> The most complex example is PPC which has a helper called >>> ppc_maybe_bswap_register() which was doing this. >>> This is still an RFC so I'm interested in feedback: >>> - is the API sane >>> - can we avoid lots of (uint8_t *) casting? >> >> Even though the series has a good intent, the fact we make everything >> "generic" makes that we lose all guarantees we could get by relying on >> static typing, and that we had possibility of mistakes when passing >> size (which happened in patch 4 if I'm correct). And explicit casting >> comes as a *strong* warning about that. >> >> By patch 7, I was really feeling it's not a win vs explicit functions >> per size. >> >> If the goal of the series is to get rid of endian aware helpers, well, >> this can be fixed in the helpers themselves, without needing to >> introduce a "generic" size helper. Maybe we are trying to solve two >> different problems here? > > It did seem natural that if you were defining a MemOp you would use all > of it rather than only its endian definition. But you are right we could > introduce the same helpers with a bool flag for endianess. > Defining MemOp and passing is ok, but loosing static typing guarantees is wrong in my opinion. C is already on the weak side regarding typing, we don't need to "void*"ize things more instead of replacing the calls with correct variants. > Maybe we should have fully formed mops and just assert in the helper: > > gdb_get_reg32(MemOp op, GByteArray *buf, uint32_t val) { > g_assert(op & MO_SIZE == MO_32); > gdb_get_register_value(op, buf, &val); > } > > I was also trying to avoid over boilerplating the code. > Adding proper functions definition instead of macros, and eliminating ifdefs is not really boilerplate. Adding casts to loosen type system is not a win versus that. >> >>> - should we have a reverse helper for setting registers >>> If this seems like the right approach I can have a go at more of the >>> frontends later. >>> There are a few other misc clean-ups I did on the way which might be >>> worth cherry picking for 10.0 but I'll leave that up to maintainers. >>> Alex. >>> Alex Bennée (10): >>> include/gdbstub: fix include guard in commands.h >>> gdbstub: introduce target independent gdb register helper >>> target/arm: convert 32 bit gdbstub to new helper >>> target/arm: convert 64 bit gdbstub to new helper >>> target/ppc: expand comment on FP/VMX/VSX access functions >>> target/ppc: make ppc_maybe_bswap_register static >>> target/ppc: convert gdbstub to new helper (!hacky) >>> gdbstub: assert earlier in handle_read_all_regs >>> include/exec: fix assert in size_memop >>> target/microblaze: convert gdbstub to new helper >>> include/exec/memop.h | 4 +- >>> include/gdbstub/commands.h | 2 +- >>> include/gdbstub/registers.h | 30 ++++++ >>> target/ppc/cpu.h | 8 +- >>> gdbstub/gdbstub.c | 24 ++++- >>> target/arm/gdbstub.c | 57 +++++++---- >>> target/arm/gdbstub64.c | 53 ++++++---- >>> target/microblaze/gdbstub.c | 44 ++++---- >>> target/ppc/gdbstub.c | 194 ++++++++++++++++++++---------------- >>> 9 files changed, 257 insertions(+), 159 deletions(-) >>> create mode 100644 include/gdbstub/registers.h >>> >
On 21/3/25 18:31, Pierrick Bouvier wrote: > On 3/21/25 04:46, Alex Bennée wrote: >> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: >> >>> On 3/19/25 11:22, Alex Bennée wrote: >>>> The aim of this work is to get rid of the endian aware helpers in >>>> gdbstub/helpers.h which due to their use of tswap() mean target >>>> gdbstubs need to be built multiple times. While this series doesn't >>>> actually build each stub once it introduces a new helper - >>>> gdb_get_register_value() which takes a MemOp which can describe the >>>> current endian state of the system. This will be a lot easier to >>>> dynamically feed from a helper function. >>>> The most complex example is PPC which has a helper called >>>> ppc_maybe_bswap_register() which was doing this. >>>> This is still an RFC so I'm interested in feedback: >>>> - is the API sane >>>> - can we avoid lots of (uint8_t *) casting? >>> >>> Even though the series has a good intent, the fact we make everything >>> "generic" makes that we lose all guarantees we could get by relying on >>> static typing, and that we had possibility of mistakes when passing >>> size (which happened in patch 4 if I'm correct). And explicit casting >>> comes as a *strong* warning about that. >>> >>> By patch 7, I was really feeling it's not a win vs explicit functions >>> per size. >>> >>> If the goal of the series is to get rid of endian aware helpers, well, >>> this can be fixed in the helpers themselves, without needing to >>> introduce a "generic" size helper. Maybe we are trying to solve two >>> different problems here? >> >> It did seem natural that if you were defining a MemOp you would use all >> of it rather than only its endian definition. But you are right we could >> introduce the same helpers with a bool flag for endianess. >> > > Defining MemOp and passing is ok, but loosing static typing guarantees > is wrong in my opinion. C is already on the weak side regarding typing, > we don't need to "void*"ize things more instead of replacing the calls > with correct variants. > >> Maybe we should have fully formed mops and just assert in the helper: >> >> gdb_get_reg32(MemOp op, GByteArray *buf, uint32_t val) { >> g_assert(op & MO_SIZE == MO_32); >> gdb_get_register_value(op, buf, &val); >> } >> >> I was also trying to avoid over boilerplating the code. >> > > Adding proper functions definition instead of macros, and eliminating > ifdefs is not really boilerplate. In another thread Richard said for these cases we should use _Generic() more. > Adding casts to loosen type system is not a win versus that. Agreed.
On 3/23/25 08:41, Philippe Mathieu-Daudé wrote: > On 21/3/25 18:31, Pierrick Bouvier wrote: >> >> Adding proper functions definition instead of macros, and eliminating >> ifdefs is not really boilerplate. > > In another thread Richard said for these cases we should use _Generic() > more. > I was thinking about suggesting it when I first answered on this thread. However, I realized it doesn't really help us towards our end goal to unify variants of a compilation unit. _Generic is just a way to have a compile time switch(typeof(x)) { case type_a: ... case type_b: ... } This is, in some ways, what our existing macros do here, based on TARGET_LONG_BITS. Some C compilers targetting embedded systems support sizeof on integral types in the preprocessor to support this kind of case before _Generic existed. It can replace macros based on static types, but it doesn't make much more towards eliminating target type dependency. It could still be used here though, to avoid the need to patch all targets using gdb register macros, that already used fixed types. But since our list of call sites is not so huge, I would be in favor to fix the code directly, instead of relying on _Generic. But I'm open to that if we collectively think it's a better way. >> Adding casts to loosen type system is not a win versus that. > > Agreed.