Message-ID: <CAKwvOdn4n1fqMz+7wcEBv-kxLf7tWHgC2P+m73mGQ7m6BoOxwQ@mail.gmail.com>
Date:   Thu, 22 Jun 2023 09:20:11 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Masahiro Yamada <masahiroy@...nel.org>
Cc:     Guenter Roeck <linux@...ck-us.net>, linux-kbuild@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Nathan Chancellor <nathan@...nel.org>,
        Nicolas Schier <nicolas@...sle.eu>,
        linux-um@...ts.infradead.org, Fangrui Song <maskray@...gle.com>
Subject: Re: [PATCH v7 03/11] kbuild: generate KSYMTAB entries by modpost

On Wed, Jun 21, 2023 at 7:27 PM Masahiro Yamada <masahiroy@...nel.org> wrote:
>
> On Thu, Jun 22, 2023 at 1:15 AM Guenter Roeck <linux@...ck-us.net> wrote:
> >
> > On Thu, Jun 08, 2023 at 11:24:20PM +0900, Masahiro Yamada wrote:
> > > Commit 7b4537199a4a ("kbuild: link symbol CRCs at final link, removing
> > > CONFIG_MODULE_REL_CRCS") made modpost output CRCs in the same way
> > > whether the EXPORT_SYMBOL() is placed in *.c or *.S.
> > >
> > ...
> >
> > > We can do this better now; modpost can selectively emit KSYMTAB entries
> > > that are really used by modules.
> > >
> >
> > This patch results in
> >
> > Building alpha:defconfig ... failed
> > --------------
> > Error log:
> > <stdin>:1519:2: warning: #warning syscall clone3 not implemented [-Wcpp]
> > WARNING: modpost: "saved_config" [vmlinux] is COMMON symbol
> I do not know much about the warning for "saved_config".
>
>
>
> __attribute((common)) was added in the following commit:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/?id=3c7940961fbf9f252e20f9c455f2fe63f273294c
>
>
> It was more than 20 years ago, and there is
> no commit description.
> I do not know the intention of __attribute((common)).

Adding __attribute__((common)) to a variable declaration, the
generated assembler will contain a .comm assembler directive.
https://godbolt.org/z/6jvoaGs8M
https://sourceware.org/binutils/docs/as/Comm.html

>>> .comm declares a common symbol named symbol. When linking, a common symbol in one object file may be merged with a defined or common symbol of the same name in another object file.

When I see this, I think some kind of ODR tricks are afoot.  C doesn't
have ODR; C++ does, but this attribute allows for ODR-like linker
merging where the linker discards duplicates because "they ought to be
the same."

IIRC, I think C++ template instantiations are emitted with common
linkage.  LLVM IR has a linkage type corresponding to this that I
initially didn't understand, because without this attribute, I don't
think that linkage type is ever emitted for C code.

Indeed, for alpha, we can see the same symbol marked common in arch/alpha/:
arch/alpha/kernel/core_tsunami.c:36:} saved_config[2] __attribute__((common));
arch/alpha/kernel/sys_sio.c:43:} saved_config __attribute((common));
arch/alpha/kernel/core_cia.c:577:} saved_config __attribute((common));
arch/alpha/kernel/core_titan.c:36:} saved_config[4] __attribute__((common));

All of these have different sizes, so according to the GAS manual,
this line comes into play:

>>> If ld sees multiple common symbols with the same name, and they do not all have the same size, it will allocate space using the largest size.

This kind of smells like some overcomplicating code to save like 30B
of static memory.  Fixing this requires careful use of enums.  For
arch/alpha/ not sure such a rework is worth it, given the imminent
demise of the architecture I sense looming on the horizon.


>
> I hope the maintainers will fix the warnings,
> but I do not know if it is likely to happen.
>
> MAINTAINERS says "Odd Fixes"
>
> If you find a build regression, please let me know.
> So far, I did not get new reports from 0day bot.
>
>
> Thanks.
>
>
>
>
> > Guenter
> >
> > ---
> > Bisect log:
> >
> > # bad: [15e71592dbae49a674429c618a10401d7f992ac3] Add linux-next specific files for 20230621
> > # good: [45a3e24f65e90a047bef86f927ebdc4c710edaa1] Linux 6.4-rc7
> > git bisect start 'HEAD' 'v6.4-rc7'
> > # bad: [e867e67cd55ae460c860ffd896c7fc96add2821c] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
> > git bisect bad e867e67cd55ae460c860ffd896c7fc96add2821c
> > # bad: [57b289d5b1005a9c39d6d6567e0ef6115bd59cea] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/riscv/linux.git
> > git bisect bad 57b289d5b1005a9c39d6d6567e0ef6115bd59cea
> > # bad: [dc6399fc9ae6d2530fc38fb3ae96bcc8393bd66f] Merge branch 'for-next/perf' of git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git
> > git bisect bad dc6399fc9ae6d2530fc38fb3ae96bcc8393bd66f
> > # good: [6d366ba598334a0457d917a7bf38efd118c5b7be] Merge branch 'mm-stable' of git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
> > git bisect good 6d366ba598334a0457d917a7bf38efd118c5b7be
> > # good: [82fe2e45cdb00de4fa648050ae33bdadf9b3294a] perf pmus: Check if we can encode the PMU number in perf_event_attr.type
> > git bisect good 82fe2e45cdb00de4fa648050ae33bdadf9b3294a
> > # bad: [d2fa756910f88c2f5871775483744407cbf67933] Merge branch 'for-next' of git://git.infradead.org/users/hch/dma-mapping.git
> > git bisect bad d2fa756910f88c2f5871775483744407cbf67933
> > # good: [1b990bc8edc396a37a3ff1a43f7c329c361ee07c] Merge branch 'mm-nonmm-unstable' into mm-everything
> > git bisect good 1b990bc8edc396a37a3ff1a43f7c329c361ee07c
> > # good: [cff6e7f50bd315e5b39c4e46c704ac587ceb965f] kbuild: Add CLANG_FLAGS to as-instr
> > git bisect good cff6e7f50bd315e5b39c4e46c704ac587ceb965f
> > # bad: [8f3847e175a0044e2212fef772e7fa912270cd6d] ia64,export.h: replace EXPORT_DATA_SYMBOL* with EXPORT_SYMBOL*
> > git bisect bad 8f3847e175a0044e2212fef772e7fa912270cd6d
> > # good: [3a3f1e573a105328a2cca45a7cfbebabbf5e3192] modpost: fix off by one in is_executable_section()
> > git bisect good 3a3f1e573a105328a2cca45a7cfbebabbf5e3192
> > # good: [92e74fb6e6196d642505ae2b74a8e327202afef9] scripts/kallsyms: constify long_options
> > git bisect good 92e74fb6e6196d642505ae2b74a8e327202afef9
> > # good: [92e2921eeafdfca9acd9b83f07d2b7ca099bac24] ARC: define ASM_NL and __ALIGN(_STR) outside #ifdef __ASSEMBLY__ guard
> > git bisect good 92e2921eeafdfca9acd9b83f07d2b7ca099bac24
> > # bad: [bb2aa9a94b41b883037a56709d995c269204ade0] kbuild: generate KSYMTAB entries by modpost
> > git bisect bad bb2aa9a94b41b883037a56709d995c269204ade0
> > # good: [94d6cb68124b7a63f24fcc345795ba5f9a27e694] modpost: pass struct module pointer to check_section_mismatch()
> > git bisect good 94d6cb68124b7a63f24fcc345795ba5f9a27e694
> > # first bad commit: [bb2aa9a94b41b883037a56709d995c269204ade0] kbuild: generate KSYMTAB entries by modpost
>
>
>
> --
> Best Regards
> Masahiro Yamada



-- 
Thanks,
~Nick Desaulniers

Powered by blists - more mailing lists