| Message ID | 20210317150838.112021-1-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: Drop const annotation for sp | expand |
Hi Palmer， I have a question about commit 52e7c52d2ded5908e6a4f8a7248e5fa6e0d6809a Author: Palmer Dabbelt <palmerdabbelt@google.com> Date: Thu Feb 27 11:07:28 2020 -0800 RISC-V: Stop relying on GCC's register allocator's hueristics Why do you only use sp_in_global directly when CONFIG_FRAME_POINTER not enabled, and keep the current_sp part when CONFIG_FRAME_POINTER enabled? If there are special reason, I think I should revert the dec822771b01, thanks. On 2021/3/17 23:08, Kefeng Wang wrote: > The const annotation should not be used for 'sp', or it will > become read only and lead to bad stack output. > > Fixes: dec822771b01 ("riscv: stacktrace: Move register keyword to beginning of declaration") > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > --- > arch/riscv/kernel/stacktrace.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c > index 3f893c9d9d85..2b3e0cb90d78 100644 > --- a/arch/riscv/kernel/stacktrace.c > +++ b/arch/riscv/kernel/stacktrace.c > @@ -14,7 +14,7 @@ > > #include <asm/stacktrace.h> > > -register const unsigned long sp_in_global __asm__("sp"); > +register unsigned long sp_in_global __asm__("sp"); > > #ifdef CONFIG_FRAME_POINTER >
On Wed, 17 Mar 2021 08:21:21 PDT (-0700), wangkefeng.wang@huawei.com wrote: > Hi Palmer， I have a question about > > commit 52e7c52d2ded5908e6a4f8a7248e5fa6e0d6809a > Author: Palmer Dabbelt <palmerdabbelt@google.com> > Date: Thu Feb 27 11:07:28 2020 -0800 > > RISC-V: Stop relying on GCC's register allocator's hueristics > > Why do you only use sp_in_global directly when CONFIG_FRAME_POINTER not > enabled, > > and keep the current_sp part when CONFIG_FRAME_POINTER enabled? I think I just missed that one when cleaning up the others. The instances of using this locally are all wrong, it's just a hint to GCC. The patch here is correct. According to the GCC manual: "Do not use type qualifiers such as const and volatile, as the outcome may be contrary to expectations." It doesn't mention in what way "const" may provide an unexpected outcome, but that's probably why I didn't have it the first time (despite us only reading the variable). I must have forgotten about that when I saw the patch go by. This is on fixes. > If there are special reason, I think I should revert the dec822771b01, > thanks. > > > On 2021/3/17 23:08, Kefeng Wang wrote: >> The const annotation should not be used for 'sp', or it will >> become read only and lead to bad stack output. >> >> Fixes: dec822771b01 ("riscv: stacktrace: Move register keyword to beginning of declaration") >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >> --- >> arch/riscv/kernel/stacktrace.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c >> index 3f893c9d9d85..2b3e0cb90d78 100644 >> --- a/arch/riscv/kernel/stacktrace.c >> +++ b/arch/riscv/kernel/stacktrace.c >> @@ -14,7 +14,7 @@ >> >> #include <asm/stacktrace.h> >> >> -register const unsigned long sp_in_global __asm__("sp"); >> +register unsigned long sp_in_global __asm__("sp"); >> >> #ifdef CONFIG_FRAME_POINTER >>
On Mon, 29 Mar 2021, Palmer Dabbelt wrote: > The patch here is correct. According to the GCC manual: "Do not use > type qualifiers such as const and volatile, as the outcome may be > contrary to expectations." It doesn't mention in what way "const" may > provide an unexpected outcome, but that's probably why I didn't have it > the first time (despite us only reading the variable). I must have > forgotten about that when I saw the patch go by. See GCC PR c/85745 and PR target/86673. It probably does not apply to global asm variables though, not at least in the way described (IMHO it doesn't make sense either, as `sp' surely isn't constant). HTH, Maciej
diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c index 3f893c9d9d85..2b3e0cb90d78 100644 --- a/arch/riscv/kernel/stacktrace.c +++ b/arch/riscv/kernel/stacktrace.c @@ -14,7 +14,7 @@ #include <asm/stacktrace.h> -register const unsigned long sp_in_global __asm__("sp"); +register unsigned long sp_in_global __asm__("sp"); #ifdef CONFIG_FRAME_POINTER
The const annotation should not be used for 'sp', or it will become read only and lead to bad stack output. Fixes: dec822771b01 ("riscv: stacktrace: Move register keyword to beginning of declaration") Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- arch/riscv/kernel/stacktrace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)