Re: [PATCH v2 2/2] kbuild: link lib-y objects to vmlinux forcibly when CONFIG_MODULES=y

Re: [PATCH v2 2/2] kbuild: link lib-y objects to vmlinux forcibly when CONFIG_MODULES=y

[Thomas Bogendoerfer]

On Thu, Mar 12, 2020 at 03:12:28PM +0900, Masahiro Yamada wrote:
> I got the following report from 0-day bot.
> Please advise me how to fix it.
> 
> 
> I am not sure how multi-platform works in MIPS.
> 
> The cavium-octeon platform has its own implementation
> of various functions.
> 
> So, vmlinux links different library routines
> depending on whether CONFIG_CAVIUM_OCTEON_SOC, correct?

for cavium memcpy is directly linked in via octeon-memcpy.o, while for
every other platform it's coming from lib/lib.a(memcpy.o).

What have you changed, that this doesn't work anymore ?

Thomas.

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

Re: [PATCH v2 2/2] kbuild: link lib-y objects to vmlinux forcibly when CONFIG_MODULES=y

[Nick Desaulniers]

+ linux-mips this time

On Mon, Mar 16, 2020 at 4:13 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Wed, Mar 11, 2020 at 11:13 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > Hi MIPS forks,
> >
> >
> > I got the following report from 0-day bot.
> > Please advise me how to fix it.
> >
> >
> > I am not sure how multi-platform works in MIPS.
> >
> > The cavium-octeon platform has its own implementation
> > of various functions.
> >
> > So, vmlinux links different library routines
> > depending on whether CONFIG_CAVIUM_OCTEON_SOC, correct?
> >
> >
> >
> > fw_init_cmdline():
> > arch/mips/cavium-octeon/setup.c
> > arch/mips/fw/lib/cmdline.c
> >
> >
> > __delay(), __udelay(), __ndelay():
> > arch/mips/cavium-octeon/csrc-octeon.c
> > arch/mips/lib/delay.S
> >
> >
> > memcpy(), memmove():
> > arch/mips/cavium-octeon/octeon-memcpy.S
> > arch/mips/lib/memcpy.S
> >
> >
> >
> > FWIW, the following fixes the multiple definition errors.
> >
> >
> >
> > diff --git a/arch/mips/fw/lib/cmdline.c b/arch/mips/fw/lib/cmdline.c
> > index 6ecda64ad184..6ac6e0493e1f 100644
> > --- a/arch/mips/fw/lib/cmdline.c
> > +++ b/arch/mips/fw/lib/cmdline.c
> > @@ -16,6 +16,7 @@ int fw_argc;
> >  int *_fw_argv;
> >  int *_fw_envp;
> >
> > +#ifndef CONFIG_CAVIUM_OCTEON_SOC
> >  void __init fw_init_cmdline(void)
>
> Alternatively, you could define this fw_init_cmdline as __weak, then
> let the strong definition in arch/mips/cavium-octeon/setup.c override
> it. But both should work, so:
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Eitherway, octeon has some multiple definition errors that should get fixed.
>
> >  {
> >         int i;
> > @@ -41,6 +42,7 @@ void __init fw_init_cmdline(void)
> >                         strlcat(arcs_cmdline, " ", COMMAND_LINE_SIZE);
> >         }
> >  }
> > +#endif
> >
> >  char * __init fw_getcmdline(void)
> >  {
> > diff --git a/arch/mips/lib/Makefile b/arch/mips/lib/Makefile
> > index 479f50559c83..4cc98af4161a 100644
> > --- a/arch/mips/lib/Makefile
> > +++ b/arch/mips/lib/Makefile
> > @@ -3,10 +3,14 @@
> >  # Makefile for MIPS-specific library files..
> >  #
> >
> > -lib-y  += bitops.o csum_partial.o delay.o memcpy.o memset.o \
> > +lib-y  += bitops.o csum_partial.o memset.o \
> >            mips-atomic.o strncpy_user.o \
> >            strnlen_user.o uncached.o
> >
> > +ifneq ($(CONFIG_CAVIUM_OCTEON_SOC),y)
> > +lib-y  += delay.o memcpy.o
> > +endif
> > +
> >  obj-y                  += iomap_copy.o
> >  obj-$(CONFIG_PCI)      += iomap-pci.o
> >  lib-$(CONFIG_GENERIC_CSUM)     := $(filter-out csum_partial.o, $(lib-y))
> >
> >
> >
> > On Thu, Mar 12, 2020 at 1:31 PM kbuild test robot <lkp@intel.com> wrote:
> > >
> > > Hi Masahiro,
> > >
> > > I love your patch! Yet something to improve:
> > >
> > > [auto build test ERROR on kbuild/for-next]
> > > [also build test ERROR on v5.6-rc5 next-20200311]
> > > [if your patch is applied to the wrong git tree, please drop us a note to help
> > > improve the system. BTW, we also suggest to use '--base' option to specify the
> > > base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> > >
> > > url:    https://github.com/0day-ci/linux/commits/Masahiro-Yamada/sparc-revive-__HAVE_ARCH_STRLEN-for-32bit-sparc/20200312-073459
> > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git for-next
> > > config: mips-cavium_octeon_defconfig (attached as .config)
> > > compiler: mips64-linux-gcc (GCC) 9.2.0
> > > reproduce:
> > >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > >         chmod +x ~/bin/make.cross
> > >         # save the attached .config to linux build tree
> > >         GCC_VERSION=9.2.0 make.cross ARCH=mips
> > >
> > > If you fix the issue, kindly add following tag
> > > Reported-by: kbuild test robot <lkp@intel.com>
> > >
> > > All errors (new ones prefixed by >>):
> > >
> > >    mips64-linux-ld: arch/mips/fw/lib/cmdline.o: in function `fw_init_cmdline':
> > > >> cmdline.c:(.init.text+0x0): multiple definition of `fw_init_cmdline'; arch/mips/cavium-octeon/setup.o:setup.c:(.init.text+0xad8): first defined here
> > >    mips64-linux-ld: arch/mips/lib/delay.o: in function `__delay':
> > > >> delay.c:(.text+0x0): multiple definition of `__delay'; arch/mips/cavium-octeon/csrc-octeon.o:csrc-octeon.c:(.text+0x80): first defined here
> > >    mips64-linux-ld: arch/mips/lib/delay.o: in function `__udelay':
> > > >> delay.c:(.text+0x10): multiple definition of `__udelay'; arch/mips/cavium-octeon/csrc-octeon.o:csrc-octeon.c:(.text+0x8): first defined here
> > >    mips64-linux-ld: arch/mips/lib/delay.o: in function `__ndelay':
> > > >> delay.c:(.text+0x50): multiple definition of `__ndelay'; arch/mips/cavium-octeon/csrc-octeon.o:csrc-octeon.c:(.text+0x40): first defined here
> > >    mips64-linux-ld: arch/mips/lib/memcpy.o: in function `memmove':
> > > >> (.text+0x0): multiple definition of `memmove'; arch/mips/cavium-octeon/octeon-memcpy.o:(.text+0x3a0): first defined here
> > >    mips64-linux-ld: arch/mips/lib/memcpy.o: in function `__rmemcpy':
> > > >> (.text+0x20): multiple definition of `__rmemcpy'; arch/mips/cavium-octeon/octeon-memcpy.o:(.text+0x3c0): first defined here
> > >    mips64-linux-ld: arch/mips/lib/memcpy.o: in function `memcpy':
> > > >> (.text+0x80): multiple definition of `memcpy'; arch/mips/cavium-octeon/octeon-memcpy.o:(.text+0x0): first defined here
> > >    mips64-linux-ld: arch/mips/lib/memcpy.o: in function `memcpy':
> > > >> (.text+0x84): multiple definition of `__copy_user'; arch/mips/cavium-octeon/octeon-memcpy.o:(.text+0x4): first defined here
> > >
> > > ---
> > > 0-DAY CI Kernel Test Service, Intel Corporation
> > > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> >
> >
> >
> > --
> > Best Regards
> > Masahiro Yamada
>
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 2/2] kbuild: link lib-y objects to vmlinux forcibly when CONFIG_MODULES=y

[Masahiro Yamada]

Hi Thomas,

On Thu, Mar 12, 2020 at 5:40 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> On Thu, Mar 12, 2020 at 03:12:28PM +0900, Masahiro Yamada wrote:
> > I got the following report from 0-day bot.
> > Please advise me how to fix it.
> >
> >
> > I am not sure how multi-platform works in MIPS.
> >
> > The cavium-octeon platform has its own implementation
> > of various functions.
> >
> > So, vmlinux links different library routines
> > depending on whether CONFIG_CAVIUM_OCTEON_SOC, correct?
>
> for cavium memcpy is directly linked in via octeon-memcpy.o, while for
> every other platform it's coming from lib/lib.a(memcpy.o).
>
> What have you changed, that this doesn't work anymore ?
>
> Thomas.


I want to change all objects from lib-y
to be linked to vmlinux (exactly like obj-y )
if CONFIG_MODULES is enabled.

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


EXPORT_SYMBOL in libraries
must be linked to vmlinux all the time,
even if there is no call-site in vmlinux.
I believe this is a good simplification because
EXPORT_SYMBOL is interface to loadable modules.

As it turned out, lib-y is (ab)used to avoid
multiple definition errors.

The 0-day detected a bug of 32-bit sparc:
https://patchwork.kernel.org/patch/11432969/

And, another is this one.

MIPS relies on that
arch/mips/lib/lib.a is weaker than octeon ones.

So, annotating __weak is a good solution
(thanks Nick!).

If I send a patch, is it acceptable?


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2 2/2] kbuild: link lib-y objects to vmlinux forcibly when CONFIG_MODULES=y

