[RFC PATCH bpf-next] bpftool: Mark generated skeleton headers as system headers

[RFC PATCH bpf-next] bpftool: Mark generated skeleton headers as system headers

[Jörn-Thorben Hinz]

Hi,

after compiling a skeleton-using program with -pedantic once and
stumbling across a tiniest incorrectness in skeletons with it[1], I was
debating whether it makes sense to suppress warnings from skeleton
headers.

Happy about comments about this. This change might be too suppressive
towards warnings and maybe ignoring only -Woverlength-strings directly
in OBJ_NAME__elf_bytes() be a better idea. Or keep all warnings from
skeletons available as-is to have them more visible in and around
bpftool’s development.

[1] https://lore.kernel.org/r/20220726133203.514087-1-jthinz@mailbox.tu-berlin.de/

Commit message:

A userspace program including a skeleton generated by bpftool might use
an arbitrary set of compiler flags, including enabling various warnings.

For example, with -Woverlength-strings the string constant in
OBJ_NAME__elf_bytes() causes a warning due to its usually huge length.
This string length is not an actual problem with GCC and clang, though,
it’s “just” not required by the C standard to be supported.

Skeleton headers are likely not placed in a system include path. To
avoid the above warning and similar noise for the *user* of a skeleton,
explicitly mark the header as a system header which disables almost all
warnings for it when included.

Skeleton headers generated during the build of bpftool are not marked to
keep potential warnings available to bpftool’s developers.

Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
---
 tools/bpf/bpftool/Makefile |  2 ++
 tools/bpf/bpftool/gen.c    | 30 +++++++++++++++++++++++++++---
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index 6b5b3a99f79d..5f484d7929db 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -196,6 +196,8 @@ endif
 
 CFLAGS += $(if $(BUILD_BPF_SKELS),,-DBPFTOOL_WITHOUT_SKELETONS)
 
+$(BOOTSTRAP_OUTPUT)%.o: CFLAGS += -DBPFTOOL_BOOTSTRAP
+
 $(BOOTSTRAP_OUTPUT)disasm.o: $(srctree)/kernel/bpf/disasm.c
 	$(QUIET_CC)$(HOSTCC) $(HOST_CFLAGS) -c -MMD $< -o $@
 
diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index 1cf53bb01936..82053aceec78 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -1006,7 +1006,15 @@ static int do_skeleton(int argc, char **argv)
 		/* THIS FILE IS AUTOGENERATED BY BPFTOOL! */		    \n\
 		#ifndef %2$s						    \n\
 		#define %2$s						    \n\
-									    \n\
+		"
+#ifndef BPFTOOL_BOOTSTRAP
+		"\
+		\n\
+		_Pragma(\"GCC system_header\")				    \n\
+		"
+#endif
+		"\
+		\n\
 		#include <bpf/skel_internal.h>				    \n\
 									    \n\
 		struct %1$s {						    \n\
@@ -1022,7 +1030,15 @@ static int do_skeleton(int argc, char **argv)
 		/* THIS FILE IS AUTOGENERATED BY BPFTOOL! */		    \n\
 		#ifndef %2$s						    \n\
 		#define %2$s						    \n\
-									    \n\
+		"
+#ifndef BPFTOOL_BOOTSTRAP
+		"\
+		\n\
+		_Pragma(\"GCC system_header\")				    \n\
+		"
+#endif
+		"\
+		\n\
 		#include <errno.h>					    \n\
 		#include <stdlib.h>					    \n\
 		#include <bpf/libbpf.h>					    \n\
@@ -1415,7 +1431,15 @@ static int do_subskeleton(int argc, char **argv)
 	/* THIS FILE IS AUTOGENERATED! */				    \n\
 	#ifndef %2$s							    \n\
 	#define %2$s							    \n\
-									    \n\
+		"
+#ifndef BPFTOOL_BOOTSTRAP
+	"\
+	\n\
+	_Pragma(\"GCC system_header\")					    \n\
+	"
+#endif
+	"\
+	\n\
 	#include <errno.h>						    \n\
 	#include <stdlib.h>						    \n\
 	#include <bpf/libbpf.h>						    \n\
-- 
2.30.2

Re: [RFC PATCH bpf-next] bpftool: Mark generated skeleton headers as system headers

[Yonghong Song]

On 7/28/22 9:56 AM, Jörn-Thorben Hinz wrote:
> Hi,
> 
> after compiling a skeleton-using program with -pedantic once and
> stumbling across a tiniest incorrectness in skeletons with it[1], I was
> debating whether it makes sense to suppress warnings from skeleton
> headers.
> 
> Happy about comments about this. This change might be too suppressive
> towards warnings and maybe ignoring only -Woverlength-strings directly
> in OBJ_NAME__elf_bytes() be a better idea. Or keep all warnings from
> skeletons available as-is to have them more visible in and around
> bpftool’s development.

This is my 2cents. As you mentioned, skeleton file are per program
and not in system header file directory. I would like not to mark
these header files as system files. Since different program will
generate different skeleton headers, suppressing warnings
will prevent from catching potential issues in certain cases.

Also, since the warning is triggered by extra user flags like -pedantic
when building bpftool, user can also add -Wno-overlength-strings
in the extra user flags.

> 
> [1] https://lore.kernel.org/r/20220726133203.514087-1-jthinz@mailbox.tu-berlin.de/
> 
> Commit message:
> 
> A userspace program including a skeleton generated by bpftool might use
> an arbitrary set of compiler flags, including enabling various warnings.
> 
> For example, with -Woverlength-strings the string constant in
> OBJ_NAME__elf_bytes() causes a warning due to its usually huge length.
> This string length is not an actual problem with GCC and clang, though,
> it’s “just” not required by the C standard to be supported.
> 
> Skeleton headers are likely not placed in a system include path. To
> avoid the above warning and similar noise for the *user* of a skeleton,
> explicitly mark the header as a system header which disables almost all
> warnings for it when included.
> 
> Skeleton headers generated during the build of bpftool are not marked to
> keep potential warnings available to bpftool’s developers.
> 
> Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
> ---
>   tools/bpf/bpftool/Makefile |  2 ++
>   tools/bpf/bpftool/gen.c    | 30 +++++++++++++++++++++++++++---
>   2 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index 6b5b3a99f79d..5f484d7929db 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile
> @@ -196,6 +196,8 @@ endif
>   
>   CFLAGS += $(if $(BUILD_BPF_SKELS),,-DBPFTOOL_WITHOUT_SKELETONS)
>   
> +$(BOOTSTRAP_OUTPUT)%.o: CFLAGS += -DBPFTOOL_BOOTSTRAP
> +
>   $(BOOTSTRAP_OUTPUT)disasm.o: $(srctree)/kernel/bpf/disasm.c
>   	$(QUIET_CC)$(HOSTCC) $(HOST_CFLAGS) -c -MMD $< -o $@
>   
> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index 1cf53bb01936..82053aceec78 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -1006,7 +1006,15 @@ static int do_skeleton(int argc, char **argv)
>   		/* THIS FILE IS AUTOGENERATED BY BPFTOOL! */		    \n\
>   		#ifndef %2$s						    \n\
>   		#define %2$s						    \n\
> -									    \n\
> +		"
> +#ifndef BPFTOOL_BOOTSTRAP
> +		"\
> +		\n\
> +		_Pragma(\"GCC system_header\")				    \n\
> +		"
> +#endif
> +		"\
> +		\n\
>   		#include <bpf/skel_internal.h>				    \n\
>   									    \n\
>   		struct %1$s {						    \n\
> @@ -1022,7 +1030,15 @@ static int do_skeleton(int argc, char **argv)
>   		/* THIS FILE IS AUTOGENERATED BY BPFTOOL! */		    \n\
>   		#ifndef %2$s						    \n\
>   		#define %2$s						    \n\
> -									    \n\
> +		"
> +#ifndef BPFTOOL_BOOTSTRAP
> +		"\
> +		\n\
> +		_Pragma(\"GCC system_header\")				    \n\
> +		"
> +#endif
> +		"\
> +		\n\
>   		#include <errno.h>					    \n\
>   		#include <stdlib.h>					    \n\
>   		#include <bpf/libbpf.h>					    \n\
> @@ -1415,7 +1431,15 @@ static int do_subskeleton(int argc, char **argv)
>   	/* THIS FILE IS AUTOGENERATED! */				    \n\
>   	#ifndef %2$s							    \n\
>   	#define %2$s							    \n\
> -									    \n\
> +		"
> +#ifndef BPFTOOL_BOOTSTRAP
> +	"\
> +	\n\
> +	_Pragma(\"GCC system_header\")					    \n\
> +	"
> +#endif
> +	"\
> +	\n\
>   	#include <errno.h>						    \n\
>   	#include <stdlib.h>						    \n\
>   	#include <bpf/libbpf.h>						    \n\

Re: [RFC PATCH bpf-next] bpftool: Mark generated skeleton headers as system headers

[Quentin Monnet]

On 28/07/2022 19:25, Yonghong Song wrote:
> 
> 
> On 7/28/22 9:56 AM, Jörn-Thorben Hinz wrote:
>> Hi,
>>
>> after compiling a skeleton-using program with -pedantic once and
>> stumbling across a tiniest incorrectness in skeletons with it[1], I was
>> debating whether it makes sense to suppress warnings from skeleton
>> headers.
>>
>> Happy about comments about this. This change might be too suppressive
>> towards warnings and maybe ignoring only -Woverlength-strings directly
>> in OBJ_NAME__elf_bytes() be a better idea. Or keep all warnings from
>> skeletons available as-is to have them more visible in and around
>> bpftool’s development.
> 
> This is my 2cents. As you mentioned, skeleton file are per program
> and not in system header file directory. I would like not to mark
> these header files as system files. Since different program will
> generate different skeleton headers, suppressing warnings
> will prevent from catching potential issues in certain cases.
> 
> Also, since the warning is triggered by extra user flags like -pedantic
> when building bpftool, user can also add -Wno-overlength-strings
> in the extra user flags.

I agree with Yonghong, I don't think it's a good idea to mark the whole
file as a system header. I would maybe consider the other solution where
we can disable the warning locally in the skeleton, just around
OBJ_NAME__elf_bytes() as you suggested. Although I suppose we'd need
several pragmas if we want to silence it for GCC and clang, for example?
It looks like your patch was only addressing GCC?

Thanks for the contribution,
Quentin

Re: [RFC PATCH bpf-next] bpftool: Mark generated skeleton headers as system headers

[Yonghong Song]

On 7/29/22 3:12 AM, Quentin Monnet wrote:
> On 28/07/2022 19:25, Yonghong Song wrote:
>>
>>
>> On 7/28/22 9:56 AM, Jörn-Thorben Hinz wrote:
>>> Hi,
>>>
>>> after compiling a skeleton-using program with -pedantic once and
>>> stumbling across a tiniest incorrectness in skeletons with it[1], I was
>>> debating whether it makes sense to suppress warnings from skeleton
>>> headers.
>>>
>>> Happy about comments about this. This change might be too suppressive
>>> towards warnings and maybe ignoring only -Woverlength-strings directly
>>> in OBJ_NAME__elf_bytes() be a better idea. Or keep all warnings from
>>> skeletons available as-is to have them more visible in and around
>>> bpftool’s development.
>>
>> This is my 2cents. As you mentioned, skeleton file are per program
>> and not in system header file directory. I would like not to mark
>> these header files as system files. Since different program will
>> generate different skeleton headers, suppressing warnings
>> will prevent from catching potential issues in certain cases.
>>
>> Also, since the warning is triggered by extra user flags like -pedantic
>> when building bpftool, user can also add -Wno-overlength-strings
>> in the extra user flags.
> 
> I agree with Yonghong, I don't think it's a good idea to mark the whole
> file as a system header. I would maybe consider the other solution where
> we can disable the warning locally in the skeleton, just around
> OBJ_NAME__elf_bytes() as you suggested. Although I suppose we'd need
> several pragmas if we want to silence it for GCC and clang, for example?
> It looks like your patch was only addressing GCC?

Quentin, "#pragma GCC system_header" also works for clang. Basically
clang implemented system_header pragma identical to gcc counterpart.

> 
> Thanks for the contribution,
> Quentin

Re: [RFC PATCH bpf-next] bpftool: Mark generated skeleton headers as system headers

[Jörn-Thorben Hinz]

On Thu, 2022-07-28 at 11:25 -0700, Yonghong Song wrote:
> 
> 
> On 7/28/22 9:56 AM, Jörn-Thorben Hinz wrote:
> > Hi,
> > 
> > after compiling a skeleton-using program with -pedantic once and
> > stumbling across a tiniest incorrectness in skeletons with it[1], I
> > was
> > debating whether it makes sense to suppress warnings from skeleton
> > headers.
> > 
> > Happy about comments about this. This change might be too
> > suppressive
> > towards warnings and maybe ignoring only -Woverlength-strings
> > directly
> > in OBJ_NAME__elf_bytes() be a better idea. Or keep all warnings
> > from
> > skeletons available as-is to have them more visible in and around
> > bpftool’s development.
> 
> This is my 2cents.
Thanks for the comment, Yonghong.

> As you mentioned, skeleton file are per program
> and not in system header file directory. I would like not to mark
> these header files as system files. Since different program will
> generate different skeleton headers, suppressing warnings
> will prevent from catching potential issues in certain cases.
I admittedly didn’t take a full detailed look at it. But isn’t the
general skeleton structure rather static, with only small differences
depending on e.g. the sections, maps present in a BPF object?

> 
> Also, since the warning is triggered by extra user flags like -
> pedantic
> when building bpftool, user can also add -Wno-overlength-strings
> in the extra user flags.
Maybe I should have worded it more clearly. This was not about somebody
adding flags to the build of bpftool itself. But rather about later
using bpftool (prebuilt by your distribution, maybe) to generate a
skeleton for some foreign BPF object/program.

That foreign program could use various compiler flags, which are
outside of the reach of bpftool. But that foreign program also does not
have any influence on the content on the skeleton header. Unless
somebody wants to patch it after generating it (very unlikely).

So I looked at it mostly as a non-kernel-developer user of bpftool.
From that view, I feel like a skeleton header should behave like any
library header and not produce unnecessary warnings in a program
including it. Like e.g header files from /usr/include, which are, well,
usually implicitly identified as system headers :-)

In the build of bpftool, I explicitly skipped the pragmas. So any
warning caused by the two skeletons generated and used during bpftool’s
build process (pid_iter.skel.h and profiler.skel.h) will still be
visible.

Would it be an idea to by default apply this patch (or a similar one),
but build the bpftool in selftests/bpf with a flag that keeps all
warnings available? — like the -DBPFTOOL_BOOTSTRAP below already
achieves, that flag could be renamed. bpftool is apparently already
built with slightly different CFLAGS for the selftests.

To add that: I’m aware this patch is probably nit-picky and the
warnings, if even, a very minor issue.

