[kvm-unit-tests PATCH] lib/stack: Restrengthen base_address

[kvm-unit-tests PATCH] lib/stack: Restrengthen base_address

[Andrew Jones]

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(-)

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
-- 
2.46.0

Re: [kvm-unit-tests PATCH] lib/stack: Restrengthen base_address

[Nicholas Piggin]

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

Re: [kvm-unit-tests PATCH] lib/stack: Restrengthen base_address

[Andrew Jones]

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