GAS <= 2.41 does not support generating R_LARCH_{32,64}_PCREL for
"label - ." and it generates R_LARCH_{ADD,SUB}{32,64} pairs instead.
objtool cannot handle R_LARCH_{ADD,SUB}{32,64} pair in __jump_table
(static key implementation) and etc. so it will produce some warnings.
This is causing the kernel CI systems to complain everywhere.
For GAS we can check if -mthin-add-sub option is available to know if
R_LARCH_{32,64}_PCREL are supported.
For Clang, we require Clang >= 18 and Clang >= 17 already supports
R_LARCH_{32,64}_PCREL, so we can always assume Clang is fine for
objtool.
Note that __jump_table is not generated by the compiler so
-fno-jump-table is completely irrelevant for this issue.
Fixes: cb8a2ef0848c ("LoongArch: Add ORC stack unwinder support")
Closes: https://lore.kernel.org/loongarch/Zl5m1ZlVmGKitAof@yujie-X299/
Closes: https://lore.kernel.org/loongarch/[email protected]/
Closes: https://lore.kernel.org/loongarch/[email protected]/
Link: https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=816029e06768
Link: https://github.com/llvm/llvm-project/commit/42cb3c6346fc
Signed-off-by: Xi Ruoyao <[email protected]>
---
arch/loongarch/Kconfig | 5 ++++-
arch/loongarch/Kconfig.debug | 1 +
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
index e38139c576ee..e461a6c59f15 100644
--- a/arch/loongarch/Kconfig
+++ b/arch/loongarch/Kconfig
@@ -143,7 +143,7 @@ config LOONGARCH
select HAVE_LIVEPATCH
select HAVE_MOD_ARCH_SPECIFIC
select HAVE_NMI
- select HAVE_OBJTOOL if AS_HAS_EXPLICIT_RELOCS
+ select HAVE_OBJTOOL if AS_HAS_EXPLICIT_RELOCS && AS_HAS_THIN_ADD_SUB
select HAVE_PCI
select HAVE_PERF_EVENTS
select HAVE_PERF_REGS
@@ -273,6 +273,9 @@ config AS_HAS_LBT_EXTENSION
config AS_HAS_LVZ_EXTENSION
def_bool $(as-instr,hvcl 0)
+config AS_HAS_THIN_ADD_SUB
+ def_bool $(cc-option,-Wa$(comma)-mthin-add-sub) || CC_IS_CLANG
+
menu "Kernel type and options"
source "kernel/Kconfig.hz"
diff --git a/arch/loongarch/Kconfig.debug b/arch/loongarch/Kconfig.debug
index 98d60630c3d4..8b2ce5b5d43e 100644
--- a/arch/loongarch/Kconfig.debug
+++ b/arch/loongarch/Kconfig.debug
@@ -28,6 +28,7 @@ config UNWINDER_PROLOGUE
config UNWINDER_ORC
bool "ORC unwinder"
+ depends on HAVE_OBJTOOL
select OBJTOOL
help
This option enables the ORC (Oops Rewind Capability) unwinder for
--
2.45.2
Hi, Ruoyao,
On Tue, Jun 4, 2024 at 11:08 PM Xi Ruoyao <[email protected]> wrote:
>
> GAS <= 2.41 does not support generating R_LARCH_{32,64}_PCREL for
> "label - ." and it generates R_LARCH_{ADD,SUB}{32,64} pairs instead.
> objtool cannot handle R_LARCH_{ADD,SUB}{32,64} pair in __jump_table
> (static key implementation) and etc. so it will produce some warnings.
> This is causing the kernel CI systems to complain everywhere.
>
> For GAS we can check if -mthin-add-sub option is available to know if
> R_LARCH_{32,64}_PCREL are supported.
I think we should give a chance to toolchains without the
-mthin-add-sub option, so I hope Tiezhu can resubmit this patch to
solve this problem.
https://lore.kernel.org/loongarch/[email protected]/
Huacai
>
> For Clang, we require Clang >= 18 and Clang >= 17 already supports
> R_LARCH_{32,64}_PCREL, so we can always assume Clang is fine for
> objtool.
>
> Note that __jump_table is not generated by the compiler so
> -fno-jump-table is completely irrelevant for this issue.
>
> Fixes: cb8a2ef0848c ("LoongArch: Add ORC stack unwinder support")
> Closes: https://lore.kernel.org/loongarch/Zl5m1ZlVmGKitAof@yujie-X299/
> Closes: https://lore.kernel.org/loongarch/[email protected]/
> Closes: https://lore.kernel.org/loongarch/[email protected]/
> Link: https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=816029e06768
> Link: https://github.com/llvm/llvm-project/commit/42cb3c6346fc
> Signed-off-by: Xi Ruoyao <[email protected]>
> ---
> arch/loongarch/Kconfig | 5 ++++-
> arch/loongarch/Kconfig.debug | 1 +
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
> index e38139c576ee..e461a6c59f15 100644
> --- a/arch/loongarch/Kconfig
> +++ b/arch/loongarch/Kconfig
> @@ -143,7 +143,7 @@ config LOONGARCH
> select HAVE_LIVEPATCH
> select HAVE_MOD_ARCH_SPECIFIC
> select HAVE_NMI
> - select HAVE_OBJTOOL if AS_HAS_EXPLICIT_RELOCS
> + select HAVE_OBJTOOL if AS_HAS_EXPLICIT_RELOCS && AS_HAS_THIN_ADD_SUB
> select HAVE_PCI
> select HAVE_PERF_EVENTS
> select HAVE_PERF_REGS
> @@ -273,6 +273,9 @@ config AS_HAS_LBT_EXTENSION
> config AS_HAS_LVZ_EXTENSION
> def_bool $(as-instr,hvcl 0)
>
> +config AS_HAS_THIN_ADD_SUB
> + def_bool $(cc-option,-Wa$(comma)-mthin-add-sub) || CC_IS_CLANG
> +
> menu "Kernel type and options"
>
> source "kernel/Kconfig.hz"
> diff --git a/arch/loongarch/Kconfig.debug b/arch/loongarch/Kconfig.debug
> index 98d60630c3d4..8b2ce5b5d43e 100644
> --- a/arch/loongarch/Kconfig.debug
> +++ b/arch/loongarch/Kconfig.debug
> @@ -28,6 +28,7 @@ config UNWINDER_PROLOGUE
>
> config UNWINDER_ORC
> bool "ORC unwinder"
> + depends on HAVE_OBJTOOL
> select OBJTOOL
> help
> This option enables the ORC (Oops Rewind Capability) unwinder for
> --
> 2.45.2
>
On Tue, 4 Jun 2024 23:07:41 +0800, Xi Ruoyao <[email protected]> wrote:
> GAS <= 2.41 does not support generating R_LARCH_{32,64}_PCREL for
> "label - ." and it generates R_LARCH_{ADD,SUB}{32,64} pairs instead.
> objtool cannot handle R_LARCH_{ADD,SUB}{32,64} pair in __jump_table
> (static key implementation) and etc. so it will produce some warnings.
> This is causing the kernel CI systems to complain everywhere.
>
> For GAS we can check if -mthin-add-sub option is available to know if
> R_LARCH_{32,64}_PCREL are supported.
>
> For Clang, we require Clang >= 18 and Clang >= 17 already supports
> R_LARCH_{32,64}_PCREL, so we can always assume Clang is fine for
> objtool.
>
> Note that __jump_table is not generated by the compiler so
> -fno-jump-table is completely irrelevant for this issue.
>
Hi Xi Ruoyao,
I want to know does this patch fix the following warning:
https://lore.kernel.org/all/[email protected]/ ?
Thanks.
> Fixes: cb8a2ef0848c ("LoongArch: Add ORC stack unwinder support")
> Closes: https://lore.kernel.org/loongarch/Zl5m1ZlVmGKitAof@yujie-X299/
> Closes: https://lore.kernel.org/loongarch/[email protected]/
> Closes: https://lore.kernel.org/loongarch/[email protected]/
> Link: https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=816029e06768
> Link: https://github.com/llvm/llvm-project/commit/42cb3c6346fc
> Signed-off-by: Xi Ruoyao <[email protected]>
> ---
> arch/loongarch/Kconfig | 5 ++++-
> arch/loongarch/Kconfig.debug | 1 +
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
> index e38139c576ee..e461a6c59f15 100644
> --- a/arch/loongarch/Kconfig
> +++ b/arch/loongarch/Kconfig
> @@ -143,7 +143,7 @@ config LOONGARCH
> select HAVE_LIVEPATCH
> select HAVE_MOD_ARCH_SPECIFIC
> select HAVE_NMI
> - select HAVE_OBJTOOL if AS_HAS_EXPLICIT_RELOCS
> + select HAVE_OBJTOOL if AS_HAS_EXPLICIT_RELOCS && AS_HAS_THIN_ADD_SUB
> select HAVE_PCI
> select HAVE_PERF_EVENTS
> select HAVE_PERF_REGS
> @@ -273,6 +273,9 @@ config AS_HAS_LBT_EXTENSION
> config AS_HAS_LVZ_EXTENSION
> def_bool $(as-instr,hvcl 0)
>
> +config AS_HAS_THIN_ADD_SUB
> + def_bool $(cc-option,-Wa$(comma)-mthin-add-sub) || CC_IS_CLANG
> +
> menu "Kernel type and options"
>
> source "kernel/Kconfig.hz"
> diff --git a/arch/loongarch/Kconfig.debug b/arch/loongarch/Kconfig.debug
> index 98d60630c3d4..8b2ce5b5d43e 100644
> --- a/arch/loongarch/Kconfig.debug
> +++ b/arch/loongarch/Kconfig.debug
> @@ -28,6 +28,7 @@ config UNWINDER_PROLOGUE
>
> config UNWINDER_ORC
> bool "ORC unwinder"
> + depends on HAVE_OBJTOOL
> select OBJTOOL
> help
> This option enables the ORC (Oops Rewind Capability) unwinder for
> --
> 2.45.2
>
On Wed, 2024-06-05 at 09:52 +0800, Huacai Chen wrote:
> Hi, Ruoyao,
>
> On Tue, Jun 4, 2024 at 11:08 PM Xi Ruoyao <[email protected]> wrote:
> >
> > GAS <= 2.41 does not support generating R_LARCH_{32,64}_PCREL for
> > "label - ." and it generates R_LARCH_{ADD,SUB}{32,64} pairs instead.
> > objtool cannot handle R_LARCH_{ADD,SUB}{32,64} pair in __jump_table
> > (static key implementation) and etc. so it will produce some warnings.
> > This is causing the kernel CI systems to complain everywhere.
> >
> > For GAS we can check if -mthin-add-sub option is available to know if
> > R_LARCH_{32,64}_PCREL are supported.
> I think we should give a chance to toolchains without the
> -mthin-add-sub option, so I hope Tiezhu can resubmit this patch to
> solve this problem.
> https://lore.kernel.org/loongarch/[email protected]/
It only handles .discard.{un,}reachable but not __jump_table.
__jump_table is our main issue here. And I don't see a quick way to
make -mno-thin-add-sub work for __jump_table because we really need to
resolve the relocation for __jump_table instead of simply skipping it.
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University
On Wed, Jun 5, 2024 at 12:38 PM Xi Ruoyao <[email protected]> wrote:
>
> On Wed, 2024-06-05 at 09:52 +0800, Huacai Chen wrote:
> > Hi, Ruoyao,
> >
> > On Tue, Jun 4, 2024 at 11:08 PM Xi Ruoyao <[email protected]> wrote:
> > >
> > > GAS <= 2.41 does not support generating R_LARCH_{32,64}_PCREL for
> > > "label - ." and it generates R_LARCH_{ADD,SUB}{32,64} pairs instead.
> > > objtool cannot handle R_LARCH_{ADD,SUB}{32,64} pair in __jump_table
> > > (static key implementation) and etc. so it will produce some warnings.
> > > This is causing the kernel CI systems to complain everywhere.
> > >
> > > For GAS we can check if -mthin-add-sub option is available to know if
> > > R_LARCH_{32,64}_PCREL are supported.
> > I think we should give a chance to toolchains without the
> > -mthin-add-sub option, so I hope Tiezhu can resubmit this patch to
> > solve this problem.
> > https://lore.kernel.org/loongarch/[email protected]/
>
> It only handles .discard.{un,}reachable but not __jump_table.
> __jump_table is our main issue here. And I don't see a quick way to
> make -mno-thin-add-sub work for __jump_table because we really need to
> resolve the relocation for __jump_table instead of simply skipping it.
__jump_table is disabled now, so we can only solve -mno-thin-add-sub
at present, and the next step is __jump_table.
Huacai
>
> --
> Xi Ruoyao <[email protected]>
> School of Aerospace Science and Technology, Xidian University
>
On Wed, 2024-06-05 at 13:21 +0800, Huacai Chen wrote:
> On Wed, Jun 5, 2024 at 12:38 PM Xi Ruoyao <[email protected]> wrote:
> >
> > On Wed, 2024-06-05 at 09:52 +0800, Huacai Chen wrote:
> > > Hi, Ruoyao,
> > >
> > > On Tue, Jun 4, 2024 at 11:08 PM Xi Ruoyao <[email protected]> wrote:
> > > >
> > > > GAS <= 2.41 does not support generating R_LARCH_{32,64}_PCREL for
> > > > "label - ." and it generates R_LARCH_{ADD,SUB}{32,64} pairs instead.
> > > > objtool cannot handle R_LARCH_{ADD,SUB}{32,64} pair in __jump_table
> > > > (static key implementation) and etc. so it will produce some warnings.
> > > > This is causing the kernel CI systems to complain everywhere.
> > > >
> > > > For GAS we can check if -mthin-add-sub option is available to know if
> > > > R_LARCH_{32,64}_PCREL are supported.
> > > I think we should give a chance to toolchains without the
> > > -mthin-add-sub option, so I hope Tiezhu can resubmit this patch to
> > > solve this problem.
> > > https://lore.kernel.org/loongarch/[email protected]/
> >
> > It only handles .discard.{un,}reachable but not __jump_table.
> > __jump_table is our main issue here. And I don't see a quick way to
> > make -mno-thin-add-sub work for __jump_table because we really need to
> > resolve the relocation for __jump_table instead of simply skipping it.
> __jump_table is disabled now, so we can only solve -mno-thin-add-sub
> at present, and the next step is __jump_table.
No, -fno-jump-table does not disable __jump_table. __jump_table is from
static key implementation in arch/loongarch/include/asm/jump_label.h,
not generated by the compiler.
And the problem of __jump_table is exact with -mno-thin-add-sub. It
reads:
#define JUMP_TABLE_ENTRY \
".pushsection __jump_table, \"aw\" \n\t" \
".align 3 \n\t" \
".long 1b - ., %l[l_yes] - . \n\t" \
".quad %0 - . \n\t" \
".popsection \n\t"
The "1b - ." and "%0 - ." things produce different relocs with -mno-
thin-add-sub and -mthin-add-sub. The objtool can only handle the relocs
from -mthin-add-sub here.
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University
On Tue, Jun 04, 2024 at 11:07:41PM +0800, Xi Ruoyao wrote:
> GAS <= 2.41 does not support generating R_LARCH_{32,64}_PCREL for
> "label - ." and it generates R_LARCH_{ADD,SUB}{32,64} pairs instead.
> objtool cannot handle R_LARCH_{ADD,SUB}{32,64} pair in __jump_table
> (static key implementation) and etc. so it will produce some warnings.
> This is causing the kernel CI systems to complain everywhere.
>
> For GAS we can check if -mthin-add-sub option is available to know if
> R_LARCH_{32,64}_PCREL are supported.
>
> For Clang, we require Clang >= 18 and Clang >= 17 already supports
> R_LARCH_{32,64}_PCREL, so we can always assume Clang is fine for
> objtool.
For what it's worth, I have noticed some warnings with clang that I
don't see with GCC but I only filed an issue on our GitHub and never
followed up on the mailing list, so sorry about that.
https://github.com/ClangBuiltLinux/linux/issues/2024
Might be tangential to this patch though but I felt it was worth
mentioning.
Cheers,
Nathan
On Tue, 2024-06-04 at 22:43 -0700, Nathan Chancellor wrote:
> On Tue, Jun 04, 2024 at 11:07:41PM +0800, Xi Ruoyao wrote:
> > GAS <= 2.41 does not support generating R_LARCH_{32,64}_PCREL for
> > "label - ." and it generates R_LARCH_{ADD,SUB}{32,64} pairs instead.
> > objtool cannot handle R_LARCH_{ADD,SUB}{32,64} pair in __jump_table
> > (static key implementation) and etc. so it will produce some warnings.
> > This is causing the kernel CI systems to complain everywhere.
> >
> > For GAS we can check if -mthin-add-sub option is available to know if
> > R_LARCH_{32,64}_PCREL are supported.
> >
> > For Clang, we require Clang >= 18 and Clang >= 17 already supports
> > R_LARCH_{32,64}_PCREL, so we can always assume Clang is fine for
> > objtool.
>
> For what it's worth, I have noticed some warnings with clang that I
> don't see with GCC but I only filed an issue on our GitHub and never
> followed up on the mailing list, so sorry about that.
>
> https://github.com/ClangBuiltLinux/linux/issues/2024
>
> Might be tangential to this patch though but I felt it was worth
> mentioning.
The warnings in GCC build is definitely the issue handled by this patch.
But the warnings in Clang build should be a different issue. Can you
attach the kernel/events/core.o file from the Clang build for analysis?
I guess we need to disable more optimization...
I personally hate "pessimizing the code generation just for debugging"
with a passion so I never enabled objtool on LoongArch, thus I didn't
notice the issue.
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University
On Wed, Jun 05, 2024 at 01:54:24PM +0800, Xi Ruoyao wrote:
> On Tue, 2024-06-04 at 22:43 -0700, Nathan Chancellor wrote:
> > For what it's worth, I have noticed some warnings with clang that I
> > don't see with GCC but I only filed an issue on our GitHub and never
> > followed up on the mailing list, so sorry about that.
> >
> > https://github.com/ClangBuiltLinux/linux/issues/2024
> >
> > Might be tangential to this patch though but I felt it was worth
> > mentioning.
>
> The warnings in GCC build is definitely the issue handled by this patch.
> But the warnings in Clang build should be a different issue. Can you
> attach the kernel/events/core.o file from the Clang build for analysis?
> I guess we need to disable more optimization...
Sure thing. Let me know if there are any issues with the attachment.
Cheers,
Nathan
On Tue, 2024-06-04 at 23:25 -0700, Nathan Chancellor wrote:
> On Wed, Jun 05, 2024 at 01:54:24PM +0800, Xi Ruoyao wrote:
> > On Tue, 2024-06-04 at 22:43 -0700, Nathan Chancellor wrote:
> > > For what it's worth, I have noticed some warnings with clang that I
> > > don't see with GCC but I only filed an issue on our GitHub and never
> > > followed up on the mailing list, so sorry about that.
> > >
> > > https://github.com/ClangBuiltLinux/linux/issues/2024
> > >
> > > Might be tangential to this patch though but I felt it was worth
> > > mentioning.
> >
> > The warnings in GCC build is definitely the issue handled by this patch.
> > But the warnings in Clang build should be a different issue. Can you
> > attach the kernel/events/core.o file from the Clang build for analysis?
> > I guess we need to disable more optimization...
>
> Sure thing. Let me know if there are any issues with the attachment.
Thanks! I've simplified it and now even...
.global test
.type test,@function
test:
addi.d $sp,$sp,-448
st.d $ra,$sp,440
st.d $fp,$sp,432
addi.d $fp,$sp,448
# do something
addi.d $sp,$fp,-448
ld.d $fp,$sp,432
ld.d $ra,$sp,440
addi.d $sp,$sp,448
ret
.size test,.-test
is enough to trigger a objtool warning:
/home/xry111/t1.o: warning: objtool: test+0x20: return with modified stack frame
And to me this warning is bogus?
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University
On Wed, 2024-06-05 at 18:57 +0800, Xi Ruoyao wrote:
> On Tue, 2024-06-04 at 23:25 -0700, Nathan Chancellor wrote:
> > On Wed, Jun 05, 2024 at 01:54:24PM +0800, Xi Ruoyao wrote:
> > > On Tue, 2024-06-04 at 22:43 -0700, Nathan Chancellor wrote:
> > > > For what it's worth, I have noticed some warnings with clang that I
> > > > don't see with GCC but I only filed an issue on our GitHub and never
> > > > followed up on the mailing list, so sorry about that.
> > > >
> > > > https://github.com/ClangBuiltLinux/linux/issues/2024
> > > >
> > > > Might be tangential to this patch though but I felt it was worth
> > > > mentioning.
> > >
> > > The warnings in GCC build is definitely the issue handled by this patch.
> > > But the warnings in Clang build should be a different issue. Can you
> > > attach the kernel/events/core.o file from the Clang build for analysis?
> > > I guess we need to disable more optimization...
> >
> > Sure thing. Let me know if there are any issues with the attachment.
>
> Thanks! I've simplified it and now even...
>
> .global test
> .type test,@function
> test:
>
> addi.d $sp,$sp,-448
> st.d $ra,$sp,440
> st.d $fp,$sp,432
> addi.d $fp,$sp,448
>
> # do something
>
> addi.d $sp,$fp,-448
> ld.d $fp,$sp,432
> ld.d $ra,$sp,440
> addi.d $sp,$sp,448
> ret
>
> .size test,.-test
>
> is enough to trigger a objtool warning:
>
> /home/xry111/t1.o: warning: objtool: test+0x20: return with modified stack frame
>
> And to me this warning is bogus?
Minimal C reproducer:
struct x { _Alignas(64) char buf[128]; };
void f(struct x *p);
void g()
{
struct x x = { .buf = "1145141919810" };
f(&x);
}
Then objtool is unhappy to the object file produced with "clang -c -O2"
from this translation unit:
/home/xry111/t2.o: warning: objtool: g+0x50: return with modified stack frame
It seems CFI_BP has a very specific semantic in objtool and Clang does
not operates $fp in the expected way. I'm not sure about my conclusion
though. Maybe Peter can explain it better.
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University
On Wed, 2024-06-05 at 21:18 +0800, Xi Ruoyao wrote:
> On Wed, 2024-06-05 at 18:57 +0800, Xi Ruoyao wrote:
> > On Tue, 2024-06-04 at 23:25 -0700, Nathan Chancellor wrote:
> > > On Wed, Jun 05, 2024 at 01:54:24PM +0800, Xi Ruoyao wrote:
> > > > On Tue, 2024-06-04 at 22:43 -0700, Nathan Chancellor wrote:
> > > > > For what it's worth, I have noticed some warnings with clang that I
> > > > > don't see with GCC but I only filed an issue on our GitHub and never
> > > > > followed up on the mailing list, so sorry about that.
> > > > >
> > > > > https://github.com/ClangBuiltLinux/linux/issues/2024
> > > > >
> > > > > Might be tangential to this patch though but I felt it was worth
> > > > > mentioning.
> > > >
> > > > The warnings in GCC build is definitely the issue handled by this patch.
> > > > But the warnings in Clang build should be a different issue. Can you
> > > > attach the kernel/events/core.o file from the Clang build for analysis?
> > > > I guess we need to disable more optimization...
> > >
> > > Sure thing. Let me know if there are any issues with the attachment.
> >
> > Thanks! I've simplified it and now even...
> >
> > .global test
> > .type test,@function
> > test:
> >
> > addi.d $sp,$sp,-448
> > st.d $ra,$sp,440
> > st.d $fp,$sp,432
> > addi.d $fp,$sp,448
> >
> > # do something
> >
> > addi.d $sp,$fp,-448
> > ld.d $fp,$sp,432
> > ld.d $ra,$sp,440
> > addi.d $sp,$sp,448
> > ret
> >
> > .size test,.-test
> >
> > is enough to trigger a objtool warning:
> >
> > /home/xry111/t1.o: warning: objtool: test+0x20: return with modified stack frame
> >
> > And to me this warning is bogus?
>
> Minimal C reproducer:
>
> struct x { _Alignas(64) char buf[128]; };
>
> void f(struct x *p);
> void g()
> {
> struct x x = { .buf = "1145141919810" };
> f(&x);
> }
>
> Then objtool is unhappy to the object file produced with "clang -c -O2"
> from this translation unit:
>
> /home/xry111/t2.o: warning: objtool: g+0x50: return with modified stack frame
>
> It seems CFI_BP has a very specific semantic in objtool and Clang does
> not operates $fp in the expected way. I'm not sure about my conclusion
> though. Maybe Peter can explain it better.
Another example: some simple rust code:
extern { fn f(x: &i64) -> i64; }
#[no_mangle]
fn g() -> i64 {
let x = 114514;
unsafe {f(&x)}
}
It's just lucky GCC doesn't use $fp as the frame pointer unless there's
some stupid code (VLA etc) thus the issue was not detected.
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University
On 2024-06-05 23:13, Xi Ruoyao wrote:
> On Wed, 2024-06-05 at 21:18 +0800, Xi Ruoyao wrote:
>> On Wed, 2024-06-05 at 18:57 +0800, Xi Ruoyao wrote:
>>> On Tue, 2024-06-04 at 23:25 -0700, Nathan Chancellor wrote:
>>>> On Wed, Jun 05, 2024 at 01:54:24PM +0800, Xi Ruoyao wrote:
>>>>> On Tue, 2024-06-04 at 22:43 -0700, Nathan Chancellor wrote:
>>>>>> For what it's worth, I have noticed some warnings with clang that I
>>>>>> don't see with GCC but I only filed an issue on our GitHub and never
>>>>>> followed up on the mailing list, so sorry about that.
>>>>>>
>>>>>> https://github.com/ClangBuiltLinux/linux/issues/2024
>>>>>>
>>>>>> Might be tangential to this patch though but I felt it was worth
>>>>>> mentioning.
>>>>> The warnings in GCC build is definitely the issue handled by this patch.
>>>>> But the warnings in Clang build should be a different issue. Can you
>>>>> attach the kernel/events/core.o file from the Clang build for analysis?
>>>>> I guess we need to disable more optimization...
>>>> Sure thing. Let me know if there are any issues with the attachment.
>>> Thanks! I've simplified it and now even...
>>>
>>> .global test
>>> .type test,@function
>>> test:
>>>
>>> addi.d $sp,$sp,-448
>>> st.d $ra,$sp,440
>>> st.d $fp,$sp,432
>>> addi.d $fp,$sp,448
>>>
>>> # do something
>>>
>>> addi.d $sp,$fp,-448
>>> ld.d $fp,$sp,432
>>> ld.d $ra,$sp,440
>>> addi.d $sp,$sp,448
>>> ret
>>>
>>> .size test,.-test
>>>
>>> is enough to trigger a objtool warning:
>>>
>>> /home/xry111/t1.o: warning: objtool: test+0x20: return with modified stack frame
>>>
>>> And to me this warning is bogus?
>> Minimal C reproducer:
>>
>> struct x { _Alignas(64) char buf[128]; };
>>
>> void f(struct x *p);
>> void g()
>> {
>> struct x x = { .buf = "1145141919810" };
>> f(&x);
>> }
>>
>> Then objtool is unhappy to the object file produced with "clang -c -O2"
>> from this translation unit:
>>
>> /home/xry111/t2.o: warning: objtool: g+0x50: return with modified stack frame
>>
>> It seems CFI_BP has a very specific semantic in objtool and Clang does
>> not operates $fp in the expected way. I'm not sure about my conclusion
>> though. Maybe Peter can explain it better.
> Another example: some simple rust code:
>
> extern { fn f(x: &i64) -> i64; }
>
> #[no_mangle]
> fn g() -> i64 {
> let x = 114514;
> unsafe {f(&x)}
> }
>
> It's just lucky GCC doesn't use $fp as the frame pointer unless there's
> some stupid code (VLA etc) thus the issue was not detected.
>
>
I think this because we did not add arch special handle in
update_cfi_state().
In our 419 repo this func has been renamed arch_update_insn_state (, now it
should be arch_update_cfi_state) and give some actions to deal with the
frame pointer case. If we support it we may deal with some case but for
clang...
.global test
.type test,@function
test:
addi.d $sp,$sp,-448
st.d $ra,$sp,440
st.d $fp,$sp,432
addi.d $fp,$sp,448
# do something <- not change $sp
# addi.d $sp,$fp,-448 <- not restore sp from cfa(fp)
ld.d $fp,$sp,432
ld.d $ra,$sp,440
addi.d $sp,$sp,448
ret
.size test,.-test
This will cause warning, too. (Gcc should be ok, I don't test.)
source.c: (clang 19, based 0c1c0d53931, -g)
void *foo() { return __builtin_frame_address(0); }
asm.s:
.cfi_sections .debug_frame
.cfi_startproc
# %bb.0:
addi.d $sp, $sp, -16
.cfi_def_cfa_offset 16
st.d $ra, $sp, 8 # 8-byte Folded Spill
st.d $fp, $sp, 0 # 8-byte Folded Spill
.cfi_offset 1, -8
.cfi_offset 22, -16
addi.d $fp, $sp, 16
.cfi_def_cfa 22, 0 <- define cfa is $r22
.Ltmp0:
.loc 0 1 22 prologue_end # hello.c:1:22
move $a0, $fp
ld.d $fp, $sp, 0 # 8-byte Folded Reload
<- restore regs from non-cfa ?
ld.d $ra, $sp, 8 # 8-byte Folded Reload
<- restore regs from non-cfa ?
.loc 0 1 15 epilogue_begin is_stmt 0 # hello.c:1:15
addi.d $sp, $sp, 16
<- restore prev-sp from non-cfa ?
ret
Maybe Clang have anything wrong?
Attach: tmp fix by support arch_update_insn_state.
Thanks,
Jinyang
On Wed, 2024-06-05 at 23:47 +0800, Jinyang He wrote:
> In our 419 repo this func has been renamed arch_update_insn_state (, now it
> should be arch_update_cfi_state) and give some actions to deal with the
> frame pointer case. If we support it we may deal with some case but for
> clang...
>
> .global test
> .type test,@function
> test:
>
> addi.d $sp,$sp,-448
> st.d $ra,$sp,440
> st.d $fp,$sp,432
> addi.d $fp,$sp,448
>
> # do something <- not change $sp
This is simplified. In the real code $sp is changed, something like:
bstrins.d $sp, $zero, 5, 0
$fp is needed to allow restoring $sp after realigning $sp for some local
over-aligned variables, as demonstrated by this example:
struct x { _Alignas(64) char buf[128]; };
void f(struct x *p);
void g()
{
struct x x = { .buf = "1145141919810" };
f(&x);
}
GCC does not align $sp, it produces the aligned address for x from an
unaligned $sp instead:
addi.d $a0, $sp, 63
srli.d $a0, $a0, 6
slli.d $a0, $a0, 6
thus there's no need to use $fp.
/* snip */
> <- restore regs from non-cfa ?
> ld.d $ra, $sp, 8 # 8-byte Folded Reload
/* snip */
> Maybe Clang have anything wrong?
I don't think we must restore regs based on $fp just because CFA is
based on $fp. The .cfi directives only provides *one* possible way to
restore the regs. This way is convenient to the unwinder, but not
necessarily convenient to the CPU thus the real instruction sequence can
use a different way. They only need to be "equivalent", not necessarily
"exactly same."
Also note that .cfi_* directives are completely irrelevant for objtool.
THe objtool synthesizes the ORC unwind info from the machine
instructions, not the DWARF unwind info coded with .cfi_*.
The entire point of ORC is avoiding DWARF. It's even named ORC because
ORC and DWARF are sworn enemies in some tales.
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University
On 2024-06-06 03:05, Xi Ruoyao wrote:
> On Wed, 2024-06-05 at 23:47 +0800, Jinyang He wrote:
>> In our 419 repo this func has been renamed arch_update_insn_state (, now it
>> should be arch_update_cfi_state) and give some actions to deal with the
>> frame pointer case. If we support it we may deal with some case but for
>> clang...
>>
>> .global test
>> .type test,@function
>> test:
>>
>> addi.d $sp,$sp,-448
>> st.d $ra,$sp,440
>> st.d $fp,$sp,432
>> addi.d $fp,$sp,448
>>
>> # do something <- not change $sp
> This is simplified. In the real code $sp is changed, something like:
>
> bstrins.d $sp, $zero, 5, 0
>
> $fp is needed to allow restoring $sp after realigning $sp for some local
> over-aligned variables, as demonstrated by this example:
>
> struct x { _Alignas(64) char buf[128]; };
>
> void f(struct x *p);
> void g()
> {
> struct x x = { .buf = "1145141919810" };
> f(&x);
> }
>
> GCC does not align $sp, it produces the aligned address for x from an
> unaligned $sp instead:
>
> addi.d $a0, $sp, 63
> srli.d $a0, $a0, 6
> slli.d $a0, $a0, 6
>
> thus there's no need to use $fp.
>
> /* snip */
>
>> <- restore regs from non-cfa ?
>> ld.d $ra, $sp, 8 # 8-byte Folded Reload
> /* snip */
>
>> Maybe Clang have anything wrong?
> I don't think we must restore regs based on $fp just because CFA is
> based on $fp.
You are right.
> The .cfi directives only provides *one* possible way to
> restore the regs.
What I just confused is that there is no ".cfi_*"
in the eplogue by clang, which may cause wrong backtrace if gdb set
breakpoint there and backtrace. (But this is out of this topic.)
> This way is convenient to the unwinder, but not
> necessarily convenient to the CPU thus the real instruction sequence can
> use a different way. They only need to be "equivalent", not necessarily
> "exactly same."
>
> Also note that .cfi_* directives are completely irrelevant for objtool.
> THe objtool synthesizes the ORC unwind info from the machine
> instructions, not the DWARF unwind info coded with .cfi_*.
> The entire point of ORC is avoiding DWARF. It's even named ORC because
> ORC and DWARF are sworn enemies in some tales.
Yes.
The yestorday attachment has something wrong.
Enmmmm.... (It seems I'm careless.) diff is,
switch (op->src.type) {
case OP_SRC_ADD:
if (op->dest.reg == CFI_SP && op->src.reg ==
CFI_SP) {
- /* addi.d sp, sp, imm */
- cfi->stack_size -= op->src.offset;
- if (cfa->base == CFI_SP)
/* addi.d sp, sp, imm */
cfi->stack_size -= op->src.offset;
if (cfa->base == CFI_SP)
Or you can get the tmp fix by [1].
[1]:
https://github.com/MQ-mengqing/linux/commit/c1f7df3eb2a2bb7a1c10c2aa6e0e3d585b457352
On Thu, 2024-06-06 at 10:10 +0800, Jinyang He wrote:
> What I just confused is that there is no ".cfi_*"
> in the eplogue by clang, which may cause wrong backtrace if gdb set
>
> breakpoint there and backtrace. (But this is out of this topic.)
I don't think it'll cause wrong backtrace. The real assemble code has
restored the registers and missing .cfi_restore will just make unwinder
restore them again. There are redundant works but not breakages.
For objtool the main difference seems a thing explained in
https://maskray.me/blog/2020-11-08-stack-unwinding by Fangrui:
Note: on RISC-V and LoongArch, the stack slot for the previous frame
pointer is stored at fp[-2] instead of fp[0]. See [Consider
standardising which stack slot fp points
to](https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/18)
for the RISC-V discussion.
So perhaps we just need to code a constant named "PREV_BP_OFFSET" or
something in arch/ and use it in update_cfi_state() instead of fully re-
implement the entire function?
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University
On 2024-06-07 13:42, Xi Ruoyao wrote:
> On Thu, 2024-06-06 at 10:10 +0800, Jinyang He wrote:
>> What I just confused is that there is no ".cfi_*"
>> in the eplogue by clang, which may cause wrong backtrace if gdb set
>>
>> breakpoint there and backtrace. (But this is out of this topic.)
> I don't think it'll cause wrong backtrace. The real assemble code has
> restored the registers and missing .cfi_restore will just make unwinder
> restore them again. There are redundant works but not breakages.
$ cat hello.c:
extern void __attribute__((noinline)) foo() {}
int main() {
foo();
return (int)(long)__builtin_frame_address(0);
}
$ clang hello.c -S -g -o hello.s -O0 -fPIC
$ cat hello.s:
[...]
addi.d $sp, $sp, -32
.cfi_def_cfa_offset 32
st.d $ra, $sp, 24 # 8-byte Folded Spill
st.d $fp, $sp, 16 # 8-byte Folded Spill
.cfi_offset 1, -8
.cfi_offset 22, -16
addi.d $fp, $sp, 32
.cfi_def_cfa 22, 0
move $a0, $zero
st.w $a0, $fp, -20
.Ltmp2:
.loc 0 3 3 prologue_end # hello.c:3:3
bl %plt(foo)
.loc 0 4 21 # hello.c:4:21
move $a0, $fp
.loc 0 4 3 is_stmt 0 # hello.c:4:3
addi.w $a0, $a0, 0
ld.d $fp, $sp, 16 # 8-byte Folded Reload
<use gdb and set break ponint there>
ld.d $ra, $sp, 24 # 8-byte Folded Reload
.loc 0 4 3 epilogue_begin # hello.c:4:3
addi.d $sp, $sp, 32
ret
[...]
So how unwinder do if we <use gdb and set break ponint there>? I think
if we not give ".cfi_restore 22" or others info, it will consider
1) the $ra is in cfa-8
2) the cfa is $fp
So it will get $ra by $ra = *(long*)($fp-8). So it may unwind failed
because $fp has been restored and not the CFA now.
> For objtool the main difference seems a thing explained in
> https://maskray.me/blog/2020-11-08-stack-unwinding by Fangrui:
>
> Note: on RISC-V and LoongArch, the stack slot for the previous frame
> pointer is stored at fp[-2] instead of fp[0]. See [Consider
> standardising which stack slot fp points
> to](https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/18)
> for the RISC-V discussion.
In most cases the $fp is saved at cfa-16. But for va args, something
becomes different at LoongArch (I do not know the case of riscv), the
$fp isn't saved at cfa-16. (e.g. printk?)
> So perhaps we just need to code a constant named "PREV_BP_OFFSET"
Can you give some detail?
> or
> something in arch/ and use it in update_cfi_state() instead of fully re-
> implement the entire function?
I feel that the update_cfi_state should be arch specific. I believe
that some logic can be reused, but each arch may have its own logic.
Thanks,
Jinyang
On Fri, 2024-06-07 at 15:14 +0800, Jinyang He wrote:
> > Note: on RISC-V and LoongArch, the stack slot for the previous frame
> > pointer is stored at fp[-2] instead of fp[0]. See [Consider
> > standardising which stack slot fp points
> > to](https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/18)
> > for the RISC-V discussion.
>
> In most cases the $fp is saved at cfa-16. But for va args, something
> becomes different at LoongArch (I do not know the case of riscv), the
> $fp isn't saved at cfa-16. (e.g. printk?)
Oops indeed. Even with a very simple case:
int sum(int a, int b) {
return a + b;
}
with -fno-omit-frame-pointer we get:
sum:
addi.d $r3,$r3,-16
st.d $r22,$r3,8
addi.d $r22,$r3,16
ld.d $r22,$r3,8
add.w $r4,$r4,$r5
addi.d $r3,$r3,16
jr $r1
So for leaf functions (where we don't save $ra) $fp is saved at cfa-8.
> I feel that the update_cfi_state should be arch specific. I believe
> that some logic can be reused, but each arch may have its own logic.
I agree it now.
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University
Hi, Ruoyao and Jinyang,
On Fri, Jun 7, 2024 at 4:29 PM Xi Ruoyao <[email protected]> wrote:
>
> On Fri, 2024-06-07 at 15:14 +0800, Jinyang He wrote:
> > > Note: on RISC-V and LoongArch, the stack slot for the previous frame
> > > pointer is stored at fp[-2] instead of fp[0]. See [Consider
> > > standardising which stack slot fp points
> > > to](https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/18)
> > > for the RISC-V discussion.
> >
> > In most cases the $fp is saved at cfa-16. But for va args, something
> > becomes different at LoongArch (I do not know the case of riscv), the
> > $fp isn't saved at cfa-16. (e.g. printk?)
>
> Oops indeed. Even with a very simple case:
>
> int sum(int a, int b) {
> return a + b;
> }
>
> with -fno-omit-frame-pointer we get:
>
> sum:
> addi.d $r3,$r3,-16
> st.d $r22,$r3,8
> addi.d $r22,$r3,16
> ld.d $r22,$r3,8
> add.w $r4,$r4,$r5
> addi.d $r3,$r3,16
> jr $r1
>
> So for leaf functions (where we don't save $ra) $fp is saved at cfa-8.
>
> > I feel that the update_cfi_state should be arch specific. I believe
> > that some logic can be reused, but each arch may have its own logic.
>
> I agree it now.
What is the conclusion about the clang part now? And for the original
-mno-thin-add-sub problem, do you have some way to fix it in the root?
I think we needn't rush, there are some weeks before 6.10 released.
Huacai
>
> --
> Xi Ruoyao <[email protected]>
> School of Aerospace Science and Technology, Xidian University
>
On Sat, 2024-06-15 at 16:45 +0800, Huacai Chen wrote:
> Hi, Ruoyao and Jinyang,
>
> On Fri, Jun 7, 2024 at 4:29 PM Xi Ruoyao <[email protected]> wrote:
> >
> > On Fri, 2024-06-07 at 15:14 +0800, Jinyang He wrote:
> > > > Note: on RISC-V and LoongArch, the stack slot for the previous frame
> > > > pointer is stored at fp[-2] instead of fp[0]. See [Consider
> > > > standardising which stack slot fp points
> > > > to](https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/18)
> > > > for the RISC-V discussion.
> > >
> > > In most cases the $fp is saved at cfa-16. But for va args, something
> > > becomes different at LoongArch (I do not know the case of riscv), the
> > > $fp isn't saved at cfa-16. (e.g. printk?)
> >
> > Oops indeed. Even with a very simple case:
> >
> > int sum(int a, int b) {
> > return a + b;
> > }
> >
> > with -fno-omit-frame-pointer we get:
> >
> > sum:
> > addi.d $r3,$r3,-16
> > st.d $r22,$r3,8
> > addi.d $r22,$r3,16
> > ld.d $r22,$r3,8
> > add.w $r4,$r4,$r5
> > addi.d $r3,$r3,16
> > jr $r1
> >
> > So for leaf functions (where we don't save $ra) $fp is saved at cfa-8.
> >
> > > I feel that the update_cfi_state should be arch specific. I believe
> > > that some logic can be reused, but each arch may have its own logic.
> >
> > I agree it now.
> What is the conclusion about the clang part now? And for the original
> -mno-thin-add-sub problem, do you have some way to fix it in the root?
> I think we needn't rush, there are some weeks before 6.10 released.
To me for now we should just make OBJTOOL and ORC depend on BROKEN and
backport to stable...
Even if we can fix both the -mno-thin-add-sub problem and the frame
pointer problem in these weeks, they'll be some nontrivial large change
and improper to backport. Thus we have to admit objtool doesn't really
work for old releases and mark it broken.
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University
On Sat, Jun 15, 2024 at 4:54 PM Xi Ruoyao <[email protected]> wrote:
>
> On Sat, 2024-06-15 at 16:45 +0800, Huacai Chen wrote:
> > Hi, Ruoyao and Jinyang,
> >
> > On Fri, Jun 7, 2024 at 4:29 PM Xi Ruoyao <[email protected]> wrote:
> > >
> > > On Fri, 2024-06-07 at 15:14 +0800, Jinyang He wrote:
> > > > > Note: on RISC-V and LoongArch, the stack slot for the previous frame
> > > > > pointer is stored at fp[-2] instead of fp[0]. See [Consider
> > > > > standardising which stack slot fp points
> > > > > to](https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/18)
> > > > > for the RISC-V discussion.
> > > >
> > > > In most cases the $fp is saved at cfa-16. But for va args, something
> > > > becomes different at LoongArch (I do not know the case of riscv), the
> > > > $fp isn't saved at cfa-16. (e.g. printk?)
> > >
> > > Oops indeed. Even with a very simple case:
> > >
> > > int sum(int a, int b) {
> > > return a + b;
> > > }
> > >
> > > with -fno-omit-frame-pointer we get:
> > >
> > > sum:
> > > addi.d $r3,$r3,-16
> > > st.d $r22,$r3,8
> > > addi.d $r22,$r3,16
> > > ld.d $r22,$r3,8
> > > add.w $r4,$r4,$r5
> > > addi.d $r3,$r3,16
> > > jr $r1
> > >
> > > So for leaf functions (where we don't save $ra) $fp is saved at cfa-8.
> > >
> > > > I feel that the update_cfi_state should be arch specific. I believe
> > > > that some logic can be reused, but each arch may have its own logic.
> > >
> > > I agree it now.
> > What is the conclusion about the clang part now? And for the original
> > -mno-thin-add-sub problem, do you have some way to fix it in the root?
> > I think we needn't rush, there are some weeks before 6.10 released.
>
> To me for now we should just make OBJTOOL and ORC depend on BROKEN and
> backport to stable...
But this patch allows clang to build objtool, which seems broken, too.
>
> Even if we can fix both the -mno-thin-add-sub problem and the frame
> pointer problem in these weeks, they'll be some nontrivial large change
> and improper to backport. Thus we have to admit objtool doesn't really
> work for old releases and mark it broken.
I don't like to disable objtool, unless there is no better solution.
And it seems there has already been some "large fix" in objtool's
history.
Huacai
>
>
> --
> Xi Ruoyao <[email protected]>
> School of Aerospace Science and Technology, Xidian University
On Sat, 2024-06-15 at 17:33 +0800, Huacai Chen wrote:
> > To me for now we should just make OBJTOOL and ORC depend on BROKEN and
> > backport to stable...
> But this patch allows clang to build objtool, which seems broken, too.
Yes, so I mean make objtool depend on CONFIG_BROKEN because it is indeed
broken as at now.
Or we'll end up at least:
select HAVE_OBJTOOL if AS_HAS_EXPLICIT_RELOCS && AS_HAS_THIN_ADD_SUB && !CC_IS_CLANG && !RUST
this is already nasty and I still don't know if it covers all broken
cases (I've no idea if GCC will generate frame pointer in some cases as
well...)
> > Even if we can fix both the -mno-thin-add-sub problem and the frame
> > pointer problem in these weeks, they'll be some nontrivial large change
> > and improper to backport. Thus we have to admit objtool doesn't really
> > work for old releases and mark it broken.
> I don't like to disable objtool, unless there is no better solution.
> And it seems there has already been some "large fix" in objtool's
> history.
Then we can still backport the large fix to the stable branches when we
finish it up.
--
Xi Ruoyao <[email protected]>
School of Aerospace Science and Technology, Xidian University