> 
> > 
> > [1] 
> > https://lore.kernel.org/r/20220726133203.514087-1-jthinz@mailbox.tu-berlin.de/
> > 
> > Commit message:
> > 
> > A userspace program including a skeleton generated by bpftool might
> > use
> > an arbitrary set of compiler flags, including enabling various
> > warnings.
> > 
> > For example, with -Woverlength-strings the string constant in
> > OBJ_NAME__elf_bytes() causes a warning due to its usually huge
> > length.
> > This string length is not an actual problem with GCC and clang,
> > though,
> > it’s “just” not required by the C standard to be supported.
> > 
> > Skeleton headers are likely not placed in a system include path. To
> > avoid the above warning and similar noise for the *user* of a
> > skeleton,
> > explicitly mark the header as a system header which disables almost
> > all
> > warnings for it when included.
> > 
> > Skeleton headers generated during the build of bpftool are not
> > marked to
> > keep potential warnings available to bpftool’s developers.
> > 
> > Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
> > ---
> >   tools/bpf/bpftool/Makefile |  2 ++
> >   tools/bpf/bpftool/gen.c    | 30 +++++++++++++++++++++++++++---
> >   2 files changed, 29 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/bpf/bpftool/Makefile
> > b/tools/bpf/bpftool/Makefile
> > index 6b5b3a99f79d..5f484d7929db 100644
> > --- a/tools/bpf/bpftool/Makefile
> > +++ b/tools/bpf/bpftool/Makefile
> > @@ -196,6 +196,8 @@ endif
> >   
> >   CFLAGS += $(if $(BUILD_BPF_SKELS),,-DBPFTOOL_WITHOUT_SKELETONS)
> >   
> > +$(BOOTSTRAP_OUTPUT)%.o: CFLAGS += -DBPFTOOL_BOOTSTRAP
> > +
> >   $(BOOTSTRAP_OUTPUT)disasm.o: $(srctree)/kernel/bpf/disasm.c
> >         $(QUIET_CC)$(HOSTCC) $(HOST_CFLAGS) -c -MMD $< -o $@
> >   
> > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> > index 1cf53bb01936..82053aceec78 100644
> > --- a/tools/bpf/bpftool/gen.c
> > +++ b/tools/bpf/bpftool/gen.c
> > @@ -1006,7 +1006,15 @@ static int do_skeleton(int argc, char
> > **argv)
> >                 /* THIS FILE IS AUTOGENERATED BY BPFTOOL!
> > */                \n\
> >                 #ifndef
> > %2$s                                                \n\
> >                 #define
> > %2$s                                                \n\
> > -
> >                                                                     
> >        \n\
> > +               "
> > +#ifndef BPFTOOL_BOOTSTRAP
> > +               "\
> > +               \n\
> > +               _Pragma(\"GCC
> > system_header\")                              \n\
> > +               "
> > +#endif
> > +               "\
> > +               \n\
> >                 #include
> > <bpf/skel_internal.h>                              \n\
> >                                                                    
> >          \n\
> >                 struct %1$s
> > {                                               \n\
> > @@ -1022,7 +1030,15 @@ static int do_skeleton(int argc, char
> > **argv)
> >                 /* THIS FILE IS AUTOGENERATED BY BPFTOOL!
> > */                \n\
> >                 #ifndef
> > %2$s                                                \n\
> >                 #define
> > %2$s                                                \n\
> > -
> >                                                                     
> >        \n\
> > +               "
> > +#ifndef BPFTOOL_BOOTSTRAP
> > +               "\
> > +               \n\
> > +               _Pragma(\"GCC
> > system_header\")                              \n\
> > +               "
> > +#endif
> > +               "\
> > +               \n\
> >                 #include
> > <errno.h>                                          \n\
> >                 #include
> > <stdlib.h>                                         \n\
> >                 #include
> > <bpf/libbpf.h>                                     \n\
> > @@ -1415,7 +1431,15 @@ static int do_subskeleton(int argc, char
> > **argv)
> >         /* THIS FILE IS AUTOGENERATED!
> > */                                   \n\
> >         #ifndef
> > %2$s                                                        \n\
> >         #define
> > %2$s                                                        \n\
> > -
> >                                                                     
> >        \n\
> > +               "
> > +#ifndef BPFTOOL_BOOTSTRAP
> > +       "\
> > +       \n\
> > +       _Pragma(\"GCC
> > system_header\")                                      \n\
> > +       "
> > +#endif
> > +       "\
> > +       \n\
> >         #include
> > <errno.h>                                                  \n\
> >         #include
> > <stdlib.h>                                                 \n\
> >         #include
> > <bpf/libbpf.h>                                             \n\

Re: [RFC PATCH bpf-next] bpftool: Mark generated skeleton headers as system headers

[Jörn-Thorben Hinz]

On Fri, 2022-07-29 at 11:12 +0100, Quentin Monnet wrote:
> On 28/07/2022 19:25, Yonghong Song wrote:
> > 
> > 
> > On 7/28/22 9:56 AM, Jörn-Thorben Hinz wrote:
> > > Hi,
> > > 
> > > after compiling a skeleton-using program with -pedantic once and
> > > stumbling across a tiniest incorrectness in skeletons with it[1],
> > > I was
> > > debating whether it makes sense to suppress warnings from
> > > skeleton
> > > headers.
> > > 
> > > Happy about comments about this. This change might be too
> > > suppressive
> > > towards warnings and maybe ignoring only -Woverlength-strings
> > > directly
> > > in OBJ_NAME__elf_bytes() be a better idea. Or keep all warnings
> > > from
> > > skeletons available as-is to have them more visible in and around
> > > bpftool’s development.
> > 
> > This is my 2cents. As you mentioned, skeleton file are per program
> > and not in system header file directory. I would like not to mark
> > these header files as system files. Since different program will
> > generate different skeleton headers, suppressing warnings
> > will prevent from catching potential issues in certain cases.
> > 
> > Also, since the warning is triggered by extra user flags like -
> > pedantic
> > when building bpftool, user can also add -Wno-overlength-strings
> > in the extra user flags.
> 
> I agree with Yonghong, I don't think it's a good idea to mark the
> whole
> file as a system header. I would maybe consider the other solution
> where
> we can disable the warning locally in the skeleton, just around
> OBJ_NAME__elf_bytes() as you suggested.
That was my first attempt. But, after a second look, -Wsign-conversion,
-Wcast-qual, and -Wreserved-id-macro would also show a warning for a
skeleton header. That’s why I switched to the `GCC system_header`.
Ignoring these four warnings for the whole header would also be an
alternative. Doing that for all of them only at the place where they
are triggered might become less pretty.

Please also see my longer answer to Yonghong.

> Although I suppose we'd need
> several pragmas if we want to silence it for GCC and clang, for
> example?
> It looks like your patch was only addressing GCC?
It is only explicitly mentioned for `GCC diagnostic …` in [1], but
clang seems to support the `GCC system_header` for compatibility, too.
So both are covered by using GCC’s pragmas here.

[1]
https://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-via-pragmas
(also the following subsection)

> 
> Thanks for the contribution,
Thanks for your reply!

> Quentin

Re: [RFC PATCH bpf-next] bpftool: Mark generated skeleton headers as system headers

[Yonghong Song]

On 7/29/22 10:29 AM, Jörn-Thorben Hinz wrote:
> On Thu, 2022-07-28 at 11:25 -0700, Yonghong Song wrote:
>>
>>
>> On 7/28/22 9:56 AM, Jörn-Thorben Hinz wrote:
>>> Hi,
>>>
>>> after compiling a skeleton-using program with -pedantic once and
>>> stumbling across a tiniest incorrectness in skeletons with it[1], I
>>> was
>>> debating whether it makes sense to suppress warnings from skeleton
>>> headers.
>>>
>>> Happy about comments about this. This change might be too
>>> suppressive
>>> towards warnings and maybe ignoring only -Woverlength-strings
>>> directly
>>> in OBJ_NAME__elf_bytes() be a better idea. Or keep all warnings
>>> from
>>> skeletons available as-is to have them more visible in and around
>>> bpftool’s development.
>>
>> This is my 2cents.
> Thanks for the comment, Yonghong.
> 
>> As you mentioned, skeleton file are per program
>> and not in system header file directory. I would like not to mark
>> these header files as system files. Since different program will
>> generate different skeleton headers, suppressing warnings
>> will prevent from catching potential issues in certain cases.
> I admittedly didn’t take a full detailed look at it. But isn’t the
> general skeleton structure rather static, with only small differences
> depending on e.g. the sections, maps present in a BPF object?

You are correctly. Most skeleton code are not changed between
different programs.

> 
>>
>> Also, since the warning is triggered by extra user flags like -
>> pedantic
>> when building bpftool, user can also add -Wno-overlength-strings
>> in the extra user flags.
> Maybe I should have worded it more clearly. This was not about somebody
> adding flags to the build of bpftool itself. But rather about later
> using bpftool (prebuilt by your distribution, maybe) to generate a
> skeleton for some foreign BPF object/program.
> 
> That foreign program could use various compiler flags, which are
> outside of the reach of bpftool. But that foreign program also does not
> have any influence on the content on the skeleton header. Unless
> somebody wants to patch it after generating it (very unlikely).
> 
> So I looked at it mostly as a non-kernel-developer user of bpftool.
>  From that view, I feel like a skeleton header should behave like any
> library header and not produce unnecessary warnings in a program
> including it. Like e.g header files from /usr/include, which are, well,
> usually implicitly identified as system headers :-)
> 
> In the build of bpftool, I explicitly skipped the pragmas. So any
> warning caused by the two skeletons generated and used during bpftool’s
> build process (pid_iter.skel.h and profiler.skel.h) will still be
> visible.
> 
> Would it be an idea to by default apply this patch (or a similar one),
> but build the bpftool in selftests/bpf with a flag that keeps all
> warnings available? — like the -DBPFTOOL_BOOTSTRAP below already
> achieves, that flag could be renamed. bpftool is apparently already
> built with slightly different CFLAGS for the selftests.

