* [PATCH] gdb: add gdbarch_stack_grows_down function @ 2024-05-09 14:59 Andrew Burgess 2024-05-09 19:43 ` Tom Tromey 0 siblings, 1 reply; 5+ messages in thread From: Andrew Burgess @ 2024-05-09 14:59 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess In another patch I'm working on I needed to ask: does the stack grow down, or grow up? Looking around I found in infcall.c some code where we needed to ask the same question, what we do there is ask: gdbarch_inner_than (gdbarch, 1, 2) which should do the job. However, I don't particularly like copying this, it feels like we're asking something slightly different that just happens to align with the question we're actually asking. I propose adding a new function `gdbarch_stack_grows_down`. This is not going to be a gdbarch method that can be overridden, instead, this will just call the gdbarch_inner_than function. We already have some gdbarch methods like this, checkout arch-utils.c for examples. I think it's now clearer what we're actually doing. There should be no user visible changes after this commit. --- gdb/gdbarch.h | 13 +++++++++++++ gdb/infcall.c | 10 ++++------ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h index 77d3406779f..70a6f43b06f 100644 --- a/gdb/gdbarch.h +++ b/gdb/gdbarch.h @@ -370,4 +370,17 @@ gdbarch_num_cooked_regs (gdbarch *arch) return gdbarch_num_regs (arch) + gdbarch_num_pseudo_regs (arch); } +/* Return true if stacks for ARCH grow down, otherwise return true. */ + +static inline bool +gdbarch_stack_grows_down (gdbarch *arch) +{ + /* The current assumption is that stacks either grow down, or they grow + up, so one of these checks should be true. */ + gdb_assert (gdbarch_inner_than (arch, 1, 2) + || gdbarch_inner_than (arch, 2, 1)); + + return gdbarch_inner_than (arch, 1, 2); +} + #endif diff --git a/gdb/infcall.c b/gdb/infcall.c index 23d5652dd21..edac9a74179 100644 --- a/gdb/infcall.c +++ b/gdb/infcall.c @@ -947,7 +947,7 @@ reserve_stack_space (const type *values_type, CORE_ADDR &sp) struct gdbarch *gdbarch = get_frame_arch (frame); CORE_ADDR addr = 0; - if (gdbarch_inner_than (gdbarch, 1, 2)) + if (gdbarch_stack_grows_down (gdbarch)) { /* Stack grows downward. Align STRUCT_ADDR and SP after making space. */ @@ -1128,7 +1128,7 @@ call_function_by_hand_dummy (struct value *function, address. AMD64 called that region the "red zone". Skip at least the "red zone" size before allocating any space on the stack. */ - if (gdbarch_inner_than (gdbarch, 1, 2)) + if (gdbarch_stack_grows_down (gdbarch)) sp -= gdbarch_frame_red_zone_size (gdbarch); else sp += gdbarch_frame_red_zone_size (gdbarch); @@ -1156,11 +1156,9 @@ call_function_by_hand_dummy (struct value *function, to pay :-). */ if (sp == old_sp) { - if (gdbarch_inner_than (gdbarch, 1, 2)) - /* Stack grows down. */ + if (gdbarch_stack_grows_down (gdbarch)) sp = gdbarch_frame_align (gdbarch, old_sp - 1); else - /* Stack grows up. */ sp = gdbarch_frame_align (gdbarch, old_sp + 1); } /* SP may have underflown address zero here from OLD_SP. Memory access @@ -1193,7 +1191,7 @@ call_function_by_hand_dummy (struct value *function, { CORE_ADDR lastval_addr = lastval->address (); - if (gdbarch_inner_than (gdbarch, 1, 2)) + if (gdbarch_stack_grows_down (gdbarch)) { gdb_assert (sp >= lastval_addr); sp = lastval_addr; base-commit: cba95c27876724059c3e99ea1857fb19b9cf8220 -- 2.25.4 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gdb: add gdbarch_stack_grows_down function 2024-05-09 14:59 [PATCH] gdb: add gdbarch_stack_grows_down function Andrew Burgess @ 2024-05-09 19:43 ` Tom Tromey 2024-05-10 8:54 ` Andrew Burgess 0 siblings, 1 reply; 5+ messages in thread From: Tom Tromey @ 2024-05-09 19:43 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches >>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes: Andrew> I propose adding a new function `gdbarch_stack_grows_down`. This is Andrew> not going to be a gdbarch method that can be overridden, instead, this Andrew> will just call the gdbarch_inner_than function. We already have some Andrew> gdbarch methods like this, checkout arch-utils.c for examples. This makes sense to me. Andrew> + /* The current assumption is that stacks either grow down, or they grow Andrew> + up, so one of these checks should be true. */ Andrew> + gdb_assert (gdbarch_inner_than (arch, 1, 2) Andrew> + || gdbarch_inner_than (arch, 2, 1)); I wonder if this would be better as a new all-arch self-test in gdbarch-selftests.c. Tom ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gdb: add gdbarch_stack_grows_down function 2024-05-09 19:43 ` Tom Tromey @ 2024-05-10 8:54 ` Andrew Burgess 2024-05-10 9:43 ` Tom Tromey 0 siblings, 1 reply; 5+ messages in thread From: Andrew Burgess @ 2024-05-10 8:54 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches Tom Tromey <tom@tromey.com> writes: >>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes: > > Andrew> I propose adding a new function `gdbarch_stack_grows_down`. This is > Andrew> not going to be a gdbarch method that can be overridden, instead, this > Andrew> will just call the gdbarch_inner_than function. We already have some > Andrew> gdbarch methods like this, checkout arch-utils.c for examples. > > This makes sense to me. > > Andrew> + /* The current assumption is that stacks either grow down, or they grow > Andrew> + up, so one of these checks should be true. */ > Andrew> + gdb_assert (gdbarch_inner_than (arch, 1, 2) > Andrew> + || gdbarch_inner_than (arch, 2, 1)); > > I wonder if this would be better as a new all-arch self-test in > gdbarch-selftests.c. Great idea. How about the update below? Thanks, Andrew --- commit aedb3394b8c59c1bce7ca48b5d1a5805071eea34 Author: Andrew Burgess <aburgess@redhat.com> Date: Sun May 5 11:00:04 2024 +0100 gdb: add gdbarch_stack_grows_down function In another patch I'm working on I needed to ask: does the stack grow down, or grow up? Looking around I found in infcall.c some code where we needed to ask the same question, what we do there is ask: gdbarch_inner_than (gdbarch, 1, 2) which should do the job. However, I don't particularly like copying this, it feels like we're asking something slightly different that just happens to align with the question we're actually asking. I propose adding a new function `gdbarch_stack_grows_down`. This is not going to be a gdbarch method that can be overridden, instead, this will just call the gdbarch_inner_than function. We already have some gdbarch methods like this, checkout arch-utils.c for examples. I think it's now clearer what we're actually doing. A new self-test ensures that all architectures have a stack that either grows down, or grows up. There should be no user visible changes after this commit. diff --git a/gdb/gdbarch-selftests.c b/gdb/gdbarch-selftests.c index 0dc0c500654..707012bcd0d 100644 --- a/gdb/gdbarch-selftests.c +++ b/gdb/gdbarch-selftests.c @@ -164,6 +164,20 @@ register_name_test (struct gdbarch *gdbarch) } } +/* Test gdbarch_stack_grows_down. Stacks must either grow down or up. */ + +static void +check_stack_growth (struct gdbarch *gdbarch) +{ + /* We don't call gdbarch_stack_grows_down here, instead we're testing the + implementation by calling gdbarch_inner_than. GDB assumes that stacks + either grow down or up (see uses of gdbarch_stack_grows_down), so one of + these needs to be true. */ + bool stack_grows = (gdbarch_inner_than (gdbarch, 1, 2) + || gdbarch_inner_than (gdbarch, 2, 1)); + SELF_CHECK (stack_grows); +} + } // namespace selftests void _initialize_gdbarch_selftests (); @@ -175,4 +189,7 @@ _initialize_gdbarch_selftests () selftests::register_test_foreach_arch ("register_name", selftests::register_name_test); + + selftests::register_test_foreach_arch ("stack_growth", + selftests::check_stack_growth); } diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h index 77d3406779f..5175ef79e5b 100644 --- a/gdb/gdbarch.h +++ b/gdb/gdbarch.h @@ -370,4 +370,12 @@ gdbarch_num_cooked_regs (gdbarch *arch) return gdbarch_num_regs (arch) + gdbarch_num_pseudo_regs (arch); } +/* Return true if stacks for ARCH grow down, otherwise return true. */ + +static inline bool +gdbarch_stack_grows_down (gdbarch *arch) +{ + return gdbarch_inner_than (arch, 1, 2); +} + #endif diff --git a/gdb/infcall.c b/gdb/infcall.c index 23d5652dd21..edac9a74179 100644 --- a/gdb/infcall.c +++ b/gdb/infcall.c @@ -947,7 +947,7 @@ reserve_stack_space (const type *values_type, CORE_ADDR &sp) struct gdbarch *gdbarch = get_frame_arch (frame); CORE_ADDR addr = 0; - if (gdbarch_inner_than (gdbarch, 1, 2)) + if (gdbarch_stack_grows_down (gdbarch)) { /* Stack grows downward. Align STRUCT_ADDR and SP after making space. */ @@ -1128,7 +1128,7 @@ call_function_by_hand_dummy (struct value *function, address. AMD64 called that region the "red zone". Skip at least the "red zone" size before allocating any space on the stack. */ - if (gdbarch_inner_than (gdbarch, 1, 2)) + if (gdbarch_stack_grows_down (gdbarch)) sp -= gdbarch_frame_red_zone_size (gdbarch); else sp += gdbarch_frame_red_zone_size (gdbarch); @@ -1156,11 +1156,9 @@ call_function_by_hand_dummy (struct value *function, to pay :-). */ if (sp == old_sp) { - if (gdbarch_inner_than (gdbarch, 1, 2)) - /* Stack grows down. */ + if (gdbarch_stack_grows_down (gdbarch)) sp = gdbarch_frame_align (gdbarch, old_sp - 1); else - /* Stack grows up. */ sp = gdbarch_frame_align (gdbarch, old_sp + 1); } /* SP may have underflown address zero here from OLD_SP. Memory access @@ -1193,7 +1191,7 @@ call_function_by_hand_dummy (struct value *function, { CORE_ADDR lastval_addr = lastval->address (); - if (gdbarch_inner_than (gdbarch, 1, 2)) + if (gdbarch_stack_grows_down (gdbarch)) { gdb_assert (sp >= lastval_addr); sp = lastval_addr; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gdb: add gdbarch_stack_grows_down function 2024-05-10 8:54 ` Andrew Burgess @ 2024-05-10 9:43 ` Tom Tromey 2024-05-10 13:31 ` Andrew Burgess 0 siblings, 1 reply; 5+ messages in thread From: Tom Tromey @ 2024-05-10 9:43 UTC (permalink / raw) To: Andrew Burgess; +Cc: Tom Tromey, gdb-patches >>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes: Andrew> Great idea. How about the update below? Andrew> + /* We don't call gdbarch_stack_grows_down here, instead we're testing the Andrew> + implementation by calling gdbarch_inner_than. GDB assumes that stacks Andrew> + either grow down or up (see uses of gdbarch_stack_grows_down), so one of Andrew> + these needs to be true. */ Andrew> + bool stack_grows = (gdbarch_inner_than (gdbarch, 1, 2) Andrew> + || gdbarch_inner_than (gdbarch, 2, 1)); It probably should check (gdbarch_inner_than() != 0) != (gdbarch_inner_than() != 0) to ensure that exactly one call returns true; with the !=0 being needed because this still returns int and not bool. Maybe that's too nit-picky though. TBH I doubt it would ever be an issue as is. Approved-By: Tom Tromey <tom@tromey.com> Tom ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gdb: add gdbarch_stack_grows_down function 2024-05-10 9:43 ` Tom Tromey @ 2024-05-10 13:31 ` Andrew Burgess 0 siblings, 0 replies; 5+ messages in thread From: Andrew Burgess @ 2024-05-10 13:31 UTC (permalink / raw) To: Tom Tromey; +Cc: Tom Tromey, gdb-patches Tom Tromey <tom@tromey.com> writes: >>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes: > > Andrew> Great idea. How about the update below? > > Andrew> + /* We don't call gdbarch_stack_grows_down here, instead we're testing the > Andrew> + implementation by calling gdbarch_inner_than. GDB assumes that stacks > Andrew> + either grow down or up (see uses of gdbarch_stack_grows_down), so one of > Andrew> + these needs to be true. */ > Andrew> + bool stack_grows = (gdbarch_inner_than (gdbarch, 1, 2) > Andrew> + || gdbarch_inner_than (gdbarch, 2, 1)); > > It probably should check > > (gdbarch_inner_than() != 0) != (gdbarch_inner_than() != 0) > > to ensure that exactly one call returns true; with the !=0 being needed > because this still returns int and not bool. > > Maybe that's too nit-picky though. TBH I doubt it would ever be an > issue as is. Please, pick those nits! I updated the patch inline with your suggestion, double check the selftest still passes, and pushed the patch below. Thanks, Andrew --- commit a4f76c0765a0b9c643dc91d5a398a1cd9519572b Author: Andrew Burgess <aburgess@redhat.com> Date: Sun May 5 11:00:04 2024 +0100 gdb: add gdbarch_stack_grows_down function In another patch I'm working on I needed to ask: does the stack grow down, or grow up? Looking around I found in infcall.c some code where we needed to ask the same question, what we do there is ask: gdbarch_inner_than (gdbarch, 1, 2) which should do the job. However, I don't particularly like copying this, it feels like we're asking something slightly different that just happens to align with the question we're actually asking. I propose adding a new function `gdbarch_stack_grows_down`. This is not going to be a gdbarch method that can be overridden, instead, this will just call the gdbarch_inner_than function. We already have some gdbarch methods like this, checkout arch-utils.c for examples. I think it's now clearer what we're actually doing. A new self-test ensures that all architectures have a stack that either grows down, or grows up. There should be no user visible changes after this commit. Approved-By: Tom Tromey <tom@tromey.com> diff --git a/gdb/gdbarch-selftests.c b/gdb/gdbarch-selftests.c index 0dc0c500654..db99fe08141 100644 --- a/gdb/gdbarch-selftests.c +++ b/gdb/gdbarch-selftests.c @@ -164,6 +164,21 @@ register_name_test (struct gdbarch *gdbarch) } } +/* Test gdbarch_stack_grows_down. Stacks must either grow down or up. */ + +static void +check_stack_growth (struct gdbarch *gdbarch) +{ + /* We don't call gdbarch_stack_grows_down here, instead we're testing the + implementation by calling gdbarch_inner_than. GDB assumes that stacks + either grow down or up (see uses of gdbarch_stack_grows_down), so exactly + one of these needs to be true. */ + bool stack_grows_down = gdbarch_inner_than (gdbarch, 1, 2) != 0; + bool stack_grows_up = gdbarch_inner_than (gdbarch, 2, 1) != 0; + + SELF_CHECK (stack_grows_up != stack_grows_down); +} + } // namespace selftests void _initialize_gdbarch_selftests (); @@ -175,4 +190,7 @@ _initialize_gdbarch_selftests () selftests::register_test_foreach_arch ("register_name", selftests::register_name_test); + + selftests::register_test_foreach_arch ("stack_growth", + selftests::check_stack_growth); } diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h index 77d3406779f..d4c6795a12b 100644 --- a/gdb/gdbarch.h +++ b/gdb/gdbarch.h @@ -370,4 +370,12 @@ gdbarch_num_cooked_regs (gdbarch *arch) return gdbarch_num_regs (arch) + gdbarch_num_pseudo_regs (arch); } +/* Return true if stacks for ARCH grow down, otherwise return true. */ + +static inline bool +gdbarch_stack_grows_down (gdbarch *arch) +{ + return gdbarch_inner_than (arch, 1, 2) != 0; +} + #endif diff --git a/gdb/infcall.c b/gdb/infcall.c index 23d5652dd21..edac9a74179 100644 --- a/gdb/infcall.c +++ b/gdb/infcall.c @@ -947,7 +947,7 @@ reserve_stack_space (const type *values_type, CORE_ADDR &sp) struct gdbarch *gdbarch = get_frame_arch (frame); CORE_ADDR addr = 0; - if (gdbarch_inner_than (gdbarch, 1, 2)) + if (gdbarch_stack_grows_down (gdbarch)) { /* Stack grows downward. Align STRUCT_ADDR and SP after making space. */ @@ -1128,7 +1128,7 @@ call_function_by_hand_dummy (struct value *function, address. AMD64 called that region the "red zone". Skip at least the "red zone" size before allocating any space on the stack. */ - if (gdbarch_inner_than (gdbarch, 1, 2)) + if (gdbarch_stack_grows_down (gdbarch)) sp -= gdbarch_frame_red_zone_size (gdbarch); else sp += gdbarch_frame_red_zone_size (gdbarch); @@ -1156,11 +1156,9 @@ call_function_by_hand_dummy (struct value *function, to pay :-). */ if (sp == old_sp) { - if (gdbarch_inner_than (gdbarch, 1, 2)) - /* Stack grows down. */ + if (gdbarch_stack_grows_down (gdbarch)) sp = gdbarch_frame_align (gdbarch, old_sp - 1); else - /* Stack grows up. */ sp = gdbarch_frame_align (gdbarch, old_sp + 1); } /* SP may have underflown address zero here from OLD_SP. Memory access @@ -1193,7 +1191,7 @@ call_function_by_hand_dummy (struct value *function, { CORE_ADDR lastval_addr = lastval->address (); - if (gdbarch_inner_than (gdbarch, 1, 2)) + if (gdbarch_stack_grows_down (gdbarch)) { gdb_assert (sp >= lastval_addr); sp = lastval_addr; ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-05-10 13:31 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-05-09 14:59 [PATCH] gdb: add gdbarch_stack_grows_down function Andrew Burgess 2024-05-09 19:43 ` Tom Tromey 2024-05-10 8:54 ` Andrew Burgess 2024-05-10 9:43 ` Tom Tromey 2024-05-10 13:31 ` Andrew Burgess
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).