| Message ID | 20240904145107.2447876-2-andrew.jones@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] lib/stack: Restrengthen base_address | expand |
On Thu Sep 5, 2024 at 12:51 AM AEST, Andrew Jones wrote: > commit a1f2b0e1efd5 ("treewide: lib/stack: Make base_address arch > specific") made base_address() a weak function in order to allow > architectures to override it. Linking for EFI doesn't seem to figure > out the right one to use though [anymore?]. It must have worked at > one point because the commit calls outs EFI as the motivation. > Anyway, just drop the weakness in favor of another HAVE_ define. I prefer HAVE_ style than weak so fine by me. How is the linker not resolving it properly? Some calls still point to weak symbol despite non-weak symbol also existing? > > Signed-off-by: Andrew Jones <andrew.jones@linux.dev> > --- > lib/riscv/asm/stack.h | 1 + > lib/riscv/stack.c | 2 +- > lib/stack.c | 10 ++++++---- > lib/stack.h | 2 +- > 4 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/lib/riscv/asm/stack.h b/lib/riscv/asm/stack.h > index f003ca37c913..708fa4215007 100644 > --- a/lib/riscv/asm/stack.h > +++ b/lib/riscv/asm/stack.h > @@ -8,5 +8,6 @@ > > #define HAVE_ARCH_BACKTRACE_FRAME > #define HAVE_ARCH_BACKTRACE > +#define HAVE_ARCH_BASE_ADDRESS > > #endif > diff --git a/lib/riscv/stack.c b/lib/riscv/stack.c > index 2cd7f012738b..a143c22a570a 100644 > --- a/lib/riscv/stack.c > +++ b/lib/riscv/stack.c > @@ -5,7 +5,7 @@ > #ifdef CONFIG_RELOC > extern char ImageBase, _text, _etext; > > -bool arch_base_address(const void *rebased_addr, unsigned long *addr) > +bool base_address(const void *rebased_addr, unsigned long *addr) > { > unsigned long ra = (unsigned long)rebased_addr; > unsigned long base = (unsigned long)&ImageBase; > diff --git a/lib/stack.c b/lib/stack.c > index 086fec544a81..e1c981085176 100644 > --- a/lib/stack.c > +++ b/lib/stack.c > @@ -12,9 +12,10 @@ > #define MAX_DEPTH 20 > > #ifdef CONFIG_RELOC > +#ifndef HAVE_ARCH_BASE_ADDRESS > extern char _text, _etext; > > -bool __attribute__((weak)) arch_base_address(const void *rebased_addr, unsigned long *addr) > +bool base_address(const void *rebased_addr, unsigned long *addr) > { > unsigned long ra = (unsigned long)rebased_addr; > unsigned long start = (unsigned long)&_text; > @@ -26,8 +27,9 @@ bool __attribute__((weak)) arch_base_address(const void *rebased_addr, unsigned > *addr = ra - start; > return true; > } > +#endif > #else > -bool __attribute__((weak)) arch_base_address(const void *rebased_addr, unsigned long *addr) > +bool base_address(const void *rebased_addr, unsigned long *addr) > { > *addr = (unsigned long)rebased_addr; > return true; Shouldn't HAVE_ARCH_BASE_ADDRESS also cover this? Thanks, Nick
On Wed, Sep 11, 2024 at 10:55:34AM GMT, Nicholas Piggin wrote: > On Thu Sep 5, 2024 at 12:51 AM AEST, Andrew Jones wrote: > > commit a1f2b0e1efd5 ("treewide: lib/stack: Make base_address arch > > specific") made base_address() a weak function in order to allow > > architectures to override it. Linking for EFI doesn't seem to figure > > out the right one to use though [anymore?]. It must have worked at > > one point because the commit calls outs EFI as the motivation. > > Anyway, just drop the weakness in favor of another HAVE_ define. > > I prefer HAVE_ style than weak so fine by me. > > How is the linker not resolving it properly? Some calls still > point to weak symbol despite non-weak symbol also existing? Yeah, I noticed traces stopped working with EFI because it was using the weak version of the function instead of the riscv non-weak version. Since I'm 99% sure it used to work, then I need to find time to try and figure out if it's something that changed in k-u-t that is now confusing the toolchain or a toolchain regression. (It's on the TODO, but there's lots of stuff on the TODO...) > > > > > > Signed-off-by: Andrew Jones <andrew.jones@linux.dev> > > --- > > lib/riscv/asm/stack.h | 1 + > > lib/riscv/stack.c | 2 +- > > lib/stack.c | 10 ++++++---- > > lib/stack.h | 2 +- > > 4 files changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/lib/riscv/asm/stack.h b/lib/riscv/asm/stack.h > > index f003ca37c913..708fa4215007 100644 > > --- a/lib/riscv/asm/stack.h > > +++ b/lib/riscv/asm/stack.h > > @@ -8,5 +8,6 @@ > > > > #define HAVE_ARCH_BACKTRACE_FRAME > > #define HAVE_ARCH_BACKTRACE > > +#define HAVE_ARCH_BASE_ADDRESS > > > > #endif > > diff --git a/lib/riscv/stack.c b/lib/riscv/stack.c > > index 2cd7f012738b..a143c22a570a 100644 > > --- a/lib/riscv/stack.c > > +++ b/lib/riscv/stack.c > > @@ -5,7 +5,7 @@ > > #ifdef CONFIG_RELOC > > extern char ImageBase, _text, _etext; > > > > -bool arch_base_address(const void *rebased_addr, unsigned long *addr) > > +bool base_address(const void *rebased_addr, unsigned long *addr) > > { > > unsigned long ra = (unsigned long)rebased_addr; > > unsigned long base = (unsigned long)&ImageBase; > > diff --git a/lib/stack.c b/lib/stack.c > > index 086fec544a81..e1c981085176 100644 > > --- a/lib/stack.c > > +++ b/lib/stack.c > > @@ -12,9 +12,10 @@ > > #define MAX_DEPTH 20 > > > > #ifdef CONFIG_RELOC > > +#ifndef HAVE_ARCH_BASE_ADDRESS > > extern char _text, _etext; > > > > -bool __attribute__((weak)) arch_base_address(const void *rebased_addr, unsigned long *addr) > > +bool base_address(const void *rebased_addr, unsigned long *addr) > > { > > unsigned long ra = (unsigned long)rebased_addr; > > unsigned long start = (unsigned long)&_text; > > @@ -26,8 +27,9 @@ bool __attribute__((weak)) arch_base_address(const void *rebased_addr, unsigned > > *addr = ra - start; > > return true; > > } > > +#endif > > #else > > -bool __attribute__((weak)) arch_base_address(const void *rebased_addr, unsigned long *addr) > > +bool base_address(const void *rebased_addr, unsigned long *addr) > > { > > *addr = (unsigned long)rebased_addr; > > return true; > > Shouldn't HAVE_ARCH_BASE_ADDRESS also cover this? Yes, I suppose that would be the cleanest thing to do. And then in lib/$ARCH/asm/stack.h we should have #ifdef CONFIG_RELOC #define HAVE_ARCH_BASE_ADDRESS #endif when the arch wants to use the implementation here (which is probably would). Thanks, drew
diff --git a/lib/riscv/asm/stack.h b/lib/riscv/asm/stack.h index f003ca37c913..708fa4215007 100644 --- a/lib/riscv/asm/stack.h +++ b/lib/riscv/asm/stack.h @@ -8,5 +8,6 @@ #define HAVE_ARCH_BACKTRACE_FRAME #define HAVE_ARCH_BACKTRACE +#define HAVE_ARCH_BASE_ADDRESS #endif diff --git a/lib/riscv/stack.c b/lib/riscv/stack.c index 2cd7f012738b..a143c22a570a 100644 --- a/lib/riscv/stack.c +++ b/lib/riscv/stack.c @@ -5,7 +5,7 @@ #ifdef CONFIG_RELOC extern char ImageBase, _text, _etext; -bool arch_base_address(const void *rebased_addr, unsigned long *addr) +bool base_address(const void *rebased_addr, unsigned long *addr) { unsigned long ra = (unsigned long)rebased_addr; unsigned long base = (unsigned long)&ImageBase; diff --git a/lib/stack.c b/lib/stack.c index 086fec544a81..e1c981085176 100644 --- a/lib/stack.c +++ b/lib/stack.c @@ -12,9 +12,10 @@ #define MAX_DEPTH 20 #ifdef CONFIG_RELOC +#ifndef HAVE_ARCH_BASE_ADDRESS extern char _text, _etext; -bool __attribute__((weak)) arch_base_address(const void *rebased_addr, unsigned long *addr) +bool base_address(const void *rebased_addr, unsigned long *addr) { unsigned long ra = (unsigned long)rebased_addr; unsigned long start = (unsigned long)&_text; @@ -26,8 +27,9 @@ bool __attribute__((weak)) arch_base_address(const void *rebased_addr, unsigned *addr = ra - start; return true; } +#endif #else -bool __attribute__((weak)) arch_base_address(const void *rebased_addr, unsigned long *addr) +bool base_address(const void *rebased_addr, unsigned long *addr) { *addr = (unsigned long)rebased_addr; return true; @@ -45,13 +47,13 @@ static void print_stack(const void **return_addrs, int depth, /* @addr indicates a non-return address, as expected by the stack * pretty printer script. */ if (depth > 0 && !top_is_return_address) { - if (arch_base_address(return_addrs[0], &addr)) + if (base_address(return_addrs[0], &addr)) printf(" @%lx", addr); i++; } for (; i < depth; i++) { - if (arch_base_address(return_addrs[i], &addr)) + if (base_address(return_addrs[i], &addr)) printf(" %lx", addr); } printf("\n"); diff --git a/lib/stack.h b/lib/stack.h index df076d94bf8f..c92112c1744d 100644 --- a/lib/stack.h +++ b/lib/stack.h @@ -34,6 +34,6 @@ static inline int backtrace_frame(const void *frame, const void **return_addrs, } #endif -bool __attribute__((weak)) arch_base_address(const void *rebased_addr, unsigned long *addr); +bool base_address(const void *rebased_addr, unsigned long *addr); #endif
commit a1f2b0e1efd5 ("treewide: lib/stack: Make base_address arch specific") made base_address() a weak function in order to allow architectures to override it. Linking for EFI doesn't seem to figure out the right one to use though [anymore?]. It must have worked at one point because the commit calls outs EFI as the motivation. Anyway, just drop the weakness in favor of another HAVE_ define. Signed-off-by: Andrew Jones <andrew.jones@linux.dev> --- lib/riscv/asm/stack.h | 1 + lib/riscv/stack.c | 2 +- lib/stack.c | 10 ++++++---- lib/stack.h | 2 +- 4 files changed, 9 insertions(+), 6 deletions(-)