| Message ID | 20240414045124.3098560-1-dmitrii.bundin.a@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bpf: btf: include linux/types.h for u32 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
please use '[PATCH bpf-next]' in subject On Sun, Apr 14, 2024 at 07:51:24AM +0300, Dmitrii Bundin wrote: > Inclusion of the header linux/btf_ids.h relies on indirect inclusion of > the header linux/types.h. Including it directly on the top level helps > to avoid potential problems if linux/types.h hasn't been included > before. > > Signed-off-by: Dmitrii Bundin <dmitrii.bundin.a@gmail.com> > --- > include/linux/btf_ids.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h > index e24aabfe8ecc..c0e3e1426a82 100644 > --- a/include/linux/btf_ids.h > +++ b/include/linux/btf_ids.h > @@ -3,6 +3,8 @@ > #ifndef _LINUX_BTF_IDS_H > #define _LINUX_BTF_IDS_H > > +#include <linux/types.h> /* for u32 */ lgtm, did it actualy cause problem anywhere? there's also tools/include/linux/btf_ids.h jirka > + > struct btf_id_set { > u32 cnt; > u32 ids[]; > -- > 2.34.1 >
On Mon, Apr 15, 2024 at 3:11 PM Jiri Olsa <olsajiri@gmail.com> wrote: > lgtm, did it actualy cause problem anywhere? > > there's also tools/include/linux/btf_ids.h It caused the problems exactly in the file tools/include/linux/btf_ids.h and was reported in https://bugzilla.kernel.org/show_bug.cgi?id=218647 The patch including linux/types.h in tools/include/linux/btf_ids.h is already there https://lore.kernel.org/all/20240328110103.28734-1-ncopa@alpinelinux.org/ I also faced the same compile-error of the form error: unknown type name 'u32' u32 cnt; ^~~ when compiling the bpf tool with glibc 2.28. I think it might be reasonable to add the inclusion in include/linux/btf_ids.h as well to prevent build problems like this.
On Tue, Apr 16, 2024 at 08:27:21AM +0300, Dmitrii Bundin wrote: > On Mon, Apr 15, 2024 at 3:11 PM Jiri Olsa <olsajiri@gmail.com> wrote: > > lgtm, did it actualy cause problem anywhere? > > > > there's also tools/include/linux/btf_ids.h > > It caused the problems exactly in the file > tools/include/linux/btf_ids.h and was reported in > https://bugzilla.kernel.org/show_bug.cgi?id=218647 > The patch including linux/types.h in tools/include/linux/btf_ids.h is > already there https://lore.kernel.org/all/20240328110103.28734-1-ncopa@alpinelinux.org/ > I also faced the same compile-error of the form > > error: unknown type name 'u32' > u32 cnt; > ^~~ > when compiling the bpf tool with glibc 2.28. > > I think it might be reasonable to add the inclusion in > include/linux/btf_ids.h as well to prevent build problems like this. ok, it's in the bpf/master already jirka
On 4/16/24 9:28 AM, Jiri Olsa wrote: > On Tue, Apr 16, 2024 at 08:27:21AM +0300, Dmitrii Bundin wrote: >> On Mon, Apr 15, 2024 at 3:11 PM Jiri Olsa <olsajiri@gmail.com> wrote: >>> lgtm, did it actualy cause problem anywhere? >>> >>> there's also tools/include/linux/btf_ids.h >> >> It caused the problems exactly in the file >> tools/include/linux/btf_ids.h and was reported in >> https://bugzilla.kernel.org/show_bug.cgi?id=218647 >> The patch including linux/types.h in tools/include/linux/btf_ids.h is >> already there https://lore.kernel.org/all/20240328110103.28734-1-ncopa@alpinelinux.org/ >> I also faced the same compile-error of the form >> >> error: unknown type name 'u32' >> u32 cnt; >> ^~~ >> when compiling the bpf tool with glibc 2.28. >> >> I think it might be reasonable to add the inclusion in >> include/linux/btf_ids.h as well to prevent build problems like this. > > ok, it's in the bpf/master already Please add the error description as motivation aka "why" into the commit description, otherwise it's not really obvious looking at it at a later point in time why the include was needed. Thanks, Daniel
On Tue, Apr 16, 2024 at 5:47 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > Please add the error description as motivation aka "why" into the commit > description, otherwise it's not really obvious looking at it at a later > point in time why the include was needed. Doesn't the comment /* for u32 */ following the include explain the purpose? I thought the include was actually missing since relying on indirect declaration of u32 is relatively fragile.
On Wed, Apr 17, 2024 at 09:26:03AM +0300, Dmitrii Bundin wrote: > On Tue, Apr 16, 2024 at 5:47 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > Please add the error description as motivation aka "why" into the commit > > description, otherwise it's not really obvious looking at it at a later > > point in time why the include was needed. > > Doesn't the comment /* for u32 */ following the include explain the > purpose? I thought the include was actually missing since relying on > indirect declaration of u32 is relatively fragile. I think you can add similar descirption as for the already merged tool change in bpf/master, and also include the Fixes/stable tags commit 62248b22d01e96a4d669cde0d7005bd51ebf9e76 Author: Natanael Copa <ncopa@alpinelinux.org> Date: Thu Mar 28 11:59:13 2024 +0100 tools/resolve_btfids: fix build with musl libc Include the header that defines u32. This fixes build of 6.6.23 and 6.1.83 kernels for Alpine Linux, which uses musl libc. I assume that GNU libc indirecly pulls in linux/types.h. Fixes: 9707ac4fe2f5 ("tools/resolve_btfids: Refactor set sorting with types from btf_ids.h") Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218647 Cc: stable@vger.kernel.org Signed-off-by: Natanael Copa <ncopa@alpinelinux.org> Tested-by: Greg Thelen <gthelen@google.com> Link: https://lore.kernel.org/r/20240328110103.28734-1-ncopa@alpinelinux.org Signed-off-by: Alexei Starovoitov <ast@kernel.org> jirka
diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h index e24aabfe8ecc..c0e3e1426a82 100644 --- a/include/linux/btf_ids.h +++ b/include/linux/btf_ids.h @@ -3,6 +3,8 @@ #ifndef _LINUX_BTF_IDS_H #define _LINUX_BTF_IDS_H +#include <linux/types.h> /* for u32 */ + struct btf_id_set { u32 cnt; u32 ids[];
Inclusion of the header linux/btf_ids.h relies on indirect inclusion of the header linux/types.h. Including it directly on the top level helps to avoid potential problems if linux/types.h hasn't been included before. Signed-off-by: Dmitrii Bundin <dmitrii.bundin.a@gmail.com> --- include/linux/btf_ids.h | 2 ++ 1 file changed, 2 insertions(+)