Clang inspired fixes, add clang CI permutation #172

evelikov · 2024-10-03T16:06:43Z

This series fixes all the issues spotted by clang (+ be that clang itself, scan-build or cland-tidy) and adds it to CI.

At a glance - from the 5 distros we have 4 major clang versions and as indicated in the patches, there's some value in having them.

Understandably, the CI matrix is becoming a bit of a zoo... Although half of it will be gone with the drop of autotools, soon (tm) 😅

Plus since they're all parallel jobs, I think we can bare with it.

evelikov · 2024-10-03T16:37:07Z

Whelp seems like sanitizers and clang don't play well together... I'm assuming gcc uses static sanitizer libs while clang shared ones, or vice versa.

Flipping to draft, feel free to review and comment though.

tools/insmod.c

libkmod/libkmod-index.c

libkmod/libkmod.c

Required for puts(), reported by clang-tidy. Fixes: b644b8d ("tools: add kmod_version() helper") Signed-off-by: Emil Velikov <[email protected]> Link: #172 Signed-off-by: Lucas De Marchi <[email protected]>

Required for size_t, reported by clang-tidy. Fixes: 38943b2 ("shared: use size_t for strbuf") Signed-off-by: Emil Velikov <[email protected]> Link: #172 Signed-off-by: Lucas De Marchi <[email protected]>

Initialize the variable, otherwise we'll get (random) stack value if the user hasn't provided --syslog. Reported by scan-build. Fixes: ca8f04e ("tools/insmod: add syslog and verbose options") Signed-off-by: Emil Velikov <[email protected]> Link: #172 Signed-off-by: Lucas De Marchi <[email protected]>

Initialize the variable, otherwise we'll get (random) stack value if the user hasn't provided --syslog. Reported by scan-build. Fixes: 24fe68d ("tools/rmmod: make opt variables non-global") Signed-off-by: Emil Velikov <[email protected]> Link: #172 Signed-off-by: Lucas De Marchi <[email protected]>

Initialize the variable, otherwise we'll get (random) stack value if the user hasn't provided --syslog. Reported by scan-build. Fixes: 6313d40 ("tools/lsmod: add basic opts like rmmod") Signed-off-by: Emil Velikov <[email protected]> Link: #172 Signed-off-by: Lucas De Marchi <[email protected]>

Older versions of clang (14.0.0 as seen in Ubuntu 22.04) will flag a warning that we're using unsigned int, with a unsigned char modifier. Newer versions like 16.0.6 (Debian unstable), 17.0.6 (Alpine), etc do not flag this as an issue. In practise the number fits in the latter range, so just add a cast to silence the warning. Signed-off-by: Emil Velikov <[email protected]> Link: #172 Signed-off-by: Lucas De Marchi <[email protected]>

Clang will detect that the enum cannot be zero, thus triggering a warning. Since this is an external (public API) function, the end-user can provide any input so we want to keep the check. Note: include the pragmas in a if defined(__clang__) guard, otherwise we'll trigger -Wunknown_pragma which will become an error in CI and developer builds. Signed-off-by: Emil Velikov <[email protected]> Link: #172 Signed-off-by: Lucas De Marchi <[email protected]>

There was some recent snafu which meant the container wasn't available. With that resolved, we can drop our hack and use the correct container directly. Signed-off-by: Emil Velikov <[email protected]> Link: #172 Signed-off-by: Lucas De Marchi <[email protected]>

I'm about to add some more and the current (almost sorted) state breaks my brain :-x Signed-off-by: Emil Velikov <[email protected]> Link: #172 Signed-off-by: Lucas De Marchi <[email protected]>

lucasdemarchi · 2024-10-09T04:22:26Z

Applied most of the patches, leaving some comments in others. Thanks

evelikov · 2024-10-09T17:20:48Z

Updated the PR dropping the already merged changes. In addition we have:

use single variable for insmod control flow
tweak the rmmod code flow
rerun the tests in verbose, if they fail
properly resolve the GCC/ASAN ordering issue - @stoeckmann please check it over
fix the UB issues reported by clang's sanitizer
- alternative suggestions for "adding zero to a NULL pointer" are more then welcome

CI now passes all permutations apart from Fedora 40 and Ubuntu 22.04:

F40, package compiler-rt should include the files although they're nowhere to be found - either find or clang -print-file-name (see the Set the environment section)
U22.04, the files are there according to find and clang ... with all their dependencies resolved, yet it fails with "No such file or directory"

As you can see I didn't succeed in excluding them - web linter complains matrix.exclude is unknown key... Then again it also complains for matrix.container -> Matrix must only contain primitive types 🤯

Any tips would be appreciated. In the meanwhile feel free to grab whatever makes sense.

libkmod/libkmod-index.c

lucasdemarchi · 2024-10-15T17:48:54Z

Is this still a draft?

evelikov · 2024-10-15T18:27:51Z

Is this still a draft?

Since it's getting really hairy, I've split out the prep patches into separate PRs.

As those land, I will respin the final patches and undraft.

evelikov · 2024-10-17T21:36:20Z

The newly enabled clang build already flagged another few issues (alongside the ELFDBG) 🎉

In addition we have:

temporary disabled Fedora + clang - the sanitizer shared lib is missing
move sanitizer enablement to build-dev.ini
convert the test wrapper to "sanitizer-env.sh" setup script
updated make references in testsuite code and README

Last two are from #172 ... With the final outstanding piece (add test toggle --disable-sanitizer) which doesn't seem doable ATM.

The recently added "always build ELFDBG" patch is already paying dividends... Clang is flagging a "fmt" is not literal warning. That's clearly wrong, although without _printf_format_ clang was struggling to figure things out. With the attribute, it helpfully flagged that handful of the modifiers are wrong. Signed-off-by: Emil Velikov <[email protected]>

Convert the existing wrapper script, into one that we source to set the environment aka LD_PRELOAD. Thus a developer can, use/test/debug the tests without using meson. Namely: - source scripts/sanitizer-env.sh - build/testsuite/test-depmod Signed-off-by: Emil Velikov <[email protected]>

With the sanitizers supported in both gcc + clang and all the issues resolved, let's enable them via build-dev.ini. This means, developers will see any issues OOTB without having to run through the CI. As a nice bonus, let's re-enable them for ubuntu:22.04 - it should be working fine now. Signed-off-by: Emil Velikov <[email protected]>

evelikov · 2024-10-17T21:52:49Z

Masking Fedora + clang, nuked clang all together 🤯 Converted to keep Fedora gcc/clang yet disable the sanitizers.

Plus some last second commit reshuffle, for extra fun 😂

scripts/sanitizer-env.sh

.github/actions/setup-alpine/action.yml

testsuite/testsuite.c

By default clang uses static sanitizer libraries, which causes build and test-time failures. Swap for the shared libasan which resolves both. Note: meson tries to be helpful here, throwing a warning that we should use -D b_lundef=false which is incorrect in our case. Signed-off-by: Emil Velikov <[email protected]>

With the clang issues resolved, let's add it to the CI matrix so fewer issues get it. Note: Fedora 40 doesn't ship the shared sanitizer library, while older Fedora versions did. Fedora 41 will be coming with LLVM 19, which will have the binary (seemingly with different name 🤦). Let's leave the Fedora/clang infra in and just mask it out sanitizers for the next month or so, until the new version comes out. Then we can re-evaluate. Signed-off-by: Emil Velikov <[email protected]>

Signed-off-by: Emil Velikov <[email protected]>

Mention that running outside of `meson test", one needs to set the env. aka "source scripts/sanitizer-env.sh". Also gently discourage using sanitizers alongside gdb and strace. While thing might work it's not the setup we want to support, I think. Signed-off-by: Emil Velikov <[email protected]>

We have added meson recently, with the goal to remove the make/autotools one in a release or two. Update the reference to the meson equivalent. Signed-off-by: Emil Velikov <[email protected]>

The recently added "always build ELFDBG" patch is already paying dividends... Clang is flagging a "fmt" is not literal warning. That's clearly wrong, although without _printf_format_ clang was struggling to figure things out. With the attribute, it helpfully flagged that handful of the modifiers are wrong. Signed-off-by: Emil Velikov <[email protected]> Link: #172 Signed-off-by: Lucas De Marchi <[email protected]>

Convert the existing wrapper script, into one that we source to set the environment aka LD_PRELOAD. Thus a developer can, use/test/debug the tests without using meson. Namely: - source scripts/sanitizer-env.sh - build/testsuite/test-depmod Signed-off-by: Emil Velikov <[email protected]> Link: #172 Signed-off-by: Lucas De Marchi <[email protected]>

With the sanitizers supported in both gcc + clang and all the issues resolved, let's enable them via build-dev.ini. This means, developers will see any issues OOTB without having to run through the CI. As a nice bonus, let's re-enable them for ubuntu:22.04 - it should be working fine now. Signed-off-by: Emil Velikov <[email protected]> Link: #172 Signed-off-by: Lucas De Marchi <[email protected]>

By default clang uses static sanitizer libraries, which causes build and test-time failures. Swap for the shared libasan which resolves both. Note: meson tries to be helpful here, throwing a warning that we should use -D b_lundef=false which is incorrect in our case. Signed-off-by: Emil Velikov <[email protected]> Link: #172 Signed-off-by: Lucas De Marchi <[email protected]>

With the clang issues resolved, let's add it to the CI matrix so fewer issues get it. Note: Fedora 40 doesn't ship the shared sanitizer library, while older Fedora versions did. Fedora 41 will be coming with LLVM 19, which will have the binary (seemingly with different name 🤦). Let's leave the Fedora/clang infra in and just mask it out sanitizers for the next month or so, until the new version comes out. Then we can re-evaluate. Signed-off-by: Emil Velikov <[email protected]> Link: #172 Signed-off-by: Lucas De Marchi <[email protected]>

Signed-off-by: Emil Velikov <[email protected]> Link: #172 Signed-off-by: Lucas De Marchi <[email protected]>

Mention that running outside of `meson test", one needs to set the env. aka "source scripts/sanitizer-env.sh". Also gently discourage using sanitizers alongside gdb and strace. While thing might work it's not the setup we want to support, I think. Signed-off-by: Emil Velikov <[email protected]> Link: #172 Signed-off-by: Lucas De Marchi <[email protected]>

