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

Automatically detecting compiler & linker used by kernel #298

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

xuzhen
Copy link
Collaborator

@xuzhen xuzhen commented Jan 25, 2023

Automatically choose the specific version of the compiler and linker used to compile the kernel

@xuzhen xuzhen changed the title Automatically detecting compiler & linker used by kernel (WIP) Automatically detecting compiler & linker used by kernel Jan 26, 2023
@xuzhen xuzhen changed the title (WIP) Automatically detecting compiler & linker used by kernel Automatically detecting compiler & linker used by kernel Jan 26, 2023
@scaronni
Copy link
Collaborator

scaronni commented Feb 3, 2023

Nice, when you think it's ready we can merge. Thanks!

@xuzhen
Copy link
Collaborator Author

xuzhen commented Feb 3, 2023

It's ready. I originally wanted to support the icc compiler too, but found that the classic icc is already deprecated and will be removed within this year, and the kernel also intends to drop its support.

@evelikov
Copy link
Collaborator

evelikov commented Feb 7, 2023

Crazy idea - instead of going great lengths to add auto-detection of the compiler and linker, would it make more sense to allow people override it via a configuration file - be that the dkms.conf or per-module one?

Edit: Motivation being is that the heuristics or their ordering even can vary from one distribution to another. Even for seemingly close forks/derivatives (say Mint, Ubuntu, Debian or Devuan) - that can be significant.

Moving that to a config will scale better by shifting the focus to the respective distribution to manage their own platform quirks. It won't affect dkms maintenance nor execution - no point in checking through all the quirks if your system is vanilla.

@xuzhen
Copy link
Collaborator Author

xuzhen commented Feb 8, 2023

This patch is mainly for the convenience of those who have kernels compiled with different compilers, like the one in #295. They don't need to create different config files for each kernel or compiler.

Edit: Motivation being is that the heuristics or their ordering even can vary from one distribution to another. Even for seemingly close forks/derivatives (say Mint, Ubuntu, Debian or Devuan) - that can be significant.

This patch mainly depends on CONFIG_CC_IS_XX and CONFIG_XX_VERSION in the kconfig file. Both of them were introduced with kernel 4.18, so the patch should meet the needs of most people today. Other detecting code is only for backward compatibility.

Moving that to a config will scale better by shifting the focus to the respective distribution to manage their own platform quirks. It won't affect dkms maintenance nor execution - no point in checking through all the quirks if your system is vanilla.

This patch doesn't rely on many distribution-specific quirks. If the kernel is compiled by gcc version a.b.c or clang version x.y.z, it only requires the presence of a executable named gcc-a.b.c / gcc-a.b / gcc-a or clang-x.y.z / clang-x.y / clang-x. If none of those files exist, then fall back to using gcc or clang without a version number. Even for distributions that do not adopt this "compiler-version" naming scheme, users still can let dkms choose the correct compiler by just creating a symbolic link or a wrapper script, as what I suggested in #299.

@evelikov
Copy link
Collaborator

evelikov commented Feb 8, 2023

If I understand correctly the link/wrapper doesn't scale when using autoinstall. Particularly people will have various kernels each compiled with a different compilers/version, so a single wrapper doesn't sound feasible.

Am I missing something?

@xuzhen
Copy link
Collaborator Author

xuzhen commented Feb 8, 2023

If I understand correctly the link/wrapper doesn't scale when using autoinstall.

In fact, the current autoinstall() implementation only supports building modules for one kernel, either the current kernel or the one specified by the command line argument -k.

If using dkms autoinstall --all, dkms will fail to build any modules even without this patch. This is because the autoinstall action dose not provide the module name & version to dkms, and dkms need these information to find kernels.

dkms/dkms.in

Lines 259 to 273 in ce3e217

