[PATCH] tools/resolve_btfids: Include linux/types.h

[PATCH] tools/resolve_btfids: Include linux/types.h

[Dmitrii Bundin]

When compiling the kernel there's no type definition for u32 within the
translation unit causing compilation errors of the following format:

btf_ids.h:7:2: error: unknown type name ‘u32’

To avoid such errors it's possible to include the common header file
linux/types.h containing the required definition.

Signed-off-by: Dmitrii Bundin <dmitrii.bundin.a@gmail.com>
---
 tools/include/linux/btf_ids.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
index 72535f00572f..7969607efe0d 100644
--- a/tools/include/linux/btf_ids.h
+++ b/tools/include/linux/btf_ids.h
@@ -3,6 +3,8 @@
 #ifndef _LINUX_BTF_IDS_H
 #define _LINUX_BTF_IDS_H
 
+#include <linux/types.h>
+
 struct btf_id_set {
 	u32 cnt;
 	u32 ids[];
-- 
2.17.1

Re: [PATCH] tools/resolve_btfids: Include linux/types.h

[Alexei Starovoitov]

On Fri, Mar 15, 2024 at 2:15 AM Dmitrii Bundin
<dmitrii.bundin.a@gmail.com> wrote:
>
> When compiling the kernel there's no type definition for u32 within the
> translation unit causing compilation errors of the following format:
>
> btf_ids.h:7:2: error: unknown type name ‘u32’

What compiler? What setup?
Steps to repro?

No one reported this, though lots of people
are building resolve_btfids that uses this header
as part of the kernel build.

pw-bot: cr

Re: [PATCH] tools/resolve_btfids: Include linux/types.h

[Dmitrii Bundin]

On Fri, Mar 15, 2024 at 6:41 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> No one reported this, though lots of people
> are building resolve_btfids that uses this header
> as part of the kernel build.

GCC version 7.5.0, GNU Make 4.1
Steps to reproduce:
1. Check out the commit e5eb28f6d1afebed4bb7d740a797d0390bd3a357
2. cd tools/bpf/resolve_btfids/
3. make

The steps above produces the following error messages (similar error
output truncated for clarity):

In file included from main.c:73:0:
/linux/tools/include/linux/btf_ids.h:7:2: error: unknown type name ‘u32’
  u32 cnt;
  ^~~

The other sources including <linux/btf_ids.h> usually includes
(directly or indirectly) <linux/types.h> before which is not the case
for tools/bpf/resolve_btfids/main.c. So that looks reasonable to me to
bring all the required type definitions into scope explicitly in
linux/btf_ids.h. Any thoughts on this?

Re: [PATCH] tools/resolve_btfids: Include linux/types.h

[Khazhy Kumykov]

On Fri, Mar 15, 2024 at 10:09 AM Dmitrii Bundin
<dmitrii.bundin.a@gmail.com> wrote:
>
> On Fri, Mar 15, 2024 at 6:41 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > No one reported this, though lots of people
I'm also seeing this, on clang.

> > are building resolve_btfids that uses this header
> > as part of the kernel build.
>
> GCC version 7.5.0, GNU Make 4.1
> Steps to reproduce:
> 1. Check out the commit e5eb28f6d1afebed4bb7d740a797d0390bd3a357
> 2. cd tools/bpf/resolve_btfids/
> 3. make
>
> The steps above produces the following error messages (similar error
> output truncated for clarity):
>
> In file included from main.c:73:0:
> /linux/tools/include/linux/btf_ids.h:7:2: error: unknown type name ‘u32’
>   u32 cnt;
yup, that's the error I'm seeing.
>   ^~~
>
> The other sources including <linux/btf_ids.h> usually includes
> (directly or indirectly) <linux/types.h> before which is not the case
> for tools/bpf/resolve_btfids/main.c. So that looks reasonable to me to
> bring all the required type definitions into scope explicitly in
> linux/btf_ids.h. Any thoughts on this?
>

Re: [PATCH] tools/resolve_btfids: Include linux/types.h

[Dmitrii Bundin]

On Mon, Mar 18, 2024 at 7:56 PM Khazhy Kumykov <khazhy@chromium.org> wrote:
> I'm also seeing this, on clang.

I think the error is more related to the libc version. I updated the
libc6 to 2.35 and noticed that the <linux/types.h> header is included
indirectly through <sys/stat.h>. The relevant sample of the include
hierarchy for <sys/stat.h> with libc6 v2.35 looks as follows:

. /usr/include/x86_64-linux-gnu/sys/stat.h
.. /usr/include/x86_64-linux-gnu/bits/stat.h
... /usr/include/x86_64-linux-gnu/bits/struct_stat.h
.. /usr/include/x86_64-linux-gnu/bits/statx.h
... /linux/tools/include/uapi/linux/stat.h
.... /linux/tools/include/linux/types.h

The <linux/types.h> is included on the latest line of the sample.
Starting the version 2.28 there's an inclusion of <bits/statx.h> which
was not presented in 2.27.

When building the resolve_btfids with the libc6 version 2.27
<linux/types.h> is not included through <sys/stat.h>. The include
hierarchy for <sys/stat.h> with libc6 v2.27 looks as follows:

. /usr/include/x86_64-linux-gnu/sys/stat.h
.. /usr/include/x86_64-linux-gnu/bits/stat.h
. /usr/include/fcntl.h

To avoid being dependent on the inclusion of <linux/types.h> at some
other place it looks reasonable to include it explicitly to bring all
the necessary declarations before their usage.

What do you think?

Re: [PATCH] tools/resolve_btfids: Include linux/types.h

[Khazhy Kumykov]

On Thu, Mar 21, 2024 at 12:51 PM Dmitrii Bundin
<dmitrii.bundin.a@gmail.com> wrote:
>
> On Mon, Mar 18, 2024 at 7:56 PM Khazhy Kumykov <khazhy@chromium.org> wrote:
> > I'm also seeing this, on clang.
>
> I think the error is more related to the libc version. I updated the
> libc6 to 2.35 and noticed that the <linux/types.h> header is included
> indirectly through <sys/stat.h>. The relevant sample of the include
> hierarchy for <sys/stat.h> with libc6 v2.35 looks as follows:
>
> . /usr/include/x86_64-linux-gnu/sys/stat.h
> .. /usr/include/x86_64-linux-gnu/bits/stat.h
> ... /usr/include/x86_64-linux-gnu/bits/struct_stat.h
> .. /usr/include/x86_64-linux-gnu/bits/statx.h
> ... /linux/tools/include/uapi/linux/stat.h
> .... /linux/tools/include/linux/types.h
>
> The <linux/types.h> is included on the latest line of the sample.
> Starting the version 2.28 there's an inclusion of <bits/statx.h> which
> was not presented in 2.27.
>
> When building the resolve_btfids with the libc6 version 2.27
> <linux/types.h> is not included through <sys/stat.h>. The include
> hierarchy for <sys/stat.h> with libc6 v2.27 looks as follows:
>
> . /usr/include/x86_64-linux-gnu/sys/stat.h
> .. /usr/include/x86_64-linux-gnu/bits/stat.h
> . /usr/include/fcntl.h
>
> To avoid being dependent on the inclusion of <linux/types.h> at some
> other place it looks reasonable to include it explicitly to bring all
> the necessary declarations before their usage.
I would agree - include what you use. The u32 type is defined in
linux/types.h, relying on indirect includes seems fragile (and in this
case, does seem to break folks).
>
> What do you think?