We have added meson recently, with the goal to remove the make/autotools one in a release or two. Update the reference to the meson equivalent. Signed-off-by: Emil Velikov <[email protected]> Link: #172 Signed-off-by: Lucas De Marchi <[email protected]>

lucasdemarchi · 2024-10-18T18:30:00Z

I did a small fixup in testsuite/README and applied. Thanks!

evelikov marked this pull request as draft October 3, 2024 16:37

lucasdemarchi reviewed Oct 9, 2024

View reviewed changes

tools/insmod.c Outdated Show resolved Hide resolved

libkmod/libkmod-index.c Outdated Show resolved Hide resolved

lucasdemarchi reviewed Oct 9, 2024

View reviewed changes

libkmod/libkmod.c Outdated Show resolved Hide resolved

evelikov force-pushed the clang-inspired-fixes branch from 7617ce7 to e8d1628 Compare October 9, 2024 17:03

stoeckmann reviewed Oct 11, 2024

View reviewed changes

libkmod/libkmod-index.c Outdated Show resolved Hide resolved

evelikov force-pushed the clang-inspired-fixes branch from e8d1628 to 0e3493f Compare October 11, 2024 14:40

This was referenced Oct 15, 2024

insmod: Fix exit code regression #178

Closed

Fix GCC sanitizer load ordering #179

Closed

Clang all the warnings #180

Closed

evelikov force-pushed the clang-inspired-fixes branch 2 times, most recently from 31ad907 to 497a04a Compare October 17, 2024 21:30

evelikov marked this pull request as ready for review October 17, 2024 21:36

evelikov added 3 commits October 17, 2024 22:41

evelikov force-pushed the clang-inspired-fixes branch from 497a04a to c89d457 Compare October 17, 2024 21:49

lucasdemarchi reviewed Oct 18, 2024

View reviewed changes

scripts/sanitizer-env.sh Outdated Show resolved Hide resolved

lucasdemarchi reviewed Oct 18, 2024

View reviewed changes

.github/actions/setup-alpine/action.yml Show resolved Hide resolved

lucasdemarchi reviewed Oct 18, 2024

View reviewed changes

testsuite/testsuite.c Show resolved Hide resolved

evelikov added 4 commits October 18, 2024 18:06

ci: sort the distribution listing

5d3b09b

Signed-off-by: Emil Velikov <[email protected]>

evelikov force-pushed the clang-inspired-fixes branch from c89d457 to 86f2f91 Compare October 18, 2024 17:22

testsuite: update "make rootfs" error message

154fe19

We have added meson recently, with the goal to remove the make/autotools one in a release or two. Update the reference to the meson equivalent. Signed-off-by: Emil Velikov <[email protected]>

evelikov force-pushed the clang-inspired-fixes branch from 86f2f91 to 154fe19 Compare October 18, 2024 17:27

lucasdemarchi pushed a commit that referenced this pull request Oct 18, 2024

ci: sort the distribution listing

2758cb5

Signed-off-by: Emil Velikov <[email protected]> Link: #172 Signed-off-by: Lucas De Marchi <[email protected]>

lucasdemarchi closed this Oct 18, 2024

evelikov deleted the clang-inspired-fixes branch October 18, 2024 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clang inspired fixes, add clang CI permutation #172

Clang inspired fixes, add clang CI permutation #172

evelikov commented Oct 3, 2024

evelikov commented Oct 3, 2024

lucasdemarchi commented Oct 9, 2024

evelikov commented Oct 9, 2024

lucasdemarchi commented Oct 15, 2024

evelikov commented Oct 15, 2024

evelikov commented Oct 17, 2024

evelikov commented Oct 17, 2024

lucasdemarchi commented Oct 18, 2024

Clang inspired fixes, add clang CI permutation #172

Clang inspired fixes, add clang CI permutation #172

Conversation

evelikov commented Oct 3, 2024

evelikov commented Oct 3, 2024

lucasdemarchi commented Oct 9, 2024

evelikov commented Oct 9, 2024

lucasdemarchi commented Oct 15, 2024

evelikov commented Oct 15, 2024

evelikov commented Oct 17, 2024

evelikov commented Oct 17, 2024

lucasdemarchi commented Oct 18, 2024