I still prefer not to mark skeleton header as system header.
Precisely due to skeleton code not changing between different
programs AND skeleton code could change due to new/changed 
functionalities, we can permit by default compilation warnings
to flag potential issues.

> 
> To add that: I’m aware this patch is probably nit-picky and the
> warnings, if even, a very minor issue.
> 
>>
>>>
>>> [1]
>>> https://lore.kernel.org/r/20220726133203.514087-1-jthinz@mailbox.tu-berlin.de/
>>>
>>> Commit message:
>>>
>>> A userspace program including a skeleton generated by bpftool might
>>> use
>>> an arbitrary set of compiler flags, including enabling various
>>> warnings.
>>>
>>> For example, with -Woverlength-strings the string constant in
>>> OBJ_NAME__elf_bytes() causes a warning due to its usually huge
>>> length.
>>> This string length is not an actual problem with GCC and clang,
>>> though,
>>> it’s “just” not required by the C standard to be supported.
>>>
>>> Skeleton headers are likely not placed in a system include path. To
>>> avoid the above warning and similar noise for the *user* of a
>>> skeleton,
>>> explicitly mark the header as a system header which disables almost
>>> all
>>> warnings for it when included.
>>>
>>> Skeleton headers generated during the build of bpftool are not
>>> marked to
>>> keep potential warnings available to bpftool’s developers.
>>>
>>> Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
>>> ---
>>>    tools/bpf/bpftool/Makefile |  2 ++
>>>    tools/bpf/bpftool/gen.c    | 30 +++++++++++++++++++++++++++---
>>>    2 files changed, 29 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/bpf/bpftool/Makefile
>>> b/tools/bpf/bpftool/Makefile
>>> index 6b5b3a99f79d..5f484d7929db 100644
>>> --- a/tools/bpf/bpftool/Makefile
>>> +++ b/tools/bpf/bpftool/Makefile
>>> @@ -196,6 +196,8 @@ endif
>>>    
>>>    CFLAGS += $(if $(BUILD_BPF_SKELS),,-DBPFTOOL_WITHOUT_SKELETONS)
>>>    
>>> +$(BOOTSTRAP_OUTPUT)%.o: CFLAGS += -DBPFTOOL_BOOTSTRAP
>>> +
>>>    $(BOOTSTRAP_OUTPUT)disasm.o: $(srctree)/kernel/bpf/disasm.c
>>>          $(QUIET_CC)$(HOSTCC) $(HOST_CFLAGS) -c -MMD $< -o $@
>>>    
>>> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
>>> index 1cf53bb01936..82053aceec78 100644
>>> --- a/tools/bpf/bpftool/gen.c
>>> +++ b/tools/bpf/bpftool/gen.c
>>> @@ -1006,7 +1006,15 @@ static int do_skeleton(int argc, char
>>> **argv)
>>>                  /* THIS FILE IS AUTOGENERATED BY BPFTOOL!
>>> */                \n\
>>>                  #ifndef
>>> %2$s                                                \n\
>>>                  #define
>>> %2$s                                                \n\
>>> -
>>>                                                                      
>>>         \n\
>>> +               "
>>> +#ifndef BPFTOOL_BOOTSTRAP
>>> +               "\
>>> +               \n\
>>> +               _Pragma(\"GCC
>>> system_header\")                              \n\
>>> +               "
>>> +#endif
>>> +               "\
>>> +               \n\
>>>                  #include
>>> <bpf/skel_internal.h>                              \n\
>>>                                                                     
>>>           \n\
>>>                  struct %1$s
>>> {                                               \n\
>>> @@ -1022,7 +1030,15 @@ static int do_skeleton(int argc, char
>>> **argv)
>>>                  /* THIS FILE IS AUTOGENERATED BY BPFTOOL!
>>> */                \n\
>>>                  #ifndef
>>> %2$s                                                \n\
>>>                  #define
>>> %2$s                                                \n\
>>> -
>>>                                                                      
>>>         \n\
>>> +               "
>>> +#ifndef BPFTOOL_BOOTSTRAP
>>> +               "\
>>> +               \n\
>>> +               _Pragma(\"GCC
>>> system_header\")                              \n\
>>> +               "
>>> +#endif
>>> +               "\
>>> +               \n\
>>>                  #include
>>> <errno.h>                                          \n\
>>>                  #include
>>> <stdlib.h>                                         \n\
>>>                  #include
>>> <bpf/libbpf.h>                                     \n\
>>> @@ -1415,7 +1431,15 @@ static int do_subskeleton(int argc, char
>>> **argv)
>>>          /* THIS FILE IS AUTOGENERATED!
>>> */                                   \n\
>>>          #ifndef
>>> %2$s                                                        \n\
>>>          #define
>>> %2$s                                                        \n\
>>> -
>>>                                                                      
>>>         \n\
>>> +               "
>>> +#ifndef BPFTOOL_BOOTSTRAP
>>> +       "\
>>> +       \n\
>>> +       _Pragma(\"GCC
>>> system_header\")                                      \n\
>>> +       "
>>> +#endif
>>> +       "\
>>> +       \n\
>>>          #include
>>> <errno.h>                                                  \n\
>>>          #include
>>> <stdlib.h>                                                 \n\
>>>          #include
>>> <bpf/libbpf.h>                                             \n\
> 
>