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

Linux: Fix build warnings from LLVM/Clang 15.0.7 #14738

Closed
wants to merge 2 commits into from
Closed

Conversation

ryao
Copy link
Contributor

@ryao ryao commented Apr 11, 2023

Motivation and Context

I recently needed to build a recent kernel on a Ubuntu 18.04 system with sanitizers that required a much more recent compiler than those included with Ubuntu 18.04. The first modern compiler that I got working on it was LLVM/Clang 15.0.7, so I built both the kernel and ZFS with it, only to discover that -Werror causes build failures because of issues that go uncaught because almost nobody builds the Linux kernel modules with LLVM/Clang.

This was done by Klara Systems and sponsored by Wasabi Technology, Inc.

Description

The individual patches have descriptions.

How Has This Been Tested?

Here are some rough instructions describing how I tested this.

I first installed pkgsrc:

cd $HOME
CVS_RSH=ssh cvs -danoncvs@anoncvs.NetBSD.org:/cvsroot checkout -P pkgsrc
cd pkgsrc/bootstrap
./bootstrap --prefer-pkgsrc yes --make-jobs $(nproc)
PATH="/usr/pkg/bin:$PATH"
MANPATH="/usr/pkg/man:$MANPATH"
cd ../lang/clang
sudo bmake install clean

I then built Linux roughly this way:

cd $HOME
git clone https://gitlab.com/linux-kernel/stable.git linux
cd linux
git checkout v6.3-rc6
cp /boot/config-5.3.0-26-generic .config
make CC=clang HOSTCC=clang -j$(nproc) olddefconfig
make CC=clang HOSTCC=clang -j$(nproc)
sudo make CC=clang HOSTCC=clang -j$(nproc) modules_install

For ZFS, I had a local branch that I rebased on master, but the commands to do the same with master are roughly this:

cd $HOME
git clone https://github.com/openzfs/zfs.git
cd zfs
git clean -xdf
./autogen.sh
CC=clang KERNEL_CC=clang ./configure --enable-debug --enable-debuginfo --with-config=all --with-linux="${HOME}/linux" --with-linux-obj="${HOME}/linux"
make -j$(nproc)

The make command will fail without these patches.

I needed to install new firmware for the newer kernel's NIC driver:

cd $HOME
git clone git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
sudo cp linux-firmware/bnx2x/*7.13.21* linux-firmware/bnx2x/*7.13.15* /lib/firmware/bnx2x/

There is a debian bug for that:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1006500

There is a weird bug in the kernel's block device driver for the controller that has the drive with the rootfs where it requires the partitions to be reprobed when kexec booting (it affects older kernels, although I have yet to confirm it also affects the latest), so I installed dracut and modified it to reprobe all devices:

sudo -i
apt install dracut
mkdir /usr/lib/dracut/modules.d/70fix-boot
cat > /usr/lib/dracut/modules.d/70fix-boot/fix-boot.sh << END
#!/bin/sh

. /lib/dracut-lib.sh

modprobe mpt3sas
modprobe smartpqi

sleep 1

for i in /dev/sd[a-z] /dev/sd[a-z][a-z]; do
        blockdev --rereadpt "\${i}"
done;
END

cat > /usr/lib/dracut/modules.d/70fix-boot/module-setup.sh << END
#!/bin/bash

check() {
        return 0
}

# called by dracut
installkernel() {
    :
}

# called by dracut
install() {
    inst_hook pre-udev 10 "\$moddir/fix-boot.sh"
    inst /sbin/blockdev
}
END
chmod u+x /usr/lib/dracut/modules.d/70fix-boot/*
exit

Then I kexec booted into the new kernel after building an initramfs archive:

sudo dracut --early-microcode --force ${HOME}/initrd.img-6.3.0-rc6 6.3.0-rc6

sudo -i kexec --type="bzImage" -l ${HOME}/linux/arch/x86/boot/bzImage --initrd=${HOME}/initrd.img-6.3.0-rc6 --command-line="$(cat /proc/cmdline)" --console-vga --real-mode --noefi

sudo -i systemctl kexec

After reconnecting to the machine over SSH, I was able to load the kernel modules:

ssh ...
cd zfs
sudo ./scripts/zfs.sh -v -r

Then I simply imported the pool and ran a workload on it. Everything worked.

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)
  • 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:

Building with Clang on Linux generates a warning that err could be
uninitialized if mnt_ns is a NULL pointer. However, mnt_ns should never
be NULL, so there is no need to put this behind an if statement.  Taking
it outside of the if statement means that the possibility of err being
uninitialized goes from being always zero in a way that the compiler
could not realize to a way that is always zero in a way that the
compiler can realize.

Sponsored-By: Wasabi Technology, Inc.
Signed-off-by: Richard Yao <richard.yao@klarasystems.com>
Clang points out that there is a comparison against -1, but we cannot
fix it because that is from the kernel headers, which we must support.
We can workaround this by using a pragma.

Sponsored-By: Wasabi Technology, Inc.
Signed-off-by: Richard Yao <richard.yao@klarasystems.com>
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Apr 11, 2023
@ryao
Copy link
Contributor Author

ryao commented Apr 12, 2023

For reference, this is the output from -Wordered-compare-function-pointers:

In file included from /home/ryao/zfs/module/os/linux/zfs/trace.c:52:
In file included from /home/ryao/zfs/include/os/linux/zfs/sys/trace_zil.h:216:
In file included from ./include/trace/define_trace.h:102:
In file included from ./include/trace/trace_events.h:250:
/home/ryao/zfs/include/os/linux/zfs/sys/trace_zil.h:161:6: error: ordered comparison of function pointers ('zil_callback_t' (aka 'void (*)(void *)') and 'zil_callback_t') [-Werror,-Wordered-compare-function-pointers]
            ITX_TP_STRUCT_ENTRY
            ^~~~~~~~~~~~~~~~~~~
/home/ryao/zfs/include/os/linux/zfs/sys/trace_zil.h:101:3: note: expanded from macro 'ITX_TP_STRUCT_ENTRY'
                __field(zil_callback_t, itx_callback)                       \
                ^
./include/trace/stages/stage4_event_fields.h:20:29: note: expanded from macro '__field'
#define __field(type, item)     __field_ext(type, item, FILTER_OTHER)
                                ^
./include/trace/stages/stage4_event_fields.h:11:15: note: expanded from macro '__field_ext'
        .is_signed = is_signed_type(_type), .filter_type = _filter_type },
                     ^
./include/linux/compiler.h:238:44: note: expanded from macro 'is_signed_type'
#define is_signed_type(type) (((type)(-1)) < (__force type)1)
                                           ^
./include/trace/stages/stage1_struct_define.h:60:35: note: expanded from macro 'TP_STRUCT__entry'
#define TP_STRUCT__entry(args...) args
                                  ^
./include/trace/trace_events.h:244:2: note: expanded from macro 'DECLARE_EVENT_CLASS'
        tstruct                                                         \
        ^~~~~~~
1 error generated.

Linux's tracepoint headers do a compile time check to see if a type is signed or unsigned, and this trips the compiler diagnostic.

behlendorf pushed a commit that referenced this pull request Apr 20, 2023
Clang points out that there is a comparison against -1, but we cannot
fix it because that is from the kernel headers, which we must support.
We can workaround this by using a pragma.

Sponsored-By: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Youzhong Yang <yyang@mathworks.com>
Signed-off-by: Richard Yao <richard.yao@klarasystems.com>
Closes #14738
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Apr 20, 2023
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request May 1, 2023
Building with Clang on Linux generates a warning that err could be
uninitialized if mnt_ns is a NULL pointer. However, mnt_ns should never
be NULL, so there is no need to put this behind an if statement.  Taking
it outside of the if statement means that the possibility of err being
uninitialized goes from being always zero in a way that the compiler
could not realize to a way that is always zero in a way that the
compiler can realize.

Sponsored-By: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Youzhong Yang <yyang@mathworks.com>
Signed-off-by: Richard Yao <richard.yao@klarasystems.com>
Closes openzfs#14738
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request May 1, 2023
Clang points out that there is a comparison against -1, but we cannot
fix it because that is from the kernel headers, which we must support.
We can workaround this by using a pragma.

Sponsored-By: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Youzhong Yang <yyang@mathworks.com>
Signed-off-by: Richard Yao <richard.yao@klarasystems.com>
Closes openzfs#14738
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants