On Wed, 9 Sep 2020 at 05:04, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Fri, Sep 4, 2020 at 4:30 AM Lorenz Bauer <lmb@xxxxxxxxxxxxxx> wrote:
> >
> > The extern symbol declaration should be on the BTF_SET_START macro, not
> > on BTF_SET_START_GLOBAL, since in the global case the symbol will be
> > declared in a header somewhere.
>
> See below about my confusion. But besides that, is there any problem
> to have this extern in both BTF_SET_START and BTF_SET_START_GLOBAL?
> Are there any problems caused by this? This commit message doesn't
> explain what problem it's trying to solve.
I was getting compilation errors, and moving the extern to match what
the BTF_ID_LIST did fixed it. Of course I now can't reproduce it when
dropping the patch, so the mistake was on my side.
>
> >
> > Fixes: eae2e83e6263 ("bpf: Add BTF_SET_START/END macros")
> > Signed-off-by: Lorenz Bauer <lmb@xxxxxxxxxxxxxx>
> > ---
> > include/linux/btf_ids.h | 6 +++---
> > tools/include/linux/btf_ids.h | 6 +++---
> > 2 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> > index 210b086188a3..42aa667d4433 100644
> > --- a/include/linux/btf_ids.h
> > +++ b/include/linux/btf_ids.h
> > @@ -121,7 +121,8 @@ asm( \
> >
> > #define BTF_SET_START(name) \
> > __BTF_ID_LIST(name, local) \
> > -__BTF_SET_START(name, local)
> > +__BTF_SET_START(name, local) \
> > +extern struct btf_id_set name;
> >
> > #define BTF_SET_START_GLOBAL(name) \
> > __BTF_ID_LIST(name, globl) \
> > @@ -131,8 +132,7 @@ __BTF_SET_START(name, globl)
> > asm( \
> > ".pushsection " BTF_IDS_SECTION ",\"a\"; \n" \
> > ".size __BTF_ID__set__" #name ", .-" #name " \n" \
> > -".popsection; \n"); \
> > -extern struct btf_id_set name;
> > +".popsection; \n");
> >
> > #else
> >
> > diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
> > index 210b086188a3..42aa667d4433 100644
> > --- a/tools/include/linux/btf_ids.h
> > +++ b/tools/include/linux/btf_ids.h
> > @@ -121,7 +121,8 @@ asm( \
> >
> > #define BTF_SET_START(name) \
> > __BTF_ID_LIST(name, local) \
> > -__BTF_SET_START(name, local)
> > +__BTF_SET_START(name, local) \
> > +extern struct btf_id_set name;
> >
> > #define BTF_SET_START_GLOBAL(name) \
> > __BTF_ID_LIST(name, globl) \
> > @@ -131,8 +132,7 @@ __BTF_SET_START(name, globl)
> > asm( \
> > ".pushsection " BTF_IDS_SECTION ",\"a\"; \n" \
> > ".size __BTF_ID__set__" #name ", .-" #name " \n" \
> > -".popsection; \n"); \
> > -extern struct btf_id_set name;
> > +".popsection; \n");
> >
>
> This diff is extremely misleading. It's actually BTF_SET_END macro.
> Coupled with your commit message, it's double-misleading, because you
> are moving extern declaration from BTF_SET_END (which is used with
> both BTF_SET_START and BTF_SET_START_GLOBAL) to BTF_SET_START. Not
> from BTF_SET_START_GLOBAL to BTF_SET_START (as your commit message
> implies, at least that's how I read it).
Yeah, that's true. Probably that's why I got the commit message wrong
as well. I think this is because the diff heuristics think
__BTF_SET_START() is a function like thing? Adding some indentation
might fix this, but I couldn't find details on what diff does with a
quick search.
>
> > #else
> >
> > --
> > 2.25.1
> >
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
