* [kvm-unit-tests PATCH 03/13] treewide: lib/stack: Fix backtrace [not found] <20240228150416.248948-15-andrew.jones@linux.dev> @ 2024-02-28 15:04 ` Andrew Jones 2024-02-28 17:33 ` Claudio Imbrenda 2024-02-29 3:31 ` Nicholas Piggin 2024-02-28 15:04 ` [kvm-unit-tests PATCH 04/13] treewide: lib/stack: Make base_address arch specific Andrew Jones 1 sibling, 2 replies; 7+ messages in thread From: Andrew Jones @ 2024-02-28 15:04 UTC (permalink / raw) To: kvm, kvm-riscv Cc: lvivier, linux-s390, thuth, nrb, frankja, npiggin, kvmarm, pbonzini, imbrenda, linuxppc-dev We should never pass the result of __builtin_frame_address(0) to another function since the compiler is within its rights to pop the frame to which it points before making the function call, as may be done for tail calls. Nobody has complained about backtrace(), so likely all compilations have been inlining backtrace_frame(), not dropping the frame on the tail call, or nobody is looking at traces. However, for riscv, when built for EFI, it does drop the frame on the tail call, and it was noticed. Preemptively fix backtrace() for all architectures. Fixes: 52266791750d ("lib: backtrace printing") Signed-off-by: Andrew Jones <andrew.jones@linux.dev> --- lib/arm/stack.c | 13 +++++-------- lib/arm64/stack.c | 12 +++++------- lib/riscv/stack.c | 12 +++++------- lib/s390x/stack.c | 12 +++++------- lib/stack.h | 24 +++++++++++++++++------- lib/x86/stack.c | 12 +++++------- 6 files changed, 42 insertions(+), 43 deletions(-) diff --git a/lib/arm/stack.c b/lib/arm/stack.c index 7d081be7c6d0..66d18b47ea53 100644 --- a/lib/arm/stack.c +++ b/lib/arm/stack.c @@ -8,13 +8,16 @@ #include <libcflat.h> #include <stack.h> -int backtrace_frame(const void *frame, const void **return_addrs, - int max_depth) +int arch_backtrace_frame(const void *frame, const void **return_addrs, + int max_depth, bool current_frame) { static int walking; int depth; const unsigned long *fp = (unsigned long *)frame; + if (current_frame) + fp = __builtin_frame_address(0); + if (walking) { printf("RECURSIVE STACK WALK!!!\n"); return 0; @@ -33,9 +36,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, walking = 0; return depth; } - -int backtrace(const void **return_addrs, int max_depth) -{ - return backtrace_frame(__builtin_frame_address(0), - return_addrs, max_depth); -} diff --git a/lib/arm64/stack.c b/lib/arm64/stack.c index 82611f4b1815..f5eb57fd8892 100644 --- a/lib/arm64/stack.c +++ b/lib/arm64/stack.c @@ -8,7 +8,8 @@ extern char vector_stub_start, vector_stub_end; -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) +int arch_backtrace_frame(const void *frame, const void **return_addrs, + int max_depth, bool current_frame) { const void *fp = frame; static bool walking; @@ -17,6 +18,9 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) bool is_exception = false; unsigned long addr; + if (current_frame) + fp = __builtin_frame_address(0); + if (walking) { printf("RECURSIVE STACK WALK!!!\n"); return 0; @@ -54,9 +58,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) walking = false; return depth; } - -int backtrace(const void **return_addrs, int max_depth) -{ - return backtrace_frame(__builtin_frame_address(0), - return_addrs, max_depth); -} diff --git a/lib/riscv/stack.c b/lib/riscv/stack.c index 712a5478d547..d865594b9671 100644 --- a/lib/riscv/stack.c +++ b/lib/riscv/stack.c @@ -2,12 +2,16 @@ #include <libcflat.h> #include <stack.h> -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) +int arch_backtrace_frame(const void *frame, const void **return_addrs, + int max_depth, bool current_frame) { static bool walking; const unsigned long *fp = (unsigned long *)frame; int depth; + if (current_frame) + fp = __builtin_frame_address(0); + if (walking) { printf("RECURSIVE STACK WALK!!!\n"); return 0; @@ -24,9 +28,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) walking = false; return depth; } - -int backtrace(const void **return_addrs, int max_depth) -{ - return backtrace_frame(__builtin_frame_address(0), - return_addrs, max_depth); -} diff --git a/lib/s390x/stack.c b/lib/s390x/stack.c index 9f234a12adf6..d194f654e94d 100644 --- a/lib/s390x/stack.c +++ b/lib/s390x/stack.c @@ -14,11 +14,15 @@ #include <stack.h> #include <asm/arch_def.h> -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) +int arch_backtrace_frame(const void *frame, const void **return_addrs, + int max_depth, bool current_frame) { int depth = 0; struct stack_frame *stack = (struct stack_frame *)frame; + if (current_frame) + stack = __builtin_frame_address(0); + for (depth = 0; stack && depth < max_depth; depth++) { return_addrs[depth] = (void *)stack->grs[8]; stack = stack->back_chain; @@ -28,9 +32,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) return depth; } - -int backtrace(const void **return_addrs, int max_depth) -{ - return backtrace_frame(__builtin_frame_address(0), - return_addrs, max_depth); -} diff --git a/lib/stack.h b/lib/stack.h index 10fc2f793354..6edc84344b51 100644 --- a/lib/stack.h +++ b/lib/stack.h @@ -11,17 +11,27 @@ #include <asm/stack.h> #ifdef HAVE_ARCH_BACKTRACE_FRAME -extern int backtrace_frame(const void *frame, const void **return_addrs, - int max_depth); +extern int arch_backtrace_frame(const void *frame, const void **return_addrs, + int max_depth, bool current_frame); + +static inline int backtrace_frame(const void *frame, const void **return_addrs, + int max_depth) +{ + return arch_backtrace_frame(frame, return_addrs, max_depth, false); +} + +static inline int backtrace(const void **return_addrs, int max_depth) +{ + return arch_backtrace_frame(NULL, return_addrs, max_depth, true); +} #else -static inline int -backtrace_frame(const void *frame __unused, const void **return_addrs __unused, - int max_depth __unused) +extern int backtrace(const void **return_addrs, int max_depth); + +static inline int backtrace_frame(const void *frame, const void **return_addrs, + int max_depth) { return 0; } #endif -extern int backtrace(const void **return_addrs, int max_depth); - #endif diff --git a/lib/x86/stack.c b/lib/x86/stack.c index 5ecd97ce90b9..58ab6c4b293a 100644 --- a/lib/x86/stack.c +++ b/lib/x86/stack.c @@ -1,12 +1,16 @@ #include <libcflat.h> #include <stack.h> -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) +int arch_backtrace_frame(const void *frame, const void **return_addrs, + int max_depth, bool current_frame) { static int walking; int depth = 0; const unsigned long *bp = (unsigned long *) frame; + if (current_frame) + bp = __builtin_frame_address(0); + if (walking) { printf("RECURSIVE STACK WALK!!!\n"); return 0; @@ -23,9 +27,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) walking = 0; return depth; } - -int backtrace(const void **return_addrs, int max_depth) -{ - return backtrace_frame(__builtin_frame_address(0), return_addrs, - max_depth); -} -- 2.43.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [kvm-unit-tests PATCH 03/13] treewide: lib/stack: Fix backtrace 2024-02-28 15:04 ` [kvm-unit-tests PATCH 03/13] treewide: lib/stack: Fix backtrace Andrew Jones @ 2024-02-28 17:33 ` Claudio Imbrenda 2024-02-29 3:31 ` Nicholas Piggin 1 sibling, 0 replies; 7+ messages in thread From: Claudio Imbrenda @ 2024-02-28 17:33 UTC (permalink / raw) To: Andrew Jones Cc: lvivier, linux-s390, thuth, nrb, frankja, kvm, npiggin, kvm-riscv, kvmarm, pbonzini, linuxppc-dev On Wed, 28 Feb 2024 16:04:19 +0100 Andrew Jones <andrew.jones@linux.dev> wrote: > We should never pass the result of __builtin_frame_address(0) to > another function since the compiler is within its rights to pop the > frame to which it points before making the function call, as may be > done for tail calls. Nobody has complained about backtrace(), so > likely all compilations have been inlining backtrace_frame(), not > dropping the frame on the tail call, or nobody is looking at traces. > However, for riscv, when built for EFI, it does drop the frame on the > tail call, and it was noticed. Preemptively fix backtrace() for all > architectures. > > Fixes: 52266791750d ("lib: backtrace printing") > Signed-off-by: Andrew Jones <andrew.jones@linux.dev> Acked-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > --- > lib/arm/stack.c | 13 +++++-------- > lib/arm64/stack.c | 12 +++++------- > lib/riscv/stack.c | 12 +++++------- > lib/s390x/stack.c | 12 +++++------- > lib/stack.h | 24 +++++++++++++++++------- > lib/x86/stack.c | 12 +++++------- > 6 files changed, 42 insertions(+), 43 deletions(-) > > diff --git a/lib/arm/stack.c b/lib/arm/stack.c > index 7d081be7c6d0..66d18b47ea53 100644 > --- a/lib/arm/stack.c > +++ b/lib/arm/stack.c > @@ -8,13 +8,16 @@ > #include <libcflat.h> > #include <stack.h> > > -int backtrace_frame(const void *frame, const void **return_addrs, > - int max_depth) > +int arch_backtrace_frame(const void *frame, const void **return_addrs, > + int max_depth, bool current_frame) > { > static int walking; > int depth; > const unsigned long *fp = (unsigned long *)frame; > > + if (current_frame) > + fp = __builtin_frame_address(0); > + > if (walking) { > printf("RECURSIVE STACK WALK!!!\n"); > return 0; > @@ -33,9 +36,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, > walking = 0; > return depth; > } > - > -int backtrace(const void **return_addrs, int max_depth) > -{ > - return backtrace_frame(__builtin_frame_address(0), > - return_addrs, max_depth); > -} > diff --git a/lib/arm64/stack.c b/lib/arm64/stack.c > index 82611f4b1815..f5eb57fd8892 100644 > --- a/lib/arm64/stack.c > +++ b/lib/arm64/stack.c > @@ -8,7 +8,8 @@ > > extern char vector_stub_start, vector_stub_end; > > -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) > +int arch_backtrace_frame(const void *frame, const void **return_addrs, > + int max_depth, bool current_frame) > { > const void *fp = frame; > static bool walking; > @@ -17,6 +18,9 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) > bool is_exception = false; > unsigned long addr; > > + if (current_frame) > + fp = __builtin_frame_address(0); > + > if (walking) { > printf("RECURSIVE STACK WALK!!!\n"); > return 0; > @@ -54,9 +58,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) > walking = false; > return depth; > } > - > -int backtrace(const void **return_addrs, int max_depth) > -{ > - return backtrace_frame(__builtin_frame_address(0), > - return_addrs, max_depth); > -} > diff --git a/lib/riscv/stack.c b/lib/riscv/stack.c > index 712a5478d547..d865594b9671 100644 > --- a/lib/riscv/stack.c > +++ b/lib/riscv/stack.c > @@ -2,12 +2,16 @@ > #include <libcflat.h> > #include <stack.h> > > -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) > +int arch_backtrace_frame(const void *frame, const void **return_addrs, > + int max_depth, bool current_frame) > { > static bool walking; > const unsigned long *fp = (unsigned long *)frame; > int depth; > > + if (current_frame) > + fp = __builtin_frame_address(0); > + > if (walking) { > printf("RECURSIVE STACK WALK!!!\n"); > return 0; > @@ -24,9 +28,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) > walking = false; > return depth; > } > - > -int backtrace(const void **return_addrs, int max_depth) > -{ > - return backtrace_frame(__builtin_frame_address(0), > - return_addrs, max_depth); > -} > diff --git a/lib/s390x/stack.c b/lib/s390x/stack.c > index 9f234a12adf6..d194f654e94d 100644 > --- a/lib/s390x/stack.c > +++ b/lib/s390x/stack.c > @@ -14,11 +14,15 @@ > #include <stack.h> > #include <asm/arch_def.h> > > -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) > +int arch_backtrace_frame(const void *frame, const void **return_addrs, > + int max_depth, bool current_frame) > { > int depth = 0; > struct stack_frame *stack = (struct stack_frame *)frame; > > + if (current_frame) > + stack = __builtin_frame_address(0); > + > for (depth = 0; stack && depth < max_depth; depth++) { > return_addrs[depth] = (void *)stack->grs[8]; > stack = stack->back_chain; > @@ -28,9 +32,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) > > return depth; > } > - > -int backtrace(const void **return_addrs, int max_depth) > -{ > - return backtrace_frame(__builtin_frame_address(0), > - return_addrs, max_depth); > -} > diff --git a/lib/stack.h b/lib/stack.h > index 10fc2f793354..6edc84344b51 100644 > --- a/lib/stack.h > +++ b/lib/stack.h > @@ -11,17 +11,27 @@ > #include <asm/stack.h> > > #ifdef HAVE_ARCH_BACKTRACE_FRAME > -extern int backtrace_frame(const void *frame, const void **return_addrs, > - int max_depth); > +extern int arch_backtrace_frame(const void *frame, const void **return_addrs, > + int max_depth, bool current_frame); > + > +static inline int backtrace_frame(const void *frame, const void **return_addrs, > + int max_depth) > +{ > + return arch_backtrace_frame(frame, return_addrs, max_depth, false); > +} > + > +static inline int backtrace(const void **return_addrs, int max_depth) > +{ > + return arch_backtrace_frame(NULL, return_addrs, max_depth, true); > +} > #else > -static inline int > -backtrace_frame(const void *frame __unused, const void **return_addrs __unused, > - int max_depth __unused) > +extern int backtrace(const void **return_addrs, int max_depth); > + > +static inline int backtrace_frame(const void *frame, const void **return_addrs, > + int max_depth) > { > return 0; > } > #endif > > -extern int backtrace(const void **return_addrs, int max_depth); > - > #endif > diff --git a/lib/x86/stack.c b/lib/x86/stack.c > index 5ecd97ce90b9..58ab6c4b293a 100644 > --- a/lib/x86/stack.c > +++ b/lib/x86/stack.c > @@ -1,12 +1,16 @@ > #include <libcflat.h> > #include <stack.h> > > -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) > +int arch_backtrace_frame(const void *frame, const void **return_addrs, > + int max_depth, bool current_frame) > { > static int walking; > int depth = 0; > const unsigned long *bp = (unsigned long *) frame; > > + if (current_frame) > + bp = __builtin_frame_address(0); > + > if (walking) { > printf("RECURSIVE STACK WALK!!!\n"); > return 0; > @@ -23,9 +27,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) > walking = 0; > return depth; > } > - > -int backtrace(const void **return_addrs, int max_depth) > -{ > - return backtrace_frame(__builtin_frame_address(0), return_addrs, > - max_depth); > -} ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [kvm-unit-tests PATCH 03/13] treewide: lib/stack: Fix backtrace 2024-02-28 15:04 ` [kvm-unit-tests PATCH 03/13] treewide: lib/stack: Fix backtrace Andrew Jones 2024-02-28 17:33 ` Claudio Imbrenda @ 2024-02-29 3:31 ` Nicholas Piggin 2024-02-29 11:48 ` Andrew Jones 1 sibling, 1 reply; 7+ messages in thread From: Nicholas Piggin @ 2024-02-29 3:31 UTC (permalink / raw) To: Andrew Jones, kvm, kvm-riscv Cc: lvivier, linux-s390, thuth, nrb, frankja, kvmarm, pbonzini, imbrenda, linuxppc-dev On Thu Feb 29, 2024 at 1:04 AM AEST, Andrew Jones wrote: > We should never pass the result of __builtin_frame_address(0) to > another function since the compiler is within its rights to pop the > frame to which it points before making the function call, as may be > done for tail calls. Nobody has complained about backtrace(), so > likely all compilations have been inlining backtrace_frame(), not > dropping the frame on the tail call, or nobody is looking at traces. > However, for riscv, when built for EFI, it does drop the frame on the > tail call, and it was noticed. Preemptively fix backtrace() for all > architectures. > > Fixes: 52266791750d ("lib: backtrace printing") > Signed-off-by: Andrew Jones <andrew.jones@linux.dev> > --- > lib/arm/stack.c | 13 +++++-------- > lib/arm64/stack.c | 12 +++++------- > lib/riscv/stack.c | 12 +++++------- > lib/s390x/stack.c | 12 +++++------- > lib/stack.h | 24 +++++++++++++++++------- > lib/x86/stack.c | 12 +++++------- > 6 files changed, 42 insertions(+), 43 deletions(-) > > diff --git a/lib/arm/stack.c b/lib/arm/stack.c > index 7d081be7c6d0..66d18b47ea53 100644 > --- a/lib/arm/stack.c > +++ b/lib/arm/stack.c > @@ -8,13 +8,16 @@ > #include <libcflat.h> > #include <stack.h> > > -int backtrace_frame(const void *frame, const void **return_addrs, > - int max_depth) > +int arch_backtrace_frame(const void *frame, const void **return_addrs, > + int max_depth, bool current_frame) > { > static int walking; > int depth; > const unsigned long *fp = (unsigned long *)frame; > > + if (current_frame) > + fp = __builtin_frame_address(0); > + > if (walking) { > printf("RECURSIVE STACK WALK!!!\n"); > return 0; > @@ -33,9 +36,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, > walking = 0; > return depth; > } > - > -int backtrace(const void **return_addrs, int max_depth) > -{ > - return backtrace_frame(__builtin_frame_address(0), > - return_addrs, max_depth); > -} > diff --git a/lib/arm64/stack.c b/lib/arm64/stack.c > index 82611f4b1815..f5eb57fd8892 100644 > --- a/lib/arm64/stack.c > +++ b/lib/arm64/stack.c > @@ -8,7 +8,8 @@ > > extern char vector_stub_start, vector_stub_end; > > -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) > +int arch_backtrace_frame(const void *frame, const void **return_addrs, > + int max_depth, bool current_frame) > { > const void *fp = frame; > static bool walking; > @@ -17,6 +18,9 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) > bool is_exception = false; > unsigned long addr; > > + if (current_frame) > + fp = __builtin_frame_address(0); > + > if (walking) { > printf("RECURSIVE STACK WALK!!!\n"); > return 0; > @@ -54,9 +58,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) > walking = false; > return depth; > } > - > -int backtrace(const void **return_addrs, int max_depth) > -{ > - return backtrace_frame(__builtin_frame_address(0), > - return_addrs, max_depth); > -} > diff --git a/lib/riscv/stack.c b/lib/riscv/stack.c > index 712a5478d547..d865594b9671 100644 > --- a/lib/riscv/stack.c > +++ b/lib/riscv/stack.c > @@ -2,12 +2,16 @@ > #include <libcflat.h> > #include <stack.h> > > -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) > +int arch_backtrace_frame(const void *frame, const void **return_addrs, > + int max_depth, bool current_frame) > { > static bool walking; > const unsigned long *fp = (unsigned long *)frame; > int depth; > > + if (current_frame) > + fp = __builtin_frame_address(0); > + > if (walking) { > printf("RECURSIVE STACK WALK!!!\n"); > return 0; > @@ -24,9 +28,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) > walking = false; > return depth; > } > - > -int backtrace(const void **return_addrs, int max_depth) > -{ > - return backtrace_frame(__builtin_frame_address(0), > - return_addrs, max_depth); > -} > diff --git a/lib/s390x/stack.c b/lib/s390x/stack.c > index 9f234a12adf6..d194f654e94d 100644 > --- a/lib/s390x/stack.c > +++ b/lib/s390x/stack.c > @@ -14,11 +14,15 @@ > #include <stack.h> > #include <asm/arch_def.h> > > -int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) > +int arch_backtrace_frame(const void *frame, const void **return_addrs, > + int max_depth, bool current_frame) > { > int depth = 0; > struct stack_frame *stack = (struct stack_frame *)frame; > > + if (current_frame) > + stack = __builtin_frame_address(0); > + > for (depth = 0; stack && depth < max_depth; depth++) { > return_addrs[depth] = (void *)stack->grs[8]; > stack = stack->back_chain; > @@ -28,9 +32,3 @@ int backtrace_frame(const void *frame, const void **return_addrs, int max_depth) > > return depth; > } > - > -int backtrace(const void **return_addrs, int max_depth) > -{ > - return backtrace_frame(__builtin_frame_address(0), > - return_addrs, max_depth); > -} > diff --git a/lib/stack.h b/lib/stack.h > index 10fc2f793354..6edc84344b51 100644 > --- a/lib/stack.h > +++ b/lib/stack.h > @@ -11,17 +11,27 @@ > #include <asm/stack.h> > > #ifdef HAVE_ARCH_BACKTRACE_FRAME > -extern int backtrace_frame(const void *frame, const void **return_addrs, > - int max_depth); > +extern int arch_backtrace_frame(const void *frame, const void **return_addrs, > + int max_depth, bool current_frame); > + > +static inline int backtrace_frame(const void *frame, const void **return_addrs, > + int max_depth) > +{ > + return arch_backtrace_frame(frame, return_addrs, max_depth, false); > +} > + > +static inline int backtrace(const void **return_addrs, int max_depth) > +{ > + return arch_backtrace_frame(NULL, return_addrs, max_depth, true); > +} > #else > -static inline int > -backtrace_frame(const void *frame __unused, const void **return_addrs __unused, > - int max_depth __unused) > +extern int backtrace(const void **return_addrs, int max_depth); > + > +static inline int backtrace_frame(const void *frame, const void **return_addrs, > + int max_depth) > { > return 0; > } > #endif > > -extern int backtrace(const void **return_addrs, int max_depth); > - > #endif Is there a reason to add the inline wrappers rather than just externs and drop the arch_ prefix? Do we want to just generally have all arch specific functions have an arch_ prefix? Fine by me. Reviewed-by: Nicholas Piggin <npiggin@gmail.com> I'm fine to rebase the powerpc patch on top of this if it goes in first. Thanks for the heads up. Thanks, Nick ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [kvm-unit-tests PATCH 03/13] treewide: lib/stack: Fix backtrace 2024-02-29 3:31 ` Nicholas Piggin @ 2024-02-29 11:48 ` Andrew Jones 0 siblings, 0 replies; 7+ messages in thread From: Andrew Jones @ 2024-02-29 11:48 UTC (permalink / raw) To: Nicholas Piggin Cc: lvivier, linux-s390, thuth, nrb, frankja, kvm, kvm-riscv, kvmarm, pbonzini, imbrenda, linuxppc-dev On Thu, Feb 29, 2024 at 01:31:52PM +1000, Nicholas Piggin wrote: > On Thu Feb 29, 2024 at 1:04 AM AEST, Andrew Jones wrote: ... > > diff --git a/lib/stack.h b/lib/stack.h > > index 10fc2f793354..6edc84344b51 100644 > > --- a/lib/stack.h > > +++ b/lib/stack.h > > @@ -11,17 +11,27 @@ > > #include <asm/stack.h> > > > > #ifdef HAVE_ARCH_BACKTRACE_FRAME > > -extern int backtrace_frame(const void *frame, const void **return_addrs, > > - int max_depth); > > +extern int arch_backtrace_frame(const void *frame, const void **return_addrs, > > + int max_depth, bool current_frame); > > + > > +static inline int backtrace_frame(const void *frame, const void **return_addrs, > > + int max_depth) > > +{ > > + return arch_backtrace_frame(frame, return_addrs, max_depth, false); > > +} > > + > > +static inline int backtrace(const void **return_addrs, int max_depth) > > +{ > > + return arch_backtrace_frame(NULL, return_addrs, max_depth, true); > > +} > > #else > > -static inline int > > -backtrace_frame(const void *frame __unused, const void **return_addrs __unused, > > - int max_depth __unused) > > +extern int backtrace(const void **return_addrs, int max_depth); > > + > > +static inline int backtrace_frame(const void *frame, const void **return_addrs, > > + int max_depth) > > { > > return 0; > > } > > #endif > > > > -extern int backtrace(const void **return_addrs, int max_depth); > > - > > #endif > > Is there a reason to add the inline wrappers rather than just externs > and drop the arch_ prefix? Only reason is to avoid duplicating the functions in each arch, but since they're oneliners which won't likely change, then we could duplicate them instead, if preferred, but I'm not sure what the benefit of that over the static inlines would be. > > Do we want to just generally have all arch specific functions have an > arch_ prefix? Fine by me. We've been slowly doing that over in 'KVM selftests', which has improved readability, so slowly adopting it here too in kvm-unit-tests would be nice. > > Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > > I'm fine to rebase the powerpc patch on top of this if it goes in first. > Thanks for the heads up. > Thanks, drew ^ permalink raw reply [flat|nested] 7+ messages in thread
* [kvm-unit-tests PATCH 04/13] treewide: lib/stack: Make base_address arch specific [not found] <20240228150416.248948-15-andrew.jones@linux.dev> 2024-02-28 15:04 ` [kvm-unit-tests PATCH 03/13] treewide: lib/stack: Fix backtrace Andrew Jones @ 2024-02-28 15:04 ` Andrew Jones 2024-02-29 3:49 ` Nicholas Piggin 1 sibling, 1 reply; 7+ messages in thread From: Andrew Jones @ 2024-02-28 15:04 UTC (permalink / raw) To: kvm, kvm-riscv Cc: lvivier, linux-s390, thuth, nrb, frankja, npiggin, kvmarm, pbonzini, imbrenda, linuxppc-dev Calculating the offset of an address is image specific, which is architecture specific. Until now, all architectures and architecture configurations which select CONFIG_RELOC were able to subtract _etext, but the EFI configuration of riscv cannot (it must subtract ImageBase). Make this function architecture specific, since the architecture's image layout already is. Signed-off-by: Andrew Jones <andrew.jones@linux.dev> --- lib/arm64/stack.c | 17 +++++++++++++++++ lib/riscv/stack.c | 18 ++++++++++++++++++ lib/stack.c | 19 ++----------------- lib/stack.h | 2 ++ lib/x86/stack.c | 17 +++++++++++++++++ 5 files changed, 56 insertions(+), 17 deletions(-) diff --git a/lib/arm64/stack.c b/lib/arm64/stack.c index f5eb57fd8892..3369031a74f7 100644 --- a/lib/arm64/stack.c +++ b/lib/arm64/stack.c @@ -6,6 +6,23 @@ #include <stdbool.h> #include <stack.h> +#ifdef CONFIG_RELOC +extern char _text, _etext; + +bool base_address(const void *rebased_addr, unsigned long *addr) +{ + unsigned long ra = (unsigned long)rebased_addr; + unsigned long start = (unsigned long)&_text; + unsigned long end = (unsigned long)&_etext; + + if (ra < start || ra >= end) + return false; + + *addr = ra - start; + return true; +} +#endif + extern char vector_stub_start, vector_stub_end; int arch_backtrace_frame(const void *frame, const void **return_addrs, diff --git a/lib/riscv/stack.c b/lib/riscv/stack.c index d865594b9671..a143c22a570a 100644 --- a/lib/riscv/stack.c +++ b/lib/riscv/stack.c @@ -2,6 +2,24 @@ #include <libcflat.h> #include <stack.h> +#ifdef CONFIG_RELOC +extern char ImageBase, _text, _etext; + +bool base_address(const void *rebased_addr, unsigned long *addr) +{ + unsigned long ra = (unsigned long)rebased_addr; + unsigned long base = (unsigned long)&ImageBase; + unsigned long start = (unsigned long)&_text; + unsigned long end = (unsigned long)&_etext; + + if (ra < start || ra >= end) + return false; + + *addr = ra - base; + return true; +} +#endif + int arch_backtrace_frame(const void *frame, const void **return_addrs, int max_depth, bool current_frame) { diff --git a/lib/stack.c b/lib/stack.c index dd6bfa8dac6e..e5099e207388 100644 --- a/lib/stack.c +++ b/lib/stack.c @@ -11,23 +11,8 @@ #define MAX_DEPTH 20 -#ifdef CONFIG_RELOC -extern char _text, _etext; - -static bool base_address(const void *rebased_addr, unsigned long *addr) -{ - unsigned long ra = (unsigned long)rebased_addr; - unsigned long start = (unsigned long)&_text; - unsigned long end = (unsigned long)&_etext; - - if (ra < start || ra >= end) - return false; - - *addr = ra - start; - return true; -} -#else -static bool base_address(const void *rebased_addr, unsigned long *addr) +#ifndef CONFIG_RELOC +bool base_address(const void *rebased_addr, unsigned long *addr) { *addr = (unsigned long)rebased_addr; return true; diff --git a/lib/stack.h b/lib/stack.h index 6edc84344b51..f8def4ad4d49 100644 --- a/lib/stack.h +++ b/lib/stack.h @@ -10,6 +10,8 @@ #include <libcflat.h> #include <asm/stack.h> +bool base_address(const void *rebased_addr, unsigned long *addr); + #ifdef HAVE_ARCH_BACKTRACE_FRAME extern int arch_backtrace_frame(const void *frame, const void **return_addrs, int max_depth, bool current_frame); diff --git a/lib/x86/stack.c b/lib/x86/stack.c index 58ab6c4b293a..7ba73becbd69 100644 --- a/lib/x86/stack.c +++ b/lib/x86/stack.c @@ -1,6 +1,23 @@ #include <libcflat.h> #include <stack.h> +#ifdef CONFIG_RELOC +extern char _text, _etext; + +bool base_address(const void *rebased_addr, unsigned long *addr) +{ + unsigned long ra = (unsigned long)rebased_addr; + unsigned long start = (unsigned long)&_text; + unsigned long end = (unsigned long)&_etext; + + if (ra < start || ra >= end) + return false; + + *addr = ra - start; + return true; +} +#endif + int arch_backtrace_frame(const void *frame, const void **return_addrs, int max_depth, bool current_frame) { -- 2.43.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [kvm-unit-tests PATCH 04/13] treewide: lib/stack: Make base_address arch specific 2024-02-28 15:04 ` [kvm-unit-tests PATCH 04/13] treewide: lib/stack: Make base_address arch specific Andrew Jones @ 2024-02-29 3:49 ` Nicholas Piggin 2024-02-29 11:54 ` Andrew Jones 0 siblings, 1 reply; 7+ messages in thread From: Nicholas Piggin @ 2024-02-29 3:49 UTC (permalink / raw) To: Andrew Jones, kvm, kvm-riscv Cc: lvivier, linux-s390, thuth, nrb, frankja, kvmarm, pbonzini, imbrenda, linuxppc-dev On Thu Feb 29, 2024 at 1:04 AM AEST, Andrew Jones wrote: > Calculating the offset of an address is image specific, which is > architecture specific. Until now, all architectures and architecture > configurations which select CONFIG_RELOC were able to subtract > _etext, but the EFI configuration of riscv cannot (it must subtract > ImageBase). Make this function architecture specific, since the > architecture's image layout already is. arch_base_address()? How about a default implementation unlesss HAVE_ARCH_BASE_ADDRESS? Thanks, Nick > > Signed-off-by: Andrew Jones <andrew.jones@linux.dev> > --- > lib/arm64/stack.c | 17 +++++++++++++++++ > lib/riscv/stack.c | 18 ++++++++++++++++++ > lib/stack.c | 19 ++----------------- > lib/stack.h | 2 ++ > lib/x86/stack.c | 17 +++++++++++++++++ > 5 files changed, 56 insertions(+), 17 deletions(-) > > diff --git a/lib/arm64/stack.c b/lib/arm64/stack.c > index f5eb57fd8892..3369031a74f7 100644 > --- a/lib/arm64/stack.c > +++ b/lib/arm64/stack.c > @@ -6,6 +6,23 @@ > #include <stdbool.h> > #include <stack.h> > > +#ifdef CONFIG_RELOC > +extern char _text, _etext; > + > +bool base_address(const void *rebased_addr, unsigned long *addr) > +{ > + unsigned long ra = (unsigned long)rebased_addr; > + unsigned long start = (unsigned long)&_text; > + unsigned long end = (unsigned long)&_etext; > + > + if (ra < start || ra >= end) > + return false; > + > + *addr = ra - start; > + return true; > +} > +#endif > + > extern char vector_stub_start, vector_stub_end; > > int arch_backtrace_frame(const void *frame, const void **return_addrs, > diff --git a/lib/riscv/stack.c b/lib/riscv/stack.c > index d865594b9671..a143c22a570a 100644 > --- a/lib/riscv/stack.c > +++ b/lib/riscv/stack.c > @@ -2,6 +2,24 @@ > #include <libcflat.h> > #include <stack.h> > > +#ifdef CONFIG_RELOC > +extern char ImageBase, _text, _etext; > + > +bool base_address(const void *rebased_addr, unsigned long *addr) > +{ > + unsigned long ra = (unsigned long)rebased_addr; > + unsigned long base = (unsigned long)&ImageBase; > + unsigned long start = (unsigned long)&_text; > + unsigned long end = (unsigned long)&_etext; > + > + if (ra < start || ra >= end) > + return false; > + > + *addr = ra - base; > + return true; > +} > +#endif > + > int arch_backtrace_frame(const void *frame, const void **return_addrs, > int max_depth, bool current_frame) > { > diff --git a/lib/stack.c b/lib/stack.c > index dd6bfa8dac6e..e5099e207388 100644 > --- a/lib/stack.c > +++ b/lib/stack.c > @@ -11,23 +11,8 @@ > > #define MAX_DEPTH 20 > > -#ifdef CONFIG_RELOC > -extern char _text, _etext; > - > -static bool base_address(const void *rebased_addr, unsigned long *addr) > -{ > - unsigned long ra = (unsigned long)rebased_addr; > - unsigned long start = (unsigned long)&_text; > - unsigned long end = (unsigned long)&_etext; > - > - if (ra < start || ra >= end) > - return false; > - > - *addr = ra - start; > - return true; > -} > -#else > -static bool base_address(const void *rebased_addr, unsigned long *addr) > +#ifndef CONFIG_RELOC > +bool base_address(const void *rebased_addr, unsigned long *addr) > { > *addr = (unsigned long)rebased_addr; > return true; > diff --git a/lib/stack.h b/lib/stack.h > index 6edc84344b51..f8def4ad4d49 100644 > --- a/lib/stack.h > +++ b/lib/stack.h > @@ -10,6 +10,8 @@ > #include <libcflat.h> > #include <asm/stack.h> > > +bool base_address(const void *rebased_addr, unsigned long *addr); > + > #ifdef HAVE_ARCH_BACKTRACE_FRAME > extern int arch_backtrace_frame(const void *frame, const void **return_addrs, > int max_depth, bool current_frame); > diff --git a/lib/x86/stack.c b/lib/x86/stack.c > index 58ab6c4b293a..7ba73becbd69 100644 > --- a/lib/x86/stack.c > +++ b/lib/x86/stack.c > @@ -1,6 +1,23 @@ > #include <libcflat.h> > #include <stack.h> > > +#ifdef CONFIG_RELOC > +extern char _text, _etext; > + > +bool base_address(const void *rebased_addr, unsigned long *addr) > +{ > + unsigned long ra = (unsigned long)rebased_addr; > + unsigned long start = (unsigned long)&_text; > + unsigned long end = (unsigned long)&_etext; > + > + if (ra < start || ra >= end) > + return false; > + > + *addr = ra - start; > + return true; > +} > +#endif > + > int arch_backtrace_frame(const void *frame, const void **return_addrs, > int max_depth, bool current_frame) > { ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [kvm-unit-tests PATCH 04/13] treewide: lib/stack: Make base_address arch specific 2024-02-29 3:49 ` Nicholas Piggin @ 2024-02-29 11:54 ` Andrew Jones 0 siblings, 0 replies; 7+ messages in thread From: Andrew Jones @ 2024-02-29 11:54 UTC (permalink / raw) To: Nicholas Piggin Cc: lvivier, linux-s390, thuth, nrb, frankja, kvm, kvm-riscv, kvmarm, pbonzini, imbrenda, linuxppc-dev On Thu, Feb 29, 2024 at 01:49:58PM +1000, Nicholas Piggin wrote: > On Thu Feb 29, 2024 at 1:04 AM AEST, Andrew Jones wrote: > > Calculating the offset of an address is image specific, which is > > architecture specific. Until now, all architectures and architecture > > configurations which select CONFIG_RELOC were able to subtract > > _etext, but the EFI configuration of riscv cannot (it must subtract > > ImageBase). Make this function architecture specific, since the > > architecture's image layout already is. > > arch_base_address()? Yeah, I should have added that prefix. > > How about a default implementation unlesss HAVE_ARCH_BASE_ADDRESS? We have a default implementation for !CONFIG_RELOC, but if an arch selects RELOC it must have an implementation of base_address(), so I wouldn't introduce a HAVE_ARCH_BASE_ADDRESS type of config since it would just always be selected when RELOC is selected. It occurred to me after posting that I probably should have just made the current base_address() implementation weak and then only introduced the new riscv one. I'll do that for v2. Thanks, drew ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-02-29 11:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20240228150416.248948-15-andrew.jones@linux.dev>
2024-02-28 15:04 ` [kvm-unit-tests PATCH 03/13] treewide: lib/stack: Fix backtrace Andrew Jones
2024-02-28 17:33 ` Claudio Imbrenda
2024-02-29 3:31 ` Nicholas Piggin
2024-02-29 11:48 ` Andrew Jones
2024-02-28 15:04 ` [kvm-unit-tests PATCH 04/13] treewide: lib/stack: Make base_address arch specific Andrew Jones
2024-02-29 3:49 ` Nicholas Piggin
2024-02-29 11:54 ` Andrew Jones
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 NNTP newsgroup(s).