Hi Petr,
After merging the printk tree, today's linux-next build (bfin
BF518F-EZBRD_defconfig) failed like this:
lib/dump_stack.o: In function `dump_stack':
lib/dump_stack.c:122: multiple definition of `dump_stack'
arch/blackfin/kernel/dumpstack.o:arch/blackfin/kernel/dumpstack.c:166: first defined here
Presumably caused by commit
8040af489957 ("printk: move dump stack related code to lib/dump_stack.c")
(Though it is not immediately obvious why.)
This fails all the blackfin builds. nds32 (a new architecture) also
has a dump_stack function.
--
Cheers,
Stephen Rothwell
On Fri 2018-03-02 16:07:32, Stephen Rothwell wrote:
> Hi Petr,
>
> After merging the printk tree, today's linux-next build (bfin
> BF518F-EZBRD_defconfig) failed like this:
>
> lib/dump_stack.o: In function `dump_stack':
> lib/dump_stack.c:122: multiple definition of `dump_stack'
> arch/blackfin/kernel/dumpstack.o:arch/blackfin/kernel/dumpstack.c:166: first defined here
>
> Presumably caused by commit
>
> 8040af489957 ("printk: move dump stack related code to lib/dump_stack.c")
I could confirm that it is caused by this commit. I have temporary
removed it from printk.git.
> (Though it is not immediately obvious why.)
It is a mistery to me. The error appears when I move any of
dump_stack_print_info() or show_regs_print_info() function
definitions from kernel/printk/printk.c to lib/dump_stack.c.
All the other changes seems unrelated.
The thing is that we basically do not touch dump_stack() definition
by that patch.
> This fails all the blackfin builds. nds32 (a new architecture) also
> has a dump_stack function.
Good to know!
Best Regards,
Petr
Hi Petr,
On Fri, 2 Mar 2018 16:54:54 +0100 Petr Mladek <[email protected]> wrote:
>
> It is a mistery to me. The error appears when I move any of
> dump_stack_print_info() or show_regs_print_info() function
> definitions from kernel/printk/printk.c to lib/dump_stack.c.
> All the other changes seems unrelated.
Presumably because the Blackfin dumpstack() calls
dump_stack_print_info() so if you move that into lib/dumpstack.o, then
that file is dragged in and it contains another copy of dumpstack().
--
Cheers,
Stephen Rothwell
Cc-ing Tejun
On (03/02/18 16:54), Petr Mladek wrote:
[..]
> > (Though it is not immediately obvious why.)
>
> It is a mistery to me. The error appears when I move any of
> dump_stack_print_info() or show_regs_print_info() function
> definitions from kernel/printk/printk.c to lib/dump_stack.c.
> All the other changes seems unrelated.
>
> The thing is that we basically do not touch dump_stack() definition
> by that patch.
Apparently dump_stack_print_info() was in lib/dump_stack.c a long
time ago, but it was deliberately moved to printk.c, when kernel gained
a "generic" (dummy) dump_stack() fallback. Some archs, like blackfin,
define their own dump_stack() symbol and make it global via EXPORT_SYMBOL.
In case of blackfin that arch-specific dump_stack() symbol invokes a
global dump_stack_print_info(). If we move dump_stack_print_info() back
to lib/dump_stack.c then we link both with arch/blackfin/dumpstack.o
and lib/dump_stack.o, which results in multiple definitions error.
If we move dump_stack_print_info() out on libdump_stack.o, then we
never link with lib/dump_stack.o
... so what are we going to do with that.
a) we can drop the patch and cherry pick only the kexec part
b) we can try to mark dummy lib/dump_stack() as __weak
EXPORT_SYMBOL and remove EXPORT_SYMBOL from arch-specific
definitions.
So we will end up with EXPORT_SYMBOL dump_stack() and archs
may re-define it. If some arch will accidentally mark its
own dump_stack() as EXPORT_SYMBOL then there should be a
linkage warning - a symbol is exported twice.
Something like below.
Opinions? Will this work?
========= 8< =========
From: Sergey Senozhatsky <[email protected]>
Subject: [PATCH] dump_stack: mark dummy dump_stack() as weak
---
arch/blackfin/kernel/dumpstack.c | 1 -
arch/nds32/kernel/traps.c | 2 --
lib/dump_stack.c | 4 ++--
3 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/arch/blackfin/kernel/dumpstack.c b/arch/blackfin/kernel/dumpstack.c
index 3c992c1f8ef2..61af017130cd 100644
--- a/arch/blackfin/kernel/dumpstack.c
+++ b/arch/blackfin/kernel/dumpstack.c
@@ -174,4 +174,3 @@ void dump_stack(void)
show_stack(current, &stack);
trace_buffer_restore(tflags);
}
-EXPORT_SYMBOL(dump_stack);
diff --git a/arch/nds32/kernel/traps.c b/arch/nds32/kernel/traps.c
index 8828b4aeb72b..455bb0787367 100644
--- a/arch/nds32/kernel/traps.c
+++ b/arch/nds32/kernel/traps.c
@@ -166,8 +166,6 @@ void dump_stack(void)
__dump(NULL, base_reg);
}
-EXPORT_SYMBOL(dump_stack);
-
void show_stack(struct task_struct *tsk, unsigned long *sp)
{
unsigned long *base_reg;
diff --git a/lib/dump_stack.c b/lib/dump_stack.c
index 5cff72f18c4a..9cf4465dbffa 100644
--- a/lib/dump_stack.c
+++ b/lib/dump_stack.c
@@ -85,7 +85,7 @@ static void __dump_stack(void)
#ifdef CONFIG_SMP
static atomic_t dump_lock = ATOMIC_INIT(-1);
-asmlinkage __visible void dump_stack(void)
+asmlinkage __weak __visible void dump_stack(void)
{
unsigned long flags;
int was_locked;
@@ -118,7 +118,7 @@ asmlinkage __visible void dump_stack(void)
local_irq_restore(flags);
}
#else
-asmlinkage __visible void dump_stack(void)
+asmlinkage __weak __visible void dump_stack(void)
{
__dump_stack();
}
--
2.16.2
On 03/03/18 at 11:47pm, Sergey Senozhatsky wrote:
> Cc-ing Tejun
>
> On (03/02/18 16:54), Petr Mladek wrote:
> [..]
> > > (Though it is not immediately obvious why.)
> >
> > It is a mistery to me. The error appears when I move any of
> > dump_stack_print_info() or show_regs_print_info() function
> > definitions from kernel/printk/printk.c to lib/dump_stack.c.
> > All the other changes seems unrelated.
> >
> > The thing is that we basically do not touch dump_stack() definition
> > by that patch.
>
> Apparently dump_stack_print_info() was in lib/dump_stack.c a long
> time ago, but it was deliberately moved to printk.c, when kernel gained
> a "generic" (dummy) dump_stack() fallback. Some archs, like blackfin,
> define their own dump_stack() symbol and make it global via EXPORT_SYMBOL.
>
> In case of blackfin that arch-specific dump_stack() symbol invokes a
> global dump_stack_print_info(). If we move dump_stack_print_info() back
> to lib/dump_stack.c then we link both with arch/blackfin/dumpstack.o
> and lib/dump_stack.o, which results in multiple definitions error.
> If we move dump_stack_print_info() out on libdump_stack.o, then we
> never link with lib/dump_stack.o
>
> ... so what are we going to do with that.
>
> a) we can drop the patch and cherry pick only the kexec part
>
> b) we can try to mark dummy lib/dump_stack() as __weak
> EXPORT_SYMBOL and remove EXPORT_SYMBOL from arch-specific
> definitions.
>
> So we will end up with EXPORT_SYMBOL dump_stack() and archs
> may re-define it. If some arch will accidentally mark its
> own dump_stack() as EXPORT_SYMBOL then there should be a
> linkage warning - a symbol is exported twice.
>
>
> Something like below.
>
> Opinions? Will this work?
I would think b) is better, thanks for the fix!
>
>
> ========= 8< =========
>
> From: Sergey Senozhatsky <[email protected]>
> Subject: [PATCH] dump_stack: mark dummy dump_stack() as weak
>
> ---
> arch/blackfin/kernel/dumpstack.c | 1 -
> arch/nds32/kernel/traps.c | 2 --
> lib/dump_stack.c | 4 ++--
> 3 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/arch/blackfin/kernel/dumpstack.c b/arch/blackfin/kernel/dumpstack.c
> index 3c992c1f8ef2..61af017130cd 100644
> --- a/arch/blackfin/kernel/dumpstack.c
> +++ b/arch/blackfin/kernel/dumpstack.c
> @@ -174,4 +174,3 @@ void dump_stack(void)
> show_stack(current, &stack);
> trace_buffer_restore(tflags);
> }
> -EXPORT_SYMBOL(dump_stack);
> diff --git a/arch/nds32/kernel/traps.c b/arch/nds32/kernel/traps.c
> index 8828b4aeb72b..455bb0787367 100644
> --- a/arch/nds32/kernel/traps.c
> +++ b/arch/nds32/kernel/traps.c
> @@ -166,8 +166,6 @@ void dump_stack(void)
> __dump(NULL, base_reg);
> }
>
> -EXPORT_SYMBOL(dump_stack);
> -
> void show_stack(struct task_struct *tsk, unsigned long *sp)
> {
> unsigned long *base_reg;
> diff --git a/lib/dump_stack.c b/lib/dump_stack.c
> index 5cff72f18c4a..9cf4465dbffa 100644
> --- a/lib/dump_stack.c
> +++ b/lib/dump_stack.c
> @@ -85,7 +85,7 @@ static void __dump_stack(void)
> #ifdef CONFIG_SMP
> static atomic_t dump_lock = ATOMIC_INIT(-1);
>
> -asmlinkage __visible void dump_stack(void)
> +asmlinkage __weak __visible void dump_stack(void)
> {
> unsigned long flags;
> int was_locked;
> @@ -118,7 +118,7 @@ asmlinkage __visible void dump_stack(void)
> local_irq_restore(flags);
> }
> #else
> -asmlinkage __visible void dump_stack(void)
> +asmlinkage __weak __visible void dump_stack(void)
> {
> __dump_stack();
> }
> --
> 2.16.2
>
Thanks
Dave
2018-03-05 11:20 GMT+08:00 Dave Young <[email protected]>:
> On 03/03/18 at 11:47pm, Sergey Senozhatsky wrote:
>> Cc-ing Tejun
>>
>> On (03/02/18 16:54), Petr Mladek wrote:
>> [..]
>> > > (Though it is not immediately obvious why.)
>> >
>> > It is a mistery to me. The error appears when I move any of
>> > dump_stack_print_info() or show_regs_print_info() function
>> > definitions from kernel/printk/printk.c to lib/dump_stack.c.
>> > All the other changes seems unrelated.
>> >
>> > The thing is that we basically do not touch dump_stack() definition
>> > by that patch.
>>
>> Apparently dump_stack_print_info() was in lib/dump_stack.c a long
>> time ago, but it was deliberately moved to printk.c, when kernel gained
>> a "generic" (dummy) dump_stack() fallback. Some archs, like blackfin,
>> define their own dump_stack() symbol and make it global via EXPORT_SYMBOL.
>>
>> In case of blackfin that arch-specific dump_stack() symbol invokes a
>> global dump_stack_print_info(). If we move dump_stack_print_info() back
>> to lib/dump_stack.c then we link both with arch/blackfin/dumpstack.o
>> and lib/dump_stack.o, which results in multiple definitions error.
>> If we move dump_stack_print_info() out on libdump_stack.o, then we
>> never link with lib/dump_stack.o
>>
>> ... so what are we going to do with that.
>>
>> a) we can drop the patch and cherry pick only the kexec part
>>
>> b) we can try to mark dummy lib/dump_stack() as __weak
>> EXPORT_SYMBOL and remove EXPORT_SYMBOL from arch-specific
>> definitions.
>>
>> So we will end up with EXPORT_SYMBOL dump_stack() and archs
>> may re-define it. If some arch will accidentally mark its
>> own dump_stack() as EXPORT_SYMBOL then there should be a
>> linkage warning - a symbol is exported twice.
>>
>>
>> Something like below.
>>
>> Opinions? Will this work?
>
> I would think b) is better, thanks for the fix!
>
Hi,
b works in nds32.
Thanks for the fix :)
>>
>>
>> ========= 8< =========
>>
>> From: Sergey Senozhatsky <[email protected]>
>> Subject: [PATCH] dump_stack: mark dummy dump_stack() as weak
>>
>> ---
>> arch/blackfin/kernel/dumpstack.c | 1 -
>> arch/nds32/kernel/traps.c | 2 --
>> lib/dump_stack.c | 4 ++--
>> 3 files changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/blackfin/kernel/dumpstack.c b/arch/blackfin/kernel/dumpstack.c
>> index 3c992c1f8ef2..61af017130cd 100644
>> --- a/arch/blackfin/kernel/dumpstack.c
>> +++ b/arch/blackfin/kernel/dumpstack.c
>> @@ -174,4 +174,3 @@ void dump_stack(void)
>> show_stack(current, &stack);
>> trace_buffer_restore(tflags);
>> }
>> -EXPORT_SYMBOL(dump_stack);
>> diff --git a/arch/nds32/kernel/traps.c b/arch/nds32/kernel/traps.c
>> index 8828b4aeb72b..455bb0787367 100644
>> --- a/arch/nds32/kernel/traps.c
>> +++ b/arch/nds32/kernel/traps.c
>> @@ -166,8 +166,6 @@ void dump_stack(void)
>> __dump(NULL, base_reg);
>> }
>>
>> -EXPORT_SYMBOL(dump_stack);
>> -
>> void show_stack(struct task_struct *tsk, unsigned long *sp)
>> {
>> unsigned long *base_reg;
>> diff --git a/lib/dump_stack.c b/lib/dump_stack.c
>> index 5cff72f18c4a..9cf4465dbffa 100644
>> --- a/lib/dump_stack.c
>> +++ b/lib/dump_stack.c
>> @@ -85,7 +85,7 @@ static void __dump_stack(void)
>> #ifdef CONFIG_SMP
>> static atomic_t dump_lock = ATOMIC_INIT(-1);
>>
>> -asmlinkage __visible void dump_stack(void)
>> +asmlinkage __weak __visible void dump_stack(void)
>> {
>> unsigned long flags;
>> int was_locked;
>> @@ -118,7 +118,7 @@ asmlinkage __visible void dump_stack(void)
>> local_irq_restore(flags);
>> }
>> #else
>> -asmlinkage __visible void dump_stack(void)
>> +asmlinkage __weak __visible void dump_stack(void)
>> {
>> __dump_stack();
>> }
>> --
>> 2.16.2
>>
>
> Thanks
> Dave
On (03/05/18 13:27), Greentime Hu wrote:
[..]
> >> Opinions? Will this work?
> >
> > I would think b) is better, thanks for the fix!
> >
> Hi,
>
> b works in nds32.
> Thanks for the fix :)
Greentime, Dave, thanks!
I'll send out a patch then.
Petr, once the patch has enough Ack/etc do you want to pick it up?
We need to merge it first, before Dave's patch.
-ss
On Sat 2018-03-03 23:47:39, Sergey Senozhatsky wrote:
> Cc-ing Tejun
>
> On (03/02/18 16:54), Petr Mladek wrote:
> [..]
> > > (Though it is not immediately obvious why.)
> >
> > It is a mistery to me. The error appears when I move any of
> > dump_stack_print_info() or show_regs_print_info() function
> > definitions from kernel/printk/printk.c to lib/dump_stack.c.
> > All the other changes seems unrelated.
> >
> > The thing is that we basically do not touch dump_stack() definition
> > by that patch.
>
> Apparently dump_stack_print_info() was in lib/dump_stack.c a long
> time ago, but it was deliberately moved to printk.c, when kernel gained
> a "generic" (dummy) dump_stack() fallback. Some archs, like blackfin,
> define their own dump_stack() symbol and make it global via EXPORT_SYMBOL.
>
> In case of blackfin that arch-specific dump_stack() symbol invokes a
> global dump_stack_print_info(). If we move dump_stack_print_info() back
> to lib/dump_stack.c then we link both with arch/blackfin/dumpstack.o
> and lib/dump_stack.o, which results in multiple definitions error.
> If we move dump_stack_print_info() out on libdump_stack.o, then we
> never link with lib/dump_stack.o
Ah, I have finally understood the meaning of the libs-y kbuild
variable. It is a nice source of these strange build failures.
> ... so what are we going to do with that.
>
> a) we can drop the patch and cherry pick only the kexec part
>
> b) we can try to mark dummy lib/dump_stack() as __weak
> EXPORT_SYMBOL and remove EXPORT_SYMBOL from arch-specific
> definitions.
Using the weak symbol makes perfect sense. I am going to
look at the patch.
Thanks a lot everybody for help.
Best Regards,
Petr