Skip to content

bpf: Avoid implicit shorten-64-to-32 in clang 19#36186

Merged
sayboras merged 1 commit intomainfrom
pr/tammach/clang-19
Dec 4, 2024
Merged

bpf: Avoid implicit shorten-64-to-32 in clang 19#36186
sayboras merged 1 commit intomainfrom
pr/tammach/clang-19

Conversation

@sayboras
Copy link
Copy Markdown
Member

@sayboras sayboras commented Nov 26, 2024 •
edited
Loading

As per the below PR, clang 19.1.x is having -Wshorten-64-to32 under the -Wimplicit-int-conversion, which is already forbidden in Makefile.bpf. This commit is to avoid the conversion error as per below sample

./lib/nodeport.h:1825:18: error: implicit conversion loses integer precision: '__u64' (aka 'unsigned long long') to '__s32' (aka 'int') [-Werror,-Wshorten-64-to-32]
 1825 |         __s32 len_old = ctx_full_len(ctx);

Relates: llvm/llvm-project#80814
Fixes: #35162 35162

@maintainer-s-little-helper 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 Nov 26, 2024
@sayboras sayboras added the release-note/misc This PR makes changes that have no direct user impact. label Nov 26, 2024
@maintainer-s-little-helper 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 Nov 26, 2024
@sayboras sayboras force-pushed the pr/tammach/clang-19 branch 2 times, most recently from 2a8187a to 8d9faf7 Compare November 26, 2024 13:57
@sayboras
Copy link
Copy Markdown
Member Author

sayboras commented Nov 26, 2024

/test

@sayboras sayboras marked this pull request as ready for review November 26, 2024 14:58
@sayboras sayboras requested a review from a team as a code owner November 26, 2024 14:58
@sayboras sayboras requested a review from jibi November 26, 2024 14:58
@julianwiedmann julianwiedmann added area/llvm Requires upstream work in LLVM. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Nov 26, 2024
@gentoo-root
Copy link
Copy Markdown
Contributor

gentoo-root commented Nov 26, 2024

I'll do a more careful review of the converted places later, but first a question (trying to gather the context): since we aren't on LLVM 19 yet, are you testing it by just adding -Wshorten-64-to32, or are you testing it by actually upgrading LLVM? Are you planning further work related to the LLVM upgrade?

@sayboras
Copy link
Copy Markdown
Member Author

sayboras commented Nov 26, 2024

are you testing it by just adding -Wshorten-64-to32, or are you testing it by actually upgrading LLVM? Are you planning further work related to the LLVM upgrade?

In clang 19, -Wshorten-64-to32 is enabled as part of -Wimplicit-int-conversion, so my local clang 19 complication just failed. This is mainly to fix my local dev, however, I am keen to work on llvm upgrade in near future as well.

PS: I saw that you have the clang 18 upgrade merged in image-tools 🎖️

@gentoo-root
Copy link
Copy Markdown
Contributor

gentoo-root commented Nov 26, 2024

Thanks for the clarification! Let's coordinate our work on LLVM upgrades, so that it doesn't clash.

I have a tracking issue here #32801 with the checklist of everything that needs to be done for the complete upgrade. It will be converted into an issue template in the future.

As you said, the 18.1.8 upgrade has been merged to image-tools, but that's merely the first step, and I'll proceed with the other steps (i.e. do the actual switch to the new version and fix any new errors) when I have more cycles. That means, Cilium is still on LLVM 17 right now (previously, there was an attempt for 18.1.6, which was reverted due to waiting too long for another issue that blocked kernel upgrades in the CI.)

So, my plan was to proceed with the upgrade to 18.1.8, and once it's done start the new iteration for the latest stable version at that moment. If you are planning to jump to 19 faster, you can go ahead, but I suggest using the same issue template and the same process for convenience and coordination. And, of course, you can merge fixes for future LLVM versions at any time — the checklist is for the upgrade itself ;)

@sayboras
Copy link
Copy Markdown
Member Author

sayboras commented Nov 28, 2024 •
edited
Loading

