Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix CLANG_BPF_CO_RE_PROBE_CMD #125

Merged
merged 1 commit into from
Dec 5, 2023
Merged

fix CLANG_BPF_CO_RE_PROBE_CMD #125

merged 1 commit into from
Dec 5, 2023

Conversation

lavenderfly
Copy link

When global variables are not initialized, the original command will get incorrect results in some versions of clang environment.

Take clang 10 as an example:

clang -v
clang version 10.0.1 (kylin 10.0.1-4.p01.ky10 119f3e344eeeca8114341787f19862d28a85f6ad)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/7.3.0
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/7.3.0
Selected GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/7.3.0
Candidate multilib: .;@m64
Selected multilib: .;@m64

Not initialized (current code):

[root@localhost src]# printf '%s\n' 'struct s { int i; } __attribute__((preserve_access_index)); struct s foo;' |     clang -g -target bpf -S -o - -x c - | grep BTF_KIND_
	.long	1                       # BTF_KIND_STRUCT(id = 1)
	.long	5                       # BTF_KIND_INT(id = 2)

Initialized (new code):

[root@localhost src]# printf '%s\n' 'struct s { int i; } __attribute__((preserve_access_index)); struct s foo={};' |     clang -g -target bpf -S -o - -x c - | grep BTF_KIND_
	.long	1                       # BTF_KIND_STRUCT(id = 1)
	.long	5                       # BTF_KIND_INT(id = 2)
	.long	9                       # BTF_KIND_VAR(id = 3)
	.long	13                      # BTF_KIND_DATASEC(id = 4)

This difference will affect the checking of clang-bpf-core features.

[root@localhost src]# make
...                        libbfd: [ on  ]
...               clang-bpf-co-re: [ OFF ]
...                          llvm: [ on  ]
...                        libcap: [ on  ]

This part of the code looks like this in the kernel:

struct test {
	int a;
	int b;
} __attribute__((preserve_access_index));
volatile struct test global_value_for_test = {};

This problem may only occur here.

@lavenderfly
Copy link
Author

#14 may be related to this PR

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks reasonable, thanks for looking into this and submitting the PR!

Can you please just update your commit title to prefix it with mirror: ? This helps filter commits that are added directly to the mirror repo from those that come from sync-ups with the kernel tree. Ideally, could you also wrap your commit description on 72 characters, please?

This problem may only occur here.

Yes probably, I reimplemented the checks for this repo. Looks like the kernel initialises its code already.

@lavenderfly
Copy link
Author

Can you please just update your commit title to prefix it with mirror: ? This helps filter commits that are added directly to the mirror repo from those that come from sync-ups with the kernel tree. Ideally, could you also wrap your commit description on 72 characters, please?

This commit still fails the check, can you give me more suggestions? For example, does "mirror:" have to be followed by a space?

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I wasn't clear enough, can you please add a space after the prefix?

And the initial description was good, it explained better why the change is needed. Here's what I meant:

mirror: Fix CLANG_BPF_CO_RE_PROBE_CMD 

When global variables are not initialized, the original command will get
incorrect results in some versions of clang environment.

Signed-off-by: ...

When global variables are not initialized, the original command will get
incorrect results in some versions of clang environment.

Signed-off-by: jinzhiguang <jinzhiguang@kylinos.cn>
@lavenderfly
Copy link
Author

Sorry, I wasn't clear enough, can you please add a space after the prefix?

And the initial description was good, it explained better why the change is needed. Here's what I meant:

mirror: Fix CLANG_BPF_CO_RE_PROBE_CMD 

When global variables are not initialized, the original command will get
incorrect results in some versions of clang environment.

Signed-off-by: ...

thanks for your patient explanation

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And thanks for quickly addressing the feedback! Looks all good this time 👍

@qmonnet qmonnet merged commit 515739f into libbpf:main Dec 5, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants