[PATCH] MIPS: fw: arc: add __weak to prom_meminit and prom_free_prom_memory

[PATCH] MIPS: fw: arc: add __weak to prom_meminit and prom_free_prom_memory

[Masahiro Yamada]

As far as I understood, prom_meminit() in arch/mips/fw/arc/memory.c
is overridden by the one in arch/mips/sgi-ip32/ip32-memory.c if
CONFIG_SGI_IP32 is enabled.

The use of EXPORT_SYMBOL in static libraries potentially causes a
problem for the llvm linker [1]. So, I want to forcibly link lib-y
objects to vmlinux when CONFIG_MODULES=y.

As a groundwork, we must fix multiple definitions that have previously
been hidden by lib-y.

The prom_cleanup() in this file is already marked as __weak (because
it is overridden by the one in arch/mips/sgi-ip22/ip22-mc.c).
I think it should be OK to do the same for these two.

[1]: https://github.com/ClangBuiltLinux/linux/issues/515

Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

If MIPS maintainers ack this patch,
I want to inser it before the following patch:

https://patchwork.kernel.org/patch/11432969/

 arch/mips/fw/arc/memory.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/fw/arc/memory.c b/arch/mips/fw/arc/memory.c
index dbbcddc82823..89fa6e62a3b3 100644
--- a/arch/mips/fw/arc/memory.c
+++ b/arch/mips/fw/arc/memory.c
@@ -117,7 +117,7 @@ static int __init prom_memtype_classify(union linux_memtypes type)
 	return memtype_classify_arc(type);
 }
 
-void __init prom_meminit(void)
+void __weak __init prom_meminit(void)
 {
 	struct linux_mdesc *p;
 
@@ -162,7 +162,7 @@ void __weak __init prom_cleanup(void)
 {
 }
 
-void __init prom_free_prom_memory(void)
+void __weak __init prom_free_prom_memory(void)
 {
 	int i;
 
-- 
2.17.1

Re: [PATCH] MIPS: fw: arc: add __weak to prom_meminit and prom_free_prom_memory

[Maciej W. Rozycki]

On Mon, 6 Apr 2020, Masahiro Yamada wrote:

> As far as I understood, prom_meminit() in arch/mips/fw/arc/memory.c
> is overridden by the one in arch/mips/sgi-ip32/ip32-memory.c if
> CONFIG_SGI_IP32 is enabled.
> 
> The use of EXPORT_SYMBOL in static libraries potentially causes a
> problem for the llvm linker [1]. So, I want to forcibly link lib-y
> objects to vmlinux when CONFIG_MODULES=y.

 It looks to me like a bug in the linker in the handling of the EXTERN 
command.  Why not fix the linker instead?

  Maciej

Re: [PATCH] MIPS: fw: arc: add __weak to prom_meminit and prom_free_prom_memory

[Masahiro Yamada]

On Mon, Apr 6, 2020 at 2:16 AM Maciej W. Rozycki <macro@linux-mips.org> wrote:
>
> On Mon, 6 Apr 2020, Masahiro Yamada wrote:
>
> > As far as I understood, prom_meminit() in arch/mips/fw/arc/memory.c
> > is overridden by the one in arch/mips/sgi-ip32/ip32-memory.c if
> > CONFIG_SGI_IP32 is enabled.
> >
> > The use of EXPORT_SYMBOL in static libraries potentially causes a
> > problem for the llvm linker [1]. So, I want to forcibly link lib-y
> > objects to vmlinux when CONFIG_MODULES=y.
>
>  It looks to me like a bug in the linker in the handling of the EXTERN
> command.  Why not fix the linker instead?
>
>   Maciej


I am not sure if this is a bug.
Anyway, they decided to not change ld.lld


MIPS code is so confusing.
There are multiple definitions,
and lib.a is (ab)used to hide them.

I fixed another one for MIPS before, and
0-day bot reported this recently.


There are lots of prom_meminit() definitions
in arch/mips/.

Making the intention clearer is a good thing, IMHO.


-- 
Best Regards
Masahiro Yamada

Re: [PATCH] MIPS: fw: arc: add __weak to prom_meminit and prom_free_prom_memory

[Maciej W. Rozycki]

On Mon, 6 Apr 2020, Masahiro Yamada wrote:

> > > As far as I understood, prom_meminit() in arch/mips/fw/arc/memory.c
> > > is overridden by the one in arch/mips/sgi-ip32/ip32-memory.c if
> > > CONFIG_SGI_IP32 is enabled.
> > >
> > > The use of EXPORT_SYMBOL in static libraries potentially causes a
> > > problem for the llvm linker [1]. So, I want to forcibly link lib-y
> > > objects to vmlinux when CONFIG_MODULES=y.
> >
> >  It looks to me like a bug in the linker in the handling of the EXTERN
> > command.  Why not fix the linker instead?
[...]
> I am not sure if this is a bug.
> Anyway, they decided to not change ld.lld

 Well, maybe that was a conscious decision, however it's a linker feature 
that has been there since forever and projects like Linux can legitimately 
rely on it.  In this case perhaps sticking to other linkers, which have 
the right features, is the right solution rather than trying to turn a 
complex and mature project like Linux upside down (and quite possibly 
introducing bugs and pessimisations on the way) just to match an inferior 
tool.  Adapt your tool to the task, not the task to your tool.

> MIPS code is so confusing.
> There are multiple definitions,
> and lib.a is (ab)used to hide them.

 It's a standard feature of libraries that a symbol reference is satisfied 
by the first symbol definition encountered.  Any extra ones provided later 
in the link order are ignored.  And we have control over the link order.

> I fixed another one for MIPS before, and
> 0-day bot reported this recently.
> 
> 
> There are lots of prom_meminit() definitions
> in arch/mips/.

 Naturally, many platforms will have its own, in addition to some generic 
(possibly dummy) one.

> Making the intention clearer is a good thing, IMHO.

 Hmm, what intention?  Can you please be more specific?

  Maciej

Re: [PATCH] MIPS: fw: arc: add __weak to prom_meminit and prom_free_prom_memory

[Nick Desaulniers]

On Mon, Apr 6, 2020 at 4:54 AM Maciej W. Rozycki <macro@linux-mips.org> wrote:
>
> On Mon, 6 Apr 2020, Masahiro Yamada wrote:
>
> > > > As far as I understood, prom_meminit() in arch/mips/fw/arc/memory.c
> > > > is overridden by the one in arch/mips/sgi-ip32/ip32-memory.c if
> > > > CONFIG_SGI_IP32 is enabled.
> > > >
> > > > The use of EXPORT_SYMBOL in static libraries potentially causes a
> > > > problem for the llvm linker [1]. So, I want to forcibly link lib-y
> > > > objects to vmlinux when CONFIG_MODULES=y.
> > >
> > >  It looks to me like a bug in the linker in the handling of the EXTERN
> > > command.  Why not fix the linker instead?
> [...]
> > I am not sure if this is a bug.
> > Anyway, they decided to not change ld.lld
>
>  Well, maybe that was a conscious decision, however it's a linker feature
> that has been there since forever and projects like Linux can legitimately
> rely on it.  In this case perhaps sticking to other linkers, which have
> the right features, is the right solution rather than trying to turn a
> complex and mature project like Linux upside down (and quite possibly
> introducing bugs and pessimisations on the way) just to match an inferior
> tool.  Adapt your tool to the task, not the task to your tool.

The feature you refer to and the feature Masahiro is referring to are
two separate issues.  If you care to understand the issue Masahiro is
trying to fix, please take the time to read the full discussion:
https://github.com/ClangBuiltLinux/linux/issues/515 and particularly
https://reviews.llvm.org/D63564

>
> > MIPS code is so confusing.
> > There are multiple definitions,
> > and lib.a is (ab)used to hide them.
>
>  It's a standard feature of libraries that a symbol reference is satisfied
> by the first symbol definition encountered.  Any extra ones provided later
> in the link order are ignored.  And we have control over the link order.

Relying on link order is terribly brittle. Renaming a file can cause
your implementation to change, and ties your hands from ever using
things like LTO or newer build systems like ninja.  Static
initialization order is a plague to C and C++.  It's explicitly
undefined behavior you've just admitted you prefer to rely on.  (A
Google search for "static initialization order" wants to autocomplete
to "static initialization order fiasco" which is also an interesting
read; https://www.cryptopp.com/wiki/Static_Initialization_Order_Fiasco).

Masahiro is right that this case has some questionable choices in
terms of redefining symbols with different implementations.  I think
__HAVE_ARCH_STRCPY and friends in lib/string.c is actually the best
pattern for not providing multiple definitions of a symbol, followed
by marking symbols meant to be overridden at link time based on config
as __weak.  He's just trying to help clean this up.

>
> > I fixed another one for MIPS before, and
> > 0-day bot reported this recently.
> >
> >
> > There are lots of prom_meminit() definitions
> > in arch/mips/.
>
>  Naturally, many platforms will have its own, in addition to some generic
> (possibly dummy) one.
>
> > Making the intention clearer is a good thing, IMHO.
>
>  Hmm, what intention?  Can you please be more specific?

That prom_meminit and prom_free_prom_memory are meant to be overridden
by other configs.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] MIPS: fw: arc: add __weak to prom_meminit and prom_free_prom_memory

[Maciej W. Rozycki]

On Mon, 6 Apr 2020, Nick Desaulniers wrote:

> >  Well, maybe that was a conscious decision, however it's a linker feature
> > that has been there since forever and projects like Linux can legitimately
> > rely on it.  In this case perhaps sticking to other linkers, which have
> > the right features, is the right solution rather than trying to turn a
> > complex and mature project like Linux upside down (and quite possibly
> > introducing bugs and pessimisations on the way) just to match an inferior
> > tool.  Adapt your tool to the task, not the task to your tool.
> 
> The feature you refer to and the feature Masahiro is referring to are
> two separate issues.  If you care to understand the issue Masahiro is
> trying to fix, please take the time to read the full discussion:
> https://github.com/ClangBuiltLinux/linux/issues/515 and particularly
> https://reviews.llvm.org/D63564

 The onus is on the patch submitter to justify a change I'm afraid, and 
not the reviewer to chase such justification.  A change is supposed to be 
self-contained and straightforward to understand for someone reasonably 
familiar with the matters covered though not necessarily with the specific 
issue.  And this change is missing such self-contained justification I'm 
afraid.

 You need to make it easy for the reviewer to accept a change.  If a 
reviewer looks at the proposed change itself, reads through the associated 
description and immediately knows what is going on and that the change is 
good, the this is a good submission.

> >  It's a standard feature of libraries that a symbol reference is satisfied
> > by the first symbol definition encountered.  Any extra ones provided later
> > in the link order are ignored.  And we have control over the link order.
> 
> Relying on link order is terribly brittle. Renaming a file can cause
> your implementation to change, and ties your hands from ever using
> things like LTO or newer build systems like ninja.  Static
> initialization order is a plague to C and C++.  It's explicitly
> undefined behavior you've just admitted you prefer to rely on.  (A
> Google search for "static initialization order" wants to autocomplete
> to "static initialization order fiasco" which is also an interesting
> read; https://www.cryptopp.com/wiki/Static_Initialization_Order_Fiasco).

 Renaming a file does not cause the link order to automatically change 
(although I can imagine a build system where that happens).  And we're not 
talking about static initialisers and their implicit dependencies that 
might be hard to follow.

 We just have pairs of symbols that need to appear in a specific order in 
the link, and this is quite straightforward to control, by placing 
platform files earlier on the linker's invocation line, followed by arch 
files, followed by generic files.  That's certainly been defined linker's 
behaviour, especially as tools had to rely on it in more complex build 
systems before weak symbols were invented.

> Masahiro is right that this case has some questionable choices in
> terms of redefining symbols with different implementations.  I think
> __HAVE_ARCH_STRCPY and friends in lib/string.c is actually the best
> pattern for not providing multiple definitions of a symbol, followed
> by marking symbols meant to be overridden at link time based on config
> as __weak.  He's just trying to help clean this up.

 That is perhaps a worthwhile improvement, but does not appear to have
anything to do with LLVM and its bug in handling the EXTERN linker script 
command.  Let's not mix up things.

 Also IIUC this will result in a code bloat: previously objects providing 
fallback symbols were not pulled from archives if not needed, whereas now 
they will always be and will remain permanently unused, wasting memory at 
run time.  So this looks to me like a (gratuitous) pessimisation.

> >  Hmm, what intention?  Can you please be more specific?
> 
> That prom_meminit and prom_free_prom_memory are meant to be overridden
> by other configs.

 Then a comment at the fallback implementation should be enough.  Besides 
you need to be somewhat familiar with code you want to work with and Linux 
has always been terse in code annotations and relied on code being 
self-explanatory instead.

  Maciej

Re: [PATCH] MIPS: fw: arc: add __weak to prom_meminit and prom_free_prom_memory

[Thomas Bogendoerfer]

On Mon, Apr 06, 2020 at 01:30:52AM +0900, Masahiro Yamada wrote:
> As far as I understood, prom_meminit() in arch/mips/fw/arc/memory.c
> is overridden by the one in arch/mips/sgi-ip32/ip32-memory.c if
> CONFIG_SGI_IP32 is enabled.
> 
> The use of EXPORT_SYMBOL in static libraries potentially causes a
> problem for the llvm linker [1]. So, I want to forcibly link lib-y
> objects to vmlinux when CONFIG_MODULES=y.
> 
> As a groundwork, we must fix multiple definitions that have previously
> been hidden by lib-y.
> 
> The prom_cleanup() in this file is already marked as __weak (because
> it is overridden by the one in arch/mips/sgi-ip22/ip22-mc.c).
> I think it should be OK to do the same for these two.
> 
> [1]: https://github.com/ClangBuiltLinux/linux/issues/515
> 
> Reported-by: kbuild test robot <lkp@intel.com>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
> If MIPS maintainers ack this patch,
> I want to inser it before the following patch:


Acked-By: Thomas Bogendoerfer <tsbogend@alpha.franken.de>

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: fw: arc: add __weak to prom_meminit and prom_free_prom_memory

[Masahiro Yamada]

On Mon, Apr 6, 2020 at 8:36 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> On Mon, Apr 06, 2020 at 01:30:52AM +0900, Masahiro Yamada wrote:
> > As far as I understood, prom_meminit() in arch/mips/fw/arc/memory.c
> > is overridden by the one in arch/mips/sgi-ip32/ip32-memory.c if
> > CONFIG_SGI_IP32 is enabled.
> >
> > The use of EXPORT_SYMBOL in static libraries potentially causes a
> > problem for the llvm linker [1]. So, I want to forcibly link lib-y
> > objects to vmlinux when CONFIG_MODULES=y.
> >
> > As a groundwork, we must fix multiple definitions that have previously
> > been hidden by lib-y.
> >
> > The prom_cleanup() in this file is already marked as __weak (because
> > it is overridden by the one in arch/mips/sgi-ip22/ip22-mc.c).
> > I think it should be OK to do the same for these two.
> >
> > [1]: https://github.com/ClangBuiltLinux/linux/issues/515
> >
> > Reported-by: kbuild test robot <lkp@intel.com>
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> > If MIPS maintainers ack this patch,
> > I want to inser it before the following patch:
>
>
> Acked-By: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
>
> Thomas.


Applied to linux-kbuild with Thomas' Ack.

Thanks.


>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200406110333.GA6266%40alpha.franken.de.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] MIPS: fw: arc: add __weak to prom_meminit and prom_free_prom_memory

[Nick Desaulniers]

On Sun, Apr 5, 2020 at 9:31 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> As far as I understood, prom_meminit() in arch/mips/fw/arc/memory.c
> is overridden by the one in arch/mips/sgi-ip32/ip32-memory.c if
> CONFIG_SGI_IP32 is enabled.

I'm curious if this issue is affected by other combinations of files
that each define prom_meminit with external linkage and symbol
visibility.  I would expect __weak to be used when there's a base
implementation that we expect to be overridden, but I don't think
that's what's happening in this case.  It's not clear to me which
definition would be the default, let alone the one in
arch/mips/fw/arc/memory.c.

Looks like CONFIG_SGI_IP32 selects CONFIG_FW_ARC, so it's not clear
why arch/mips/sgi-ip32/ip32-memory.c and arch/mips/fw/arc/memory.c
define different implementations of prom_meminit, and which one was
expected to be used.

Hopefully the maintainers can clarify.

>
> The use of EXPORT_SYMBOL in static libraries potentially causes a
> problem for the llvm linker [1]. So, I want to forcibly link lib-y
> objects to vmlinux when CONFIG_MODULES=y.
>
> As a groundwork, we must fix multiple definitions that have previously
> been hidden by lib-y.
>
> The prom_cleanup() in this file is already marked as __weak (because
> it is overridden by the one in arch/mips/sgi-ip22/ip22-mc.c).
> I think it should be OK to do the same for these two.
>
> [1]: https://github.com/ClangBuiltLinux/linux/issues/515
>
> Reported-by: kbuild test robot <lkp@intel.com>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
> If MIPS maintainers ack this patch,
> I want to inser it before the following patch:
>
> https://patchwork.kernel.org/patch/11432969/
>
>  arch/mips/fw/arc/memory.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/fw/arc/memory.c b/arch/mips/fw/arc/memory.c
> index dbbcddc82823..89fa6e62a3b3 100644
> --- a/arch/mips/fw/arc/memory.c
> +++ b/arch/mips/fw/arc/memory.c
> @@ -117,7 +117,7 @@ static int __init prom_memtype_classify(union linux_memtypes type)
>         return memtype_classify_arc(type);
>  }
>
> -void __init prom_meminit(void)
> +void __weak __init prom_meminit(void)
>  {
>         struct linux_mdesc *p;
>
> @@ -162,7 +162,7 @@ void __weak __init prom_cleanup(void)
>  {
>  }
>
> -void __init prom_free_prom_memory(void)
> +void __weak __init prom_free_prom_memory(void)
>  {
>         int i;
>
> --

-- 
Thanks,
~Nick Desaulniers