Thanks to your awesome work, the main branch is with llvm 18 now (#36197).

I think we can slowly proceed with fixes for future LLVM versions now.

@sayboras sayboras force-pushed the pr/tammach/clang-19 branch from 8d9faf7 to ebcb886 Compare November 28, 2024 00:23
@sayboras
Copy link
Copy Markdown
Member Author

sayboras commented Nov 28, 2024

/test

@jibi jibi removed their request for review November 28, 2024 15:29
@sayboras sayboras mentioned this pull request Nov 30, 2024
Closed
3 tasks
ldelossa
ldelossa approved these changes Dec 3, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

Running into this with latest Fedora builds.

I tested building and running this commit. Looks good to me.

@sayboras sayboras force-pushed the pr/tammach/clang-19 branch from ebcb886 to eab2b77 Compare December 4, 2024 00:07
@sayboras
bpf: Avoid implicit shorten-64-to-32 in clang 19
6cea350 
As per the below PR, clang 19.1.x is having -Wshorten-64-to32 under the
-Wimplicit-int-conversion, which is already forbidden in Makefile.bpf.
This commit is to avoid the conversion error as per below sample

```
./lib/nodeport.h:1825:18: error: implicit conversion loses integer precision: '__u64' (aka 'unsigned long long') to '__s32' (aka 'int') [-Werror,-Wshorten-64-to-32]
 1825 |         __s32 len_old = ctx_full_len(ctx);
```

Relates: llvm/llvm-project#80814
Signed-off-by: Tam Mach <[email protected]>
@sayboras sayboras force-pushed the pr/tammach/clang-19 branch from eab2b77 to 6cea350 Compare December 4, 2024 00:11
@sayboras
Copy link
Copy Markdown
Member Author

sayboras commented Dec 4, 2024

/test

@sayboras sayboras enabled auto-merge December 4, 2024 00:12
@sayboras sayboras added this pull request to the merge queue Dec 4, 2024
Merged via the queue into main with commit 79b4dd9 Dec 4, 2024
@sayboras sayboras deleted the pr/tammach/clang-19 branch December 4, 2024 02:34
@cilium-release-bot cilium-release-bot bot mentioned this pull request Dec 18, 2024
Merged
sayboras added a commit that referenced this pull request May 20, 2025
@sayboras
images: Bump llvm image to 19.1.7
9a330d6 
Relates: #36186

Signed-off-by: Tam Mach <[email protected]>
@sayboras sayboras mentioned this pull request May 20, 2025
Merged
sayboras added a commit that referenced this pull request May 20, 2025
@sayboras
bpf: Avoid implicit shorten-64-to-32 in clang 19
a8d2b67 
```
bpf_nat_tests.c:1141:10: error: implicit conversion loses integer precision: 'long' to '__u32' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
 1141 |         iters = loop(SNAT_TEST_ITERATIONS, snat_callback_tcp, &cb_ctx, 0);
      |               ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
bpf_nat_tests.c:1272:10: error: implicit conversion loses integer precision: 'long' to '__u32' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
 1272 |         iters = loop(SNAT_TEST_ITERATIONS, snat_callback_udp, &cb_ctx, 0);
      |               ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```

Relates: #36186
Signed-off-by: Tam Mach <[email protected]>
sayboras added a commit that referenced this pull request May 20, 2025
@sayboras
images: Bump llvm image to 19.1.7
7e69f7e 
Relates: #36186

Signed-off-by: Tam Mach <[email protected]>
sayboras added a commit that referenced this pull request May 20, 2025
@sayboras
images: Bump llvm image to 19.1.7
66ccecd 
Relates: #36186

Signed-off-by: Tam Mach <[email protected]>
sayboras added a commit that referenced this pull request May 20, 2025
@sayboras
images: Bump llvm image to 19.1.7
2a2f216 
Relates: #36186

Signed-off-by: Tam Mach <[email protected]>
sayboras added a commit that referenced this pull request May 20, 2025
@sayboras
bpf: Avoid implicit shorten-64-to-32 in clang 19
ad8d5d4 
```
bpf_nat_tests.c:1141:10: error: implicit conversion loses integer precision: 'long' to '__u32' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
 1141 |         iters = loop(SNAT_TEST_ITERATIONS, snat_callback_tcp, &cb_ctx, 0);
      |               ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
bpf_nat_tests.c:1272:10: error: implicit conversion loses integer precision: 'long' to '__u32' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
 1272 |         iters = loop(SNAT_TEST_ITERATIONS, snat_callback_udp, &cb_ctx, 0);
      |               ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```

Relates: #36186
Signed-off-by: Tam Mach <[email protected]>
sayboras added a commit that referenced this pull request May 20, 2025
@sayboras
images: Bump llvm image to 19.1.7
82e2559 
Relates: #36186

Signed-off-by: Tam Mach <[email protected]>
sayboras added a commit that referenced this pull request May 21, 2025
@sayboras
bpf: Avoid implicit shorten-64-to-32 in clang 19
9da00a4 
```
bpf_nat_tests.c:1141:10: error: implicit conversion loses integer precision: 'long' to '__u32' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
 1141 |         iters = loop(SNAT_TEST_ITERATIONS, snat_callback_tcp, &cb_ctx, 0);
      |               ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
bpf_nat_tests.c:1272:10: error: implicit conversion loses integer precision: 'long' to '__u32' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
 1272 |         iters = loop(SNAT_TEST_ITERATIONS, snat_callback_udp, &cb_ctx, 0);
      |               ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```

Relates: #36186
Signed-off-by: Tam Mach <[email protected]>
sayboras added a commit that referenced this pull request May 21, 2025
@sayboras
images: Bump llvm image to 19.1.7
94d26ae 
Relates: #36186

Signed-off-by: Tam Mach <[email protected]>
sayboras added a commit that referenced this pull request May 21, 2025
@sayboras
images: Bump llvm image to 19.1.7
10abf85 
Relates: #36186

Signed-off-by: Tam Mach <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request May 21, 2025
@sayboras
bpf: Avoid implicit shorten-64-to-32 in clang 19
fb6d2ce 
```
bpf_nat_tests.c:1141:10: error: implicit conversion loses integer precision: 'long' to '__u32' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
 1141 |         iters = loop(SNAT_TEST_ITERATIONS, snat_callback_tcp, &cb_ctx, 0);
      |               ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
bpf_nat_tests.c:1272:10: error: implicit conversion loses integer precision: 'long' to '__u32' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
 1272 |         iters = loop(SNAT_TEST_ITERATIONS, snat_callback_udp, &cb_ctx, 0);
      |               ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```

Relates: #36186
Signed-off-by: Tam Mach <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request May 21, 2025
@sayboras
images: Bump llvm image to 19.1.7
b335f1a 
Relates: #36186

Signed-off-by: Tam Mach <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ldelossa ldelossa ldelossa approved these changes

@gentoo-root gentoo-root Awaiting requested review from gentoo-root

Assignees

No one assigned

Labels

area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/llvm Requires upstream work in LLVM. release-note/misc This PR makes changes that have no direct user impact.

Projects

None yet

Milestone

No milestone

4 participants

@sayboras @gentoo-root @ldelossa @julianwiedmann