setup_kernels_arches()
{
# If all is set, use dkms status to fill the arrays
if [[ $all && $1 != status ]]; then
local i=0
while read line; do
line=${line#*/}; line=${line#*/};
# (I would leave out the delimiters in the status output
# in the first place.)
kernelver[$i]=${line%/*}
arch[$i]=${line#*/}
i=$(($i + 1))
done < <(module_status_built "$module" "$module_version")
fi

This is another bug worth a separate pull request IMO.

@xuzhen
Copy link
Collaborator Author

xuzhen commented Feb 8, 2023

Particularly people will have various kernels each compiled with a different compilers/version, so a single wrapper doesn't sound feasible.

Yes, the user may need to create multiple link/wrapper, each one for a specified version of a compiler/linker. But because the new version of the compiler is usually released less frequently than the kernel, so I think it is more convenient to create link/wrapper for the compiler version than for the kernel version for people who have such a need.

@@ -2304,7 +2395,7 @@ PATH="$PATH:/usr/lib/dkms"
umask 022

# Unset environment variables that may interfere with the build
unset CC CXX CFLAGS CXXFLAGS LDFLAGS
unset CC CXX LD CFLAGS CXXFLAGS LDFLAGS KERNEL_CC KERNEL_LD
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAICT KERNEL_CC KERNEL_LD are only local, so I would drop them from here.

run_with_expected_output sh -c "modinfo /lib/modules/${KERNEL_VER}/${expected_dest_loc}/dkms_test.ko${mod_compression_ext} | head -n 4" << EOF
filename: /lib/modules/${KERNEL_VER}/${expected_dest_loc}/dkms_test.ko${mod_compression_ext}
version: 1.0
run_with_expected_output sh -c "modinfo /lib/modules/${KERNEL_VER}/${expected_dest_loc}/dkms_test.ko${mod_compression_ext} | head -n 4 | sort" << EOF
Copy link
Collaborator

@evelikov evelikov Mar 12, 2023

Choose a reason for hiding this comment

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

On which platforms (distro, version, kmod version) do we need the sort? I can send a fix to kmod, if I know a bit more.

But realistically: if we need the sort, it should happen before the headm and ideally it'll be applied to all modinfo invocations.

Edit: please split it into it's own MR (alongside the other suggestions coming shortly), so we can land that ASAP.

fi
else
echo "CONFIG_MODULE_SIG_HASH=\"${ALTER_HASH}\"" > /tmp/dkms_test_kconfig
fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

With my MR in mind #312, I think we can split this (+ KCONFIG above) into it's own patch and simplify:

  • assume KCONFIG is always present - error otherwise
  • change, do not set CONFIG_MODULE_SIG_HASH

strategy:
matrix:
version:
- 22.04
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add 20.04 into the mix as well. How long are the run times?

sed -i 's/^CONFIG_LOCALVERSION=.*$/CONFIG_LOCALVERSION="-clang14"/' .config
yes $'\n' | make LLVM=-14
sudo make modules_install
sudo make install
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move this to a .github/workflow/tiny-clang.sh script and use a simple loop. Something like the following (untested)

#!/bin/bash

declare -A llvm_sha={
 [11]="1",
 [14]="512",
)

for ver in "${!llvm_sha[@]}"; do
        sha=${llvm_sha[${ver}]

        make clean
        make LLVM=-${ver} tinyconfig
        echo "CONFIG_MODULES=y" >> .config
        echo "CONFIG_MODULE_SIG=y" >> .config
        echo "CONFIG_MODULE_SIG_SHA${sha}=y" >> .config
        echo "CONFIG_MODULE_SIG_HASH=\"sha${sha}\"" >> .config
        sed -i 's/^CONFIG_LOCALVERSION=.*$/CONFIG_LOCALVERSION="-clang${ver}"/' .config
        yes $'\n' | make LLVM=-${ver}
        sudo make modules_install
        sudo make install
done

"$1-$(printf "%d.%d" "$2" "$3")"
"$1-$(printf "%d" "$2")"
"$1-$((10#$2)).$((10#$3))"
"$1-$((10#$2))"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please squash this with the respective commit and add an inline comment or two.
Something like "Zero extend (or zero prefix perhaps?) the respective versions to avoid issues like XXX being detected as YYY"

@@ -833,6 +816,90 @@ run_build_script() {
fi
}

# helper functions for getting cc/ld used by kernel
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mind adding a bit more context here;
Create an array of candidates, first one having the complete version suffixed and strip numbers until we find a match.

fi
fi
done
return 1
Copy link
Collaborator

@evelikov evelikov Mar 12, 2023

Choose a reason for hiding this comment

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

Do we need to read the binary, if the KCONFIG is always available?

Edit: since we always add a KCONFIG in the tests, the codepath is not covered in CI. So let's drop this, if the existing code does not work then we can worry about it.

if [[ -z "${ver_full}" ]] || [[ "${ver_full}" = 0 ]]; then
return 1
fi
local ver_major="${ver_full:0: -4}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add a couple of examples for illustration purposes - one GCC and one CLANG. For example my system has CONFIG_GCC_VERSION=60300
Looking at the code below - seems like ver_patch is always set so one can drop the explicit check if [[ "$4" ]] in detect_toolchain_version, right?

local ver_minor="${ver_full: -4:2}"
local ver_patch="${ver_full: -2}"
detect_toolchain_version "$2" "${ver_major}" "${ver_minor}" "${ver_patch}"
return $?
Copy link
Collaborator

Choose a reason for hiding this comment

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

The trailing return $? should not be needed.

KERNEL_CC="$(detect_toolchain_from_vmlinux GCC gcc || \
detect_toolchain_from_vmlinux clang clang)"
fi
if [[ "$KERNEL_CC" ]]; then
Copy link
Collaborator

@evelikov evelikov Mar 12, 2023

Choose a reason for hiding this comment

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

Since KERNEL_CC and KERNEL_LD are local, so let's use local KERNEL_CC... in this function.

Both variables were exported with #294, although searching through the Debian patches/tooling does not indicate why. So I suspect keeping them local is fine.

Wires got shorted - zfs is the one needing the exports. Although it can/will use CC/LD, which we explicit export for that purpose.

@evelikov
Copy link
Collaborator

After a closer look - I absolutely love the PR. There are a handful of small comments and it will be perfect.

KERNEL_CC="$(detect_toolchain_from_kconfig CLANG clang)"
elif [[ -f "$kernel_source_dir/.kernelvariables" ]]; then
# Debian specific
KERNEL_CC="$(echo -e "show-%:\n\t@echo \$(\$*)\ninclude $kernel_source_dir/.kernelvariables" | make -f - show-CC)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

In my testing, detect_toolchain_from_kconfig works correctly and this kernelvariables workaround could be dropped. Not a big deal if you want to keep it, but in practise it's untested and never used.

@evelikov
Copy link
Collaborator

Had a look for the various CONFIG symbols and when they are introduced:

  • CC_IS_GCC GCC_VERSION CC_IS_CLANG CLANG_VERSION - 4.18.0
  • LD_VERSION - 5.7
  • LD_IS_LLD - 5.8 - initial implementation seems broken. uses grep -q LLD, while the actual string is lowercase
  • CONFIG_CC_VERSION_TEXT - 5.8
  • LLD_VERSION - 5.10
  • LD_IS_BFD - 5.12

From the above, is seems that the CONFIG_CC_VERSION_TEXT fallback is of limited use - gcc/clang version fields predate and are consistently handled. Sadly LD handling is all over the place :-(

Most distributions in our matrix are 4.18+ with the odd ones - Centos 7 (3.10), Ubuntu 18 (4.15).

Checking the vmlinux .comment output on my Arch kernel - there are no linker details, only gcc note identical to CONFIG_CC_VERSION_TEXT.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants