diff mbox series

[1/3] RISC-V: Stop relying on GCC's register allocator's hueristics

Message ID 20200227213450.87194-2-palmer@dabbelt.com (mailing list archive)
State New, archived
Headers show
Series [1/3] RISC-V: Stop relying on GCC's register allocator's hueristics | expand

Commit Message

Palmer Dabbelt Feb. 27, 2020, 9:34 p.m. UTC 
From: Palmer Dabbelt <palmerdabbelt@google.com>

GCC allows users to hint to the register allocation that a variable should be
placed in a register by using a syntax along the lines of

    function(...) {
        register long in_REG __asm__("REG");
    }

We've abused this a bit throughout the RISC-V port to access fixed registers
directly as C variables.  In practice it's never going to blow up because GCC
isn't going to allocate these registers, but it's not a well defined syntax so
we really shouldn't be relying upon this.  Luckily there is a very similar but
well defined syntax that allows us to still access these registers directly as
C variables, which is to simply declare the register variables globally.  For
fixed variables this doesn't change the ABI.

LLVM disallows this ambiguous syntax, so this isn't just strictly a formatting
change.

Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com>
---
 arch/riscv/include/asm/current.h | 5 +++--
 arch/riscv/kernel/process.c      | 5 +++--
 arch/riscv/kernel/stacktrace.c   | 7 ++++---
 3 files changed, 10 insertions(+), 7 deletions(-)

Comments

Nick Desaulniers Feb. 27, 2020, 9:56 p.m. UTC | #1 
On Thu, Feb 27, 2020 at 1:35 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> From: Palmer Dabbelt <palmerdabbelt@google.com>
>
> GCC allows users to hint to the register allocation that a variable should be
> placed in a register by using a syntax along the lines of
>
>     function(...) {
>         register long in_REG __asm__("REG");
>     }
>
> We've abused this a bit throughout the RISC-V port to access fixed registers
> directly as C variables.  In practice it's never going to blow up because GCC
> isn't going to allocate these registers, but it's not a well defined syntax so
> we really shouldn't be relying upon this.  Luckily there is a very similar but
> well defined syntax that allows us to still access these registers directly as
> C variables, which is to simply declare the register variables globally.  For
> fixed variables this doesn't change the ABI.
>
> LLVM disallows this ambiguous syntax, so this isn't just strictly a formatting
> change.
>
> Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com>

Thanks for the patches!
Indeed, looks like the local register variables are very much intended
to be used as inputs & outputs to extended inline assembly, which in
these cases are not.
Link: https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html
Link: https://gcc.gnu.org/onlinedocs/gcc/Global-Register-Variables.html
One fixup, below:

