All of lore.kernel.org
 help / color / mirror / Atom feed
From: Nick Desaulniers <ndesaulniers@google.com>
To: Jeff Johnson <quic_jjohnson@quicinc.com>
Cc: Masahiro Yamada <masahiroy@kernel.org>,
	 Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
	 Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Nathan Chancellor <nathan@kernel.org>,
	 Nicolas Schier <nicolas@fjasle.eu>,
	Peter Zijlstra <peterz@infradead.org>,
	 linux-modules <linux-modules@vger.kernel.org>,
	linux-s390 <linux-s390@vger.kernel.org>,
	 linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
	Ard Biesheuvel <ardb@kernel.org>,
	 Sami Tolvanen <samitolvanen@google.com>,
	clang-built-linux <llvm@lists.linux.dev>
Subject: Re: [PATCH v4 03/14] modpost: split the section mismatch checks into section-check.c
Date: Wed, 11 May 2022 13:01:13 -0700	[thread overview]
Message-ID: <CAKwvOdk1z6jnTA79W8qyy1sWuoa29YxppU1gAkHkQ=g6UiVvCw@mail.gmail.com> (raw)
In-Reply-To: <8babc23b-5e24-f662-6c4a-eb1c30e0e6da@quicinc.com>

On Wed, May 11, 2022 at 12:51 PM Jeff Johnson <quic_jjohnson@quicinc.com> wrote:
>
> On 5/11/2022 12:27 PM, Masahiro Yamada wrote:
> > On Thu, May 12, 2022 at 3:48 AM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> >>
> >> On Mon, May 9, 2022 at 11:57 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >>>
> >>>>> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> >>>>> index a78b75f0eeb0..e7e2c70a98f5 100644
> >>>>> --- a/scripts/mod/modpost.c
> >>>>> +++ b/scripts/mod/modpost.c
> >>>>> @@ -31,7 +31,7 @@ static bool external_module;
> >>>>>   /* Only warn about unresolved symbols */
> >>>>>   static bool warn_unresolved;
> >>>>>
> >>>>> -static int sec_mismatch_count;
> >>>>> +int sec_mismatch_count;
> >>>>
> >>>> ^ this should go in modpost.h if it is to be used in two translation
> >>>> units, rather than forward declaring it in section-check.c.  You did
> >>>> this for the functions.
> >>>
> >>>
> >>> Sorry, I do not understand.
> >>>
> >>>
> >>> In modpost.h, I put the declaration:
> >>>
> >>>    extern int sec_mismatch_count;
> >>>
> >>> If I moved it to the header without 'extern'
> >>> I would get multiple definitions.
> >>
> >> Yeah, you need to _declare_ it w/ extern in the header, then _define_
> >> it in one source file.
> >>
> >> That way, if the type ever changes, the sources will agree on type in
> >> all source files. You will get a redefinition error if the definition
> >> changes the type of the variable since the last declaration.
> >>
> >> What you're doing is forward declaring, which works, and is a common
> >> pattern for (bloated) C++, but is less type safe than sharing a single
> >> common declaration between multiple source files via a single common
> >> shared header. (Sorry I didn't respond before you sent v5)
> >
> > Sorry, I still do not understand your suggestion.
> >
> >
> > Could you provide me with a code diff
> > showing how to do this better?
>
> I think you are doing exactly what he's asking for:
> declare it with extern in the header (modpost.h change)
> define it in one source file (modpost.c change)
>

Oh! Indeed, sorry, I'm getting lost here in the review. Sorry for the
noise then.
-- 
Thanks,
~Nick Desaulniers

WARNING: multiple messages have this Message-ID
From: Nick Desaulniers <ndesaulniers@google.com>
To: Jeff Johnson <quic_jjohnson@quicinc.com>
Cc: linux-s390 <linux-s390@vger.kernel.org>,
	Nicolas Schier <nicolas@fjasle.eu>,
	Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Masahiro Yamada <masahiroy@kernel.org>,
	clang-built-linux <llvm@lists.linux.dev>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Nathan Chancellor <nathan@kernel.org>,
	Sami Tolvanen <samitolvanen@google.com>,
	linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
	Ard Biesheuvel <ardb@kernel.org>,
	linux-modules <linux-modules@vger.kernel.org>
Subject: Re: [PATCH v4 03/14] modpost: split the section mismatch checks into section-check.c
Date: Wed, 11 May 2022 13:01:13 -0700	[thread overview]
Message-ID: <CAKwvOdk1z6jnTA79W8qyy1sWuoa29YxppU1gAkHkQ=g6UiVvCw@mail.gmail.com> (raw)
In-Reply-To: <8babc23b-5e24-f662-6c4a-eb1c30e0e6da@quicinc.com>

On Wed, May 11, 2022 at 12:51 PM Jeff Johnson <quic_jjohnson@quicinc.com> wrote:
>
> On 5/11/2022 12:27 PM, Masahiro Yamada wrote:
> > On Thu, May 12, 2022 at 3:48 AM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> >>
> >> On Mon, May 9, 2022 at 11:57 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >>>
> >>>>> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> >>>>> index a78b75f0eeb0..e7e2c70a98f5 100644
> >>>>> --- a/scripts/mod/modpost.c
> >>>>> +++ b/scripts/mod/modpost.c
> >>>>> @@ -31,7 +31,7 @@ static bool external_module;
> >>>>>   /* Only warn about unresolved symbols */
> >>>>>   static bool warn_unresolved;
> >>>>>
> >>>>> -static int sec_mismatch_count;
> >>>>> +int sec_mismatch_count;
> >>>>
> >>>> ^ this should go in modpost.h if it is to be used in two translation
> >>>> units, rather than forward declaring it in section-check.c.  You did
> >>>> this for the functions.
> >>>
> >>>
> >>> Sorry, I do not understand.
> >>>
> >>>
> >>> In modpost.h, I put the declaration:
> >>>
> >>>    extern int sec_mismatch_count;
> >>>
> >>> If I moved it to the header without 'extern'
> >>> I would get multiple definitions.
> >>
> >> Yeah, you need to _declare_ it w/ extern in the header, then _define_
> >> it in one source file.
> >>
> >> That way, if the type ever changes, the sources will agree on type in
> >> all source files. You will get a redefinition error if the definition
> >> changes the type of the variable since the last declaration.
> >>
> >> What you're doing is forward declaring, which works, and is a common
> >> pattern for (bloated) C++, but is less type safe than sharing a single
> >> common declaration between multiple source files via a single common
> >> shared header. (Sorry I didn't respond before you sent v5)
> >
> > Sorry, I still do not understand your suggestion.
> >
> >
> > Could you provide me with a code diff
> > showing how to do this better?
>
> I think you are doing exactly what he's asking for:
> declare it with extern in the header (modpost.h change)
> define it in one source file (modpost.c change)
>

Oh! Indeed, sorry, I'm getting lost here in the review. Sorry for the
noise then.
-- 
Thanks,
~Nick Desaulniers

  reply	other threads:[~2022-05-11 20:01 UTC|newest]

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-08 19:06 [PATCH v4 00/14] kbuild: yet another series of cleanups (modpost, LTO, MODULE_REL_CRCS, export.h) Masahiro Yamada
2022-05-08 19:06 ` Masahiro Yamada
2022-05-08 19:06 ` [PATCH v4 01/14] modpost: remove left-over cross_compile declaration Masahiro Yamada
2022-05-08 19:06   ` Masahiro Yamada
2022-05-09 17:08   ` Nick Desaulniers
2022-05-09 17:08     ` Nick Desaulniers
2022-05-12  4:35   ` Masahiro Yamada
2022-05-12  4:35     ` Masahiro Yamada
2022-05-08 19:06 ` [PATCH v4 02/14] modpost: change the license of EXPORT_SYMBOL to bool type Masahiro Yamada
2022-05-08 19:06   ` Masahiro Yamada
2022-05-12  4:36   ` Masahiro Yamada
2022-05-12  4:36     ` Masahiro Yamada
2022-05-08 19:06 ` [PATCH v4 03/14] modpost: split the section mismatch checks into section-check.c Masahiro Yamada
2022-05-08 19:06   ` Masahiro Yamada
2022-05-09 17:19   ` Nick Desaulniers
2022-05-09 17:19     ` Nick Desaulniers
2022-05-10  6:55     ` Masahiro Yamada
2022-05-10  6:55       ` Masahiro Yamada
2022-05-11 18:47       ` Nick Desaulniers
2022-05-11 18:47         ` Nick Desaulniers
2022-05-11 19:27         ` Masahiro Yamada
2022-05-11 19:27           ` Masahiro Yamada
2022-05-11 19:51           ` Jeff Johnson
2022-05-11 19:51             ` Jeff Johnson
2022-05-11 20:01             ` Nick Desaulniers [this message]
2022-05-11 20:01               ` Nick Desaulniers
2022-05-08 19:06 ` [PATCH v4 04/14] modpost: add sym_find_with_module() helper Masahiro Yamada
2022-05-08 19:06   ` Masahiro Yamada
2022-05-08 19:06 ` [PATCH v4 05/14] modpost: extract symbol versions from *.cmd files Masahiro Yamada
2022-05-08 19:06   ` Masahiro Yamada
2022-05-09 21:52   ` Sami Tolvanen
2022-05-09 21:52     ` Sami Tolvanen
2022-05-08 19:06 ` [PATCH v4 06/14] kbuild: link symbol CRCs at final link, removing CONFIG_MODULE_REL_CRCS Masahiro Yamada
2022-05-08 19:06   ` Masahiro Yamada
2022-05-09 17:50   ` Nick Desaulniers
2022-05-09 17:50     ` Nick Desaulniers
2022-05-10 13:03     ` Masahiro Yamada
2022-05-10 13:03       ` Masahiro Yamada
2022-05-08 19:06 ` [PATCH v4 07/14] kbuild: stop merging *.symversions Masahiro Yamada
2022-05-08 19:06   ` Masahiro Yamada
2022-05-08 19:06 ` [PATCH v4 08/14] genksyms: adjust the output format to modpost Masahiro Yamada
2022-05-08 19:06   ` Masahiro Yamada
2022-05-08 19:06 ` [PATCH v4 09/14] kbuild: do not create *.prelink.o for Clang LTO or IBT Masahiro Yamada
2022-05-08 19:06   ` Masahiro Yamada
2022-05-09 23:13   ` Sami Tolvanen
2022-05-09 23:13     ` Sami Tolvanen
2022-05-08 19:06 ` [PATCH v4 10/14] kbuild: check static EXPORT_SYMBOL* by script instead of modpost Masahiro Yamada
2022-05-08 19:06   ` Masahiro Yamada
2022-05-09 18:05   ` Nick Desaulniers
2022-05-09 18:05     ` Nick Desaulniers
2022-05-10 13:53     ` Masahiro Yamada
2022-05-10 13:53       ` Masahiro Yamada
2022-05-08 19:06 ` [PATCH v4 11/14] kbuild: make built-in.a rule robust against too long argument error Masahiro Yamada
2022-05-08 19:06   ` Masahiro Yamada
2022-05-08 19:06 ` [PATCH v4 12/14] kbuild: make *.mod " Masahiro Yamada
2022-05-08 19:06   ` Masahiro Yamada
2022-05-08 19:06 ` [PATCH v4 13/14] kbuild: add cmd_and_savecmd macro Masahiro Yamada
2022-05-08 19:06   ` Masahiro Yamada
2022-05-08 19:06 ` [PATCH v4 14/14] kbuild: rebuild multi-object modules when objtool is updated Masahiro Yamada
2022-05-08 19:06   ` Masahiro Yamada
2022-05-09  4:24 ` [PATCH v4 00/14] kbuild: yet another series of cleanups (modpost, LTO, MODULE_REL_CRCS, export.h) Masahiro Yamada
2022-05-09  4:24   ` Masahiro Yamada
2022-05-09 22:13   ` Nathan Chancellor
2022-05-09 22:13     ` Nathan Chancellor
2022-05-10  6:55     ` Masahiro Yamada
2022-05-10  6:55       ` Masahiro Yamada
2022-05-09 22:19   ` Nathan Chancellor
2022-05-09 22:19     ` Nathan Chancellor

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAKwvOdk1z6jnTA79W8qyy1sWuoa29YxppU1gAkHkQ=g6UiVvCw@mail.gmail.com' \
    --to=ndesaulniers@google.com \
    --cc=ardb@kernel.org \
    --cc=linux-kbuild@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-modules@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=llvm@lists.linux.dev \
    --cc=masahiroy@kernel.org \
    --cc=nathan@kernel.org \
    --cc=nicolas@fjasle.eu \
    --cc=peterz@infradead.org \
    --cc=quic_jjohnson@quicinc.com \
    --cc=samitolvanen@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.