| Message ID | 20200904112401.667645-2-lmb@cloudflare.com |
|---|---|
| State | RFC |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | RFC: Make check_func_arg table driven | expand |
On Fri, Sep 4, 2020 at 4:30 AM Lorenz Bauer <lmb@cloudflare.com> 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. > > Fixes: eae2e83e6263 ("bpf: Add BTF_SET_START/END macros") > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com> > --- > 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). > #else > > -- > 2.25.1 >
On Wed, 9 Sep 2020 at 05:04, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Sep 4, 2020 at 4:30 AM Lorenz Bauer <lmb@cloudflare.com> 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@cloudflare.com> > > --- > > 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
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"); #else
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. Fixes: eae2e83e6263 ("bpf: Add BTF_SET_START/END macros") Signed-off-by: Lorenz Bauer <lmb@cloudflare.com> --- include/linux/btf_ids.h | 6 +++--- tools/include/linux/btf_ids.h | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-)