> ---
>  arch/riscv/include/asm/current.h | 5 +++--
>  arch/riscv/kernel/process.c      | 5 +++--
>  arch/riscv/kernel/stacktrace.c   | 7 ++++---
>  3 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/arch/riscv/include/asm/current.h b/arch/riscv/include/asm/current.h
> index dd973efe5d7c..1de233d8e8de 100644
> --- a/arch/riscv/include/asm/current.h
> +++ b/arch/riscv/include/asm/current.h
> @@ -17,6 +17,8 @@
>
>  struct task_struct;
>
> +register struct task_struct *riscv_current_is_tp __asm__("tp");
> +
>  /*
>   * This only works because "struct thread_info" is at offset 0 from "struct
>   * task_struct".  This constraint seems to be necessary on other architectures
> @@ -26,8 +28,7 @@ struct task_struct;
>   */
>  static __always_inline struct task_struct *get_current(void)
>  {
> -       register struct task_struct *tp __asm__("tp");
> -       return tp;
> +       return riscv_current_is_tp;
>  }
>
>  #define current get_current()
> diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
> index 817cf7b0974c..610c11e91606 100644
> --- a/arch/riscv/kernel/process.c
> +++ b/arch/riscv/kernel/process.c
> @@ -22,6 +22,8 @@
>  #include <asm/switch_to.h>
>  #include <asm/thread_info.h>
>
> +unsigned long gp_in_global __asm__("gp");
> +
>  extern asmlinkage void ret_from_fork(void);
>  extern asmlinkage void ret_from_kernel_thread(void);
>
> @@ -107,9 +109,8 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long usp,
>         /* p->thread holds context to be restored by __switch_to() */
>         if (unlikely(p->flags & PF_KTHREAD)) {
>                 /* Kernel thread */
> -               const register unsigned long gp __asm__ ("gp");
>                 memset(childregs, 0, sizeof(struct pt_regs));
> -               childregs->gp = gp;
> +               childregs->gp = gp_in_global;
>                 /* Supervisor/Machine, irqs on: */
>                 childregs->status = SR_PP | SR_PIE;
>
> diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
> index 0940681d2f68..02087fe539c6 100644
> --- a/arch/riscv/kernel/stacktrace.c
> +++ b/arch/riscv/kernel/stacktrace.c
> @@ -19,6 +19,8 @@ struct stackframe {
>         unsigned long ra;
>  };
>
> +register unsigned long sp_in_global __asm__("sp");
> +
>  void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
>                              bool (*fn)(unsigned long, void *), void *arg)
>  {
> @@ -29,7 +31,7 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
>                 sp = user_stack_pointer(regs);
>                 pc = instruction_pointer(regs);
>         } else if (task == NULL || task == current) {
> -               const register unsigned long current_sp __asm__ ("sp");
> +               const register unsigned long current_sp = sp_in_global;
>                 fp = (unsigned long)__builtin_frame_address(0);
>                 sp = current_sp;

^ probably this should just be:
sp = sp_in_global;

>                 pc = (unsigned long)walk_stackframe;
> @@ -73,8 +75,7 @@ static void notrace walk_stackframe(struct task_struct *task,
>                 sp = user_stack_pointer(regs);
>                 pc = instruction_pointer(regs);
>         } else if (task == NULL || task == current) {
> -               const register unsigned long current_sp __asm__ ("sp");
> -               sp = current_sp;
> +               sp = sp_in_global;
>                 pc = (unsigned long)walk_stackframe;
>         } else {
>                 /* task blocked in __switch_to */
> --
> 2.25.1.481.gfbce0eb801-goog
>
> --
Maciej W. Rozycki March 19, 2020, 6:53 p.m. UTC | #2 
On Thu, 27 Feb 2020, Nick Desaulniers wrote:

> Indeed, looks like the local register variables are very much intended
> to be used as inputs & outputs to extended inline assembly, which in
> these cases are not.
> Link: https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html

 There is a further restriction on local register variables causing them 
to be possibly clobbered as a result of a function call, which may happen 
at unusual places due to inlining.

 For an example of this happening see a glibc bug recently reported here: 
<https://sourceware.org/bugzilla/show_bug.cgi?id=25523> and the cascade of 
fixes following, including one for the RISC-V target.

  Maciej
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/current.h b/arch/riscv/include/asm/current.h
index dd973efe5d7c..1de233d8e8de 100644
--- a/arch/riscv/include/asm/current.h
+++ b/arch/riscv/include/asm/current.h
@@ -17,6 +17,8 @@ 
 
 struct task_struct;
 
+register struct task_struct *riscv_current_is_tp __asm__("tp");
+
 /*
  * This only works because "struct thread_info" is at offset 0 from "struct
  * task_struct".  This constraint seems to be necessary on other architectures
@@ -26,8 +28,7 @@  struct task_struct;
  */
 static __always_inline struct task_struct *get_current(void)
 {
-	register struct task_struct *tp __asm__("tp");
-	return tp;
+	return riscv_current_is_tp;
 }
 
 #define current get_current()
diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
index 817cf7b0974c..610c11e91606 100644
--- a/arch/riscv/kernel/process.c
+++ b/arch/riscv/kernel/process.c
@@ -22,6 +22,8 @@ 
 #include <asm/switch_to.h>
 #include <asm/thread_info.h>
 
+unsigned long gp_in_global __asm__("gp");
+
 extern asmlinkage void ret_from_fork(void);
 extern asmlinkage void ret_from_kernel_thread(void);
 
@@ -107,9 +109,8 @@  int copy_thread_tls(unsigned long clone_flags, unsigned long usp,
 	/* p->thread holds context to be restored by __switch_to() */
 	if (unlikely(p->flags & PF_KTHREAD)) {
 		/* Kernel thread */
-		const register unsigned long gp __asm__ ("gp");
 		memset(childregs, 0, sizeof(struct pt_regs));
-		childregs->gp = gp;
+		childregs->gp = gp_in_global;
 		/* Supervisor/Machine, irqs on: */
 		childregs->status = SR_PP | SR_PIE;
 
diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
index 0940681d2f68..02087fe539c6 100644
--- a/arch/riscv/kernel/stacktrace.c
+++ b/arch/riscv/kernel/stacktrace.c
@@ -19,6 +19,8 @@  struct stackframe {
 	unsigned long ra;
 };
 
+register unsigned long sp_in_global __asm__("sp");
+
 void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
 			     bool (*fn)(unsigned long, void *), void *arg)
 {
@@ -29,7 +31,7 @@  void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
 		sp = user_stack_pointer(regs);
 		pc = instruction_pointer(regs);
 	} else if (task == NULL || task == current) {
-		const register unsigned long current_sp __asm__ ("sp");
+		const register unsigned long current_sp = sp_in_global;
 		fp = (unsigned long)__builtin_frame_address(0);
 		sp = current_sp;
 		pc = (unsigned long)walk_stackframe;
@@ -73,8 +75,7 @@  static void notrace walk_stackframe(struct task_struct *task,
 		sp = user_stack_pointer(regs);
 		pc = instruction_pointer(regs);
 	} else if (task == NULL || task == current) {
-		const register unsigned long current_sp __asm__ ("sp");
-		sp = current_sp;
+		sp = sp_in_global;
 		pc = (unsigned long)walk_stackframe;
 	} else {
 		/* task blocked in __switch_to */