bpf|loader: keep CFLAGS (CLANG_FLAGS) in sync between loader and BPF unit tests #39636

msune · 2025-05-20T13:24:12Z

Prior to this commit BPF unit tests ran with their own set of CFLAGS
(CLANG_FLAGS). They were manually synced to match loader's CFLAGS.

This commit reads the golang base CFLAGS definition and uses it as
base CFLAGS for the unit tests.

This patchset also:

Changes loader's default to gnu99
Removes -Wdeclaration-after-statement. See the following message within this PR for the context.

msune · 2025-05-20T13:26:18Z

On this:

Removes -Wdeclaration-after-statement. See the following message within this PR for the context.

It's either this, or fixing the existing issues (many I suspect, but some come from common headers).

Also mind (in the last commit message):

and checkpatch will anyhow spot them.

Some errors...

bpf_ct_tests.c:42:16: error: mixing declarations and code is incompatible with standards before C99 [-Werror,-Wdeclaration-after-statement]
   42 |         struct iphdr *l3 = dst;
      |                       ^
bpf_ct_tests.c:95:9: error: mixing declarations and code is incompatible with standards before C99 [-Werror,-Wdeclaration-after-statement]
   95 |                 void *data = (void *)(long)ctx->data;
      |                       ^
bpf_ct_tests.c:140:20: error: mixing declarations and code is incompatible with standards before C99 [-Werror,-Wdeclaration-after-statement]
  140 |                 struct ct_entry *entry = map_lookup_elem(get_ct_map4(&tuple), &tuple);
      |                                  ^