[Thomas Bogendoerfer]

On Fri, Mar 20, 2020 at 12:48:20AM +0900, Masahiro Yamada wrote:
> Hi Thomas,
> 
> On Thu, Mar 12, 2020 at 5:40 PM Thomas Bogendoerfer
> <tsbogend@alpha.franken.de> wrote:
> >
> > On Thu, Mar 12, 2020 at 03:12:28PM +0900, Masahiro Yamada wrote:
> > > I got the following report from 0-day bot.
> > > Please advise me how to fix it.
> > >
> > >
> > > I am not sure how multi-platform works in MIPS.
> > >
> > > The cavium-octeon platform has its own implementation
> > > of various functions.
> > >
> > > So, vmlinux links different library routines
> > > depending on whether CONFIG_CAVIUM_OCTEON_SOC, correct?
> >
> > for cavium memcpy is directly linked in via octeon-memcpy.o, while for
> > every other platform it's coming from lib/lib.a(memcpy.o).
> >
> > What have you changed, that this doesn't work anymore ?
> >
> > Thomas.
> 
> 
> I want to change all objects from lib-y
> to be linked to vmlinux (exactly like obj-y )
> if CONFIG_MODULES is enabled.

ic

> So, annotating __weak is a good solution
> (thanks Nick!).
> 
> If I send a patch, is it acceptable?

sure.

Thomas.

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