⭐ Football Update: openzfs/zfs/pull/

robn · 2025-07-18T02:24:32Z

Motivation and Context

After #17488 I spent some time messing with some static analysers. They're a real mixed bag in what they do and don't catch, and I'm not yet sure if I've learned anything new.

Still, they did all find a bunch of places where we initialise or update a variable with a value that we never use. Usually, it's an initialisation at declaration, with an assignment nearer to where we use it. None of these are bad, and dead-store optimisation will usually get rid of them anyway, but it's extra stuff to read and understand and wonder why its there, and that's no fun.

Description

See commits.

How Has This Been Tested?

Compile checked only (on Linux and FreeBSD). I'll be trusting CI to spot any breakages (and be astonished if it does).

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Quality assurance (non-breaking change which makes the code more robust against bugs)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

They aren't used outside these very small blocks, and their initial values are never used at all. Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <robn@despairlabs.com>

Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <robn@despairlabs.com>

It would have been optimised away anyway so it doesn't matter, but it does make things a little tougher to read. Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <robn@despairlabs.com>

These are all cases where we initialise or update a variable, and then never use it. None of them particularly matter, as the compiler should optimise them all away during dead store elimination, but some static analysers complain about them and they are extra work for casual readers to follow, so worth removing. Sponsored-by: https://despairlabs.com/sponsor/ Signed-off-by: Rob Norris <robn@despairlabs.com>

behlendorf

Good stuff.

behlendorf · 2025-07-18T16:48:53Z

I spent some time messing with some static analysers. They're a real mixed bag in what they do and don't catch.

That's definitely been my experience, particularity when kernel code is involved. While I haven't played with it much myself, I have heard good things about smatch which as I understand it was built specifically for the Linux kernel.

amotin

The changes look correct, but I worry that in few places they may depend on compiler/analyzer understanding things like noreturn attribute to not throw an unitialized variable warning. Do we care about compilers that are not the ones used in or CI? If so, few places look questionable to me.

robn · 2025-07-19T06:43:36Z

Which change specifically were you concerned about? I don't remember one around noreturn, but I also didn't follow the trail all the way through.

I'm not sure yet the right thing to do in general. The static analysis we do have (cppcheck) didn't catch #17488 and I've been trying to understand why, and work out if there's a better/different option. I'm not there yet, but while I'm playing I'm taking the opportunity to fix the "obvious" things that come up.

robn · 2025-07-19T06:51:23Z

While I haven't played with it much myself, I have heard good things about smatch which as I understand it was built specifically for the Linux kernel.

Oh interesting, hadn't seen it, will take a look. Thanks!

amotin · 2025-07-21T18:09:32Z

Which change specifically were you concerned about? I don't remember one around noreturn, but I also didn't follow the trail all the way through.

cmd/ztest.c change, for example. It depends on knowledge that usage() will never return.

behlendorf · 2025-07-21T22:55:47Z

Do we care about compilers that are not the ones used in or CI? If so, few places look questionable to me.

Primarily I'm concerned with making sure the default compilers used by the CI builders build cleanly (gcc and clang).

That said, there are a bunch of environments we've wanted to add to the CI to simply verify the code compiles without warnings. Off the top of my head and it no particular order.

cross-compile for aarch64 and possible other architectures.
an alternate libc builder, musl libc
VPATH builder
in-tree kernel builder

robn · 2025-07-22T02:01:08Z

cmd/ztest.c change, for example. It depends on knowledge that usage() will never return.

Right, I see. But in that case, the function is marked noreturn, so I feel like this only makes sense as an argument if there are compilers that will accept __attribute__((noreturn)) but ignore it. Which feels weird/dangerous to me; __attribute__ isn't part of the language so it should outright fail to parse in compilers that don't support it, and istr ignoring noreturn is dangerous in some contexts because the compiler can take some shortcuts on saving registers etc if it knows it won't need them.

You aren't wrong! And I'm not going to push back very hard if you insist, at least not for now. My goal however is to get us to a useful static analyser config that has as many things enabled as possible with no false positives, so if we're not going to be able to use (some) annotations to inform that, then we're gonna have a difficult time I think.

amotin

I won't insist if you think it is OK.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Signed-off-by: Rob Norris <robn@despairlabs.com> Sponsored-by: https://despairlabs.com/sponsor/ Closes #17551

Seems like we haven't set it since the SPL was pulled into the main ZFS tree. In removing the define, I've taken the 64-bit version (ie the one that _hasn't_ been running since back then) because it looks like its closer to the intended width by the way its used. Since the macros ar eno longer needed as a selector, pull those too. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Signed-off-by: Rob Norris <robn@despairlabs.com> Sponsored-by: https://despairlabs.com/sponsor/ Closes #17551

They aren't used outside these very small blocks, and their initial values are never used at all. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Signed-off-by: Rob Norris <robn@despairlabs.com> Sponsored-by: https://despairlabs.com/sponsor/ Closes #17551

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Signed-off-by: Rob Norris <robn@despairlabs.com> Sponsored-by: https://despairlabs.com/sponsor/ Closes #17551

It would have been optimised away anyway so it doesn't matter, but it does make things a little tougher to read. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Signed-off-by: Rob Norris <robn@despairlabs.com> Sponsored-by: https://despairlabs.com/sponsor/ Closes #17551

These are all cases where we initialise or update a variable, and then never use it. None of them particularly matter, as the compiler should optimise them all away during dead store elimination, but some static analysers complain about them and they are extra work for casual readers to follow, so worth removing. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Signed-off-by: Rob Norris <robn@despairlabs.com> Sponsored-by: https://despairlabs.com/sponsor/ Closes #17551