bpf_ct_tests.c:104:2: error: mixing declarations and code is incompatible with standards before C99 [-Werror,-Wdeclaration-after-statement]
  104 |         TEST("ct4_syn", {
      |         ^
./common.h:233:20: note: expanded from macro 'TEST'
  233 |         static const char ____name[] = name;                               \
      |                           ^
bpf_ct_tests.c:200:20: error: mixing declarations and code is incompatible with standards before C99 [-Werror,-Wdeclaration-after-statement]
  200 |                 struct ct_entry *entry = map_lookup_elem(get_ct_map4(&tuple), &tuple);
      |                                  ^
bpf_ct_tests.c:165:2: error: mixing declarations and code is incompatible with standards before C99 [-Werror,-Wdeclaration-after-statement]
  165 |         TEST("ct4_rst", {
      |         ^
./common.h:233:20: note: expanded from macro 'TEST'
  233 |         static const char ____name[] = name;                               \
      |                           ^
bpf_ct_tests.c:87:6: error: mixing declarations and code is incompatible with standards before C99 [-Werror,-Wdeclaration-after-statement]
   87 |         int pkt_size = mkpkt(pkt, true);
      |             ^
bpf_ct_tests.c:85:2: error: mixing declarations and code is incompatible with standards before C99 [-Werror,-Wdeclaration-after-statement]
   85 |         test_init();
      |         ^
./common.h:219:23: note: expanded from macro 'test_init'
  219 |         __maybe_unused char *test_result_cursor = 0;                      \
      |                              ^
8 errors generated.

cc @ti-mo

bpf/tests/Makefile

julianwiedmann

Nice improvement! I think we could do the same for -mcpu in a follow-on PR.

One note below. Besides that, could you please re-order the patch series? First harmonize the use of -std and -Wdeclaration-after-statement, and only de-dup the two paths when they're truly identical.

bpf/tests/Makefile

sayboras · 2025-05-22T13:23:20Z

Nice, and thanks for rebase to pick up clang 19.1.7 #39632

msune · 2025-05-22T13:27:52Z

One note below. Besides that, could you please re-order the patch series? First harmonize the use of -std and -Wdeclaration-after-statement, and only de-dup the two paths when they're truly identical.

Reordered.

I think we could do the same for -mcpu in a follow-on PR.

Added the patch here c3c2eb4. As discussed in the commit message, I am not sure if it's worth. GetBPFCPU() uses a kprobe to detect ISA v, and this take ~1second to detect, while we have a reliable $(KERNEL) var, and kernel 5.4 support won't be there forever. I would suggest to drop it, but up to you 😉

julianwiedmann · 2025-05-26T12:31:56Z

I think we could do the same for -mcpu in a follow-on PR.

Added the patch here c3c2eb4. As discussed in the commit message, I am not sure if it's worth. GetBPFCPU() uses a kprobe to detect ISA v, and this take ~1second to detect, while we have a reliable $(KERNEL) var, and kernel 5.4 support won't be there forever. I would suggest to drop it, but up to you 😉

Yes, my suggestion was to explore this in a follow-on PR (and not expand the scope of this PR). 👍

julianwiedmann

lgtm, thank you!

Confused about one part in the patch description though:

Unit test machinery and some tests declare after statement, and checkpatch will anyhow spot them.

I find the "checkpatch will warn" aspect difficult to believe. Did we just ignore all those warnings when adding such code in the unit tests?

msune · 2025-05-26T12:46:45Z

Yes, my suggestion was to explore this in a follow-on PR (and not expand the scope of this PR). 👍

Well.. explored it is/was, then 😃. Dropped last commit.

I find the "checkpatch will warn" aspect difficult to believe. Did we just ignore all those warnings when adding such code in the unit tests?

You are right, even passing --strict to checkpatch.pl. I really thought it was.

I will drop it from the patch info, if that's ok.

Details of checkpatch.pl and an offending commit

~/dev/cilium2/bpf$ git log -1 -p
commit 57d1aa1ee305eadd3708fb2f67923be8fbb034c6 (HEAD -> cflags_sync_unit_loader)
Author: Marc Suñé <marc.sune@isovalent.com>
Date:   Mon May 26 14:36:08 2025 +0200

    Tc
    
    Signed-off-by: Marc Suñé <marc.sune@isovalent.com>

diff --git a/bpf/lib/icmp6.h b/bpf/lib/icmp6.h
index 69a6eb37b9..b0613f0e2b 100644
--- a/bpf/lib/icmp6.h
+++ b/bpf/lib/icmp6.h
@@ -133,6 +133,10 @@ int icmp6_send_ndisc_adv(struct __ctx_buff *ctx, int nh_off,
        union v6addr target_ip;
        __be32 sum;
 
+       sum = 2;
+
+       __be32 sum2;
+
        /*
         * According to RFC4861, sections 4.3 and 7.2.2 unicast neighbour
         * solicitations (reachability check) SHOULD but are NOT REQUIRED to
~/dev/cilium2/bpf$ make checkpatch 
docker container run --rm \
	--workdir /workspace \
	--volume /home/msuneclo/dev/cilium2/bpf/..:/workspace \
	--user "1000:1000" \
	-e GITHUB_REF= -e GITHUB_REPOSITORY= \
	--entrypoint /bin/bash quay.io/cilium/cilium-checkpatch:40ae57b72ea430ab77b45fbed9a1cd4295838ef6@sha256:ccd20b54c779c4645e72dbb48bfcdacb9929cae5d334b0fa6d1ef996af64348f /checkpatch/checkpatch.sh -- --ignore MACRO_ARG_REUSE --strict 
Retrieved 1 commits on top of ref origin/cflags_sync_unit_loader

=========================================================
[1/1] Running on 57d1aa1ee305eadd3708fb2f67923be8fbb034c6
Tc
=========================================================

"[PATCH] Tc" has no obvious style problems and is ready for submission.

NOTE: Ignored message types: BIT_MACRO C99_COMMENT_TOLERANCE COMMIT_LOG_LONG_LINE COMMIT_MESSAGE COMPLEX_MACRO CONSTANT_CONVERSION CONST_STRUCT EMAIL_SUBJECT FILE_PATH_CHANGES FROM_SIGN_OFF_MISMATCH GIT_COMMIT_ID JIFFIES_COMPARISON LEADING_SPACE LONG_LINE_COMMENT MACRO_ARG_REUSE MACRO_WITH_FLOW_CONTROL MULTISTATEMENT_MACRO_USE_DO_WHILE NOT_UNIFIED_DIFF PRINTK_WITHOUT_KERN_LEVEL TRAILING_SEMICOLON TRAILING_STATEMENTS VOLATILE
=========================================================
Running on changes from local HEAD
=========================================================
All done

But.. that made me realise, there is also this in Makefile.bpf, which we should probably also consolidate.

It seems to me that make (all) is only used as a fast way to check for compilation errors, as neither the loader nor units seem to rely on this. Am I correct?

julianwiedmann · 2025-05-26T12:58:30Z

It seems to me that make (all) is only used as a fast way to check for compilation errors, as neither the loader nor units seem to rely on this. Am I correct?

Have a look at how this is used in CI for build testing (.github/workflows/lint-bpf-checks.yaml, .github/workflows/lint-build-commits.yaml) and verifier testing (test/verifier/verifier_test.go).

Use gnu99 instead of gnu89, to be aligned with BPF unit tests. Signed-off-by: Marc Suñé <marc.sune@isovalent.com>

Remove -Wdeclaration-after-statement from default CLANG flags. Unit test machinery and some tests declare after statement. Signed-off-by: Marc Suñé <marc.sune@isovalent.com>

Prior to this commit BPF unit tests ran with their own set of CFLAGS, (CLANG_FLAGS). They were manually synced to match loader's CFLAGS. This commit reads the golang base CFLAGS definition and uses it as base CFLAGS for the unit tests. Some adjustments will come in upcoming commits. Signed-off-by: Marc Suñé <marc.sune@isovalent.com> Signed-off-by: Alexandre Perrin <alex@isovalent.com>

msune · 2025-05-26T13:06:27Z

Mention in the commit description dropped. Should be ready to merge now.

Have a look at how this is used in CI for build testing (.github/workflows/lint-bpf-checks.yaml, .github/workflows/lint-build-commits.yaml) and verifier testing (test/verifier/verifier_test.go).

ACK. I will align (shared Makefile.clang or alike) in a subsquent PR.

msune · 2025-05-27T12:38:33Z

cc @ti-mo

julianwiedmann · 2025-05-29T14:35:49Z

/test

julianwiedmann · 2025-05-29T14:36:22Z

@cilium/loader I feel this would be a good improvement to merge, wdyt?

msune requested review from a team as code owners May 20, 2025 13:24

msune requested review from ti-mo and julianwiedmann May 20, 2025 13:24

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 20, 2025

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label May 20, 2025

kaworu reviewed May 20, 2025

View reviewed changes

bpf/tests/Makefile Outdated Show resolved Hide resolved

msune force-pushed the cflags_sync_unit_loader branch from 0c8689d to b5b0fb7 Compare May 20, 2025 13:36

joestringer added the release-note/misc This PR makes changes that have no direct user impact. label May 20, 2025

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 20, 2025

julianwiedmann reviewed May 22, 2025

View reviewed changes

bpf/tests/Makefile Show resolved Hide resolved

msune force-pushed the cflags_sync_unit_loader branch 3 times, most recently from 8559b80 to c3c2eb4 Compare May 22, 2025 13:20

msune force-pushed the cflags_sync_unit_loader branch from c3c2eb4 to a2cd19b Compare May 26, 2025 12:33

julianwiedmann approved these changes May 26, 2025

View reviewed changes

msune added 3 commits May 26, 2025 15:04

pkg/datapath/loader: use gnu99 instead of 89

d974d06

Use gnu99 instead of gnu89, to be aligned with BPF unit tests. Signed-off-by: Marc Suñé <marc.sune@isovalent.com>

loader: remove -Wdeclaration-after-statement

1774ba5

Remove -Wdeclaration-after-statement from default CLANG flags. Unit test machinery and some tests declare after statement. Signed-off-by: Marc Suñé <marc.sune@isovalent.com>

msune force-pushed the cflags_sync_unit_loader branch from a2cd19b to 411198c Compare May 26, 2025 13:05

ti-mo enabled auto-merge June 2, 2025 07:32

ti-mo approved these changes Jun 2, 2025

View reviewed changes

ti-mo added this pull request to the merge queue Jun 2, 2025

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 2, 2025

Merged via the queue into cilium:main with commit b3a213b Jun 2, 2025
67 checks passed

msune mentioned this pull request Jun 2, 2025

bpf: unify CFLAGS deduction into a common Makefile.clang #39831

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

bpf|loader: keep CFLAGS (CLANG_FLAGS) in sync between loader and BPF unit tests #39636

bpf|loader: keep CFLAGS (CLANG_FLAGS) in sync between loader and BPF unit tests #39636

Uh oh!

msune commented May 20, 2025

Uh oh!

msune commented May 20, 2025 •

edited

Loading

Uh oh!

Uh oh!

julianwiedmann left a comment

Uh oh!

Uh oh!

sayboras commented May 22, 2025

Uh oh!

msune commented May 22, 2025

Uh oh!

julianwiedmann commented May 26, 2025

Uh oh!

julianwiedmann left a comment

Uh oh!

msune commented May 26, 2025

Uh oh!

julianwiedmann commented May 26, 2025

Uh oh!

msune commented May 26, 2025 •

edited

Loading

Uh oh!

msune commented May 27, 2025

Uh oh!

julianwiedmann commented May 29, 2025

Uh oh!

julianwiedmann commented May 29, 2025

Uh oh!

Uh oh!

Uh oh!

bpf|loader: keep CFLAGS (CLANG_FLAGS) in sync between loader and BPF unit tests #39636

bpf|loader: keep CFLAGS (CLANG_FLAGS) in sync between loader and BPF unit tests #39636

Uh oh!

Conversation

msune commented May 20, 2025

Uh oh!

msune commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

julianwiedmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sayboras commented May 22, 2025

Uh oh!

msune commented May 22, 2025

Uh oh!

julianwiedmann commented May 26, 2025

Uh oh!

julianwiedmann left a comment

Choose a reason for hiding this comment

Uh oh!

msune commented May 26, 2025

Uh oh!

julianwiedmann commented May 26, 2025

Uh oh!

msune commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msune commented May 27, 2025

Uh oh!

julianwiedmann commented May 29, 2025

Uh oh!

julianwiedmann commented May 29, 2025

Uh oh!

Uh oh!

Uh oh!

msune commented May 20, 2025 •

edited

Loading

msune commented May 26, 2025 •

edited

Loading