Automatically detecting compiler & linker used by kernel #298

xuzhen · 2023-01-25T16:14:48Z

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

scaronni · 2023-02-03T09:01:11Z

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

xuzhen · 2023-02-03T14:04:56Z

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 · 2023-02-07T11:52:00Z

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 · 2023-02-08T03:11:26Z

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 · 2023-02-08T10:51:29Z

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 · 2023-02-08T12:04:40Z

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 · 2023-02-08T12:37:19Z

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.

evelikov · 2023-03-12T11:24:09Z

dkms.in

@@ -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


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

evelikov · 2023-03-12T11:27:44Z

run_test.sh

-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


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.

evelikov · 2023-03-12T11:32:33Z

run_test.sh

+            fi
+        else
+            echo "CONFIG_MODULE_SIG_HASH=\"${ALTER_HASH}\"" > /tmp/dkms_test_kconfig
+        fi


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

evelikov · 2023-03-12T11:33:37Z

.github/workflows/tests.yml

+    strategy:
+      matrix:
+        version:
+          - 22.04


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

evelikov · 2023-03-12T11:45:30Z

.github/workflows/tests.yml

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


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

evelikov · 2023-03-12T11:48:50Z

dkms.in

-        "$1-$(printf "%d.%d" "$2" "$3")"
-        "$1-$(printf "%d" "$2")"
+        "$1-$((10#$2)).$((10#$3))"
+        "$1-$((10#$2))"


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"

evelikov · 2023-03-12T11:51:43Z

dkms.in

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

+# helper functions for getting cc/ld used by kernel


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.

evelikov · 2023-03-12T11:54:03Z

dkms.in

+            fi
+        fi
+    done
+    return 1


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.

evelikov · 2023-03-12T12:01:01Z

dkms.in

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


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?

evelikov · 2023-03-12T12:01:42Z

dkms.in

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


The trailing return $? should not be needed.

evelikov · 2023-03-12T12:13:54Z

dkms.in

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


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 · 2023-03-12T12:16:41Z

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

evelikov · 2023-03-12T12:19:00Z

dkms.in

+                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)"


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 · 2023-06-12T10:45:37Z

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.

xuzhen force-pushed the cc branch from 8380c42 to 6dfa6f4 Compare January 26, 2023 04:03

xuzhen changed the title ~~Automatically detecting compiler & linker used by kernel~~ (WIP) Automatically detecting compiler & linker used by kernel Jan 26, 2023

Automatically detecting CC & LD used by kernel

a4d904f

xuzhen force-pushed the cc branch 3 times, most recently from 62c5f4f to 4cc99c9 Compare January 26, 2023 06:09

xuzhen added 2 commits January 26, 2023 15:01

Fix test script for kernel compiled by clang

111ae83

Add clang test workflow

0841cb0

xuzhen force-pushed the cc branch from 4cc99c9 to 0841cb0 Compare January 26, 2023 07:02

xuzhen changed the title ~~(WIP) Automatically detecting compiler & linker used by kernel~~ Automatically detecting compiler & linker used by kernel Jan 26, 2023

xuzhen mentioned this pull request Jan 26, 2023

Workaround for activating rhel's software collection toolchain #299

Open

Fix printf errors caused by leading zeros

7f44fd9

evelikov reviewed Mar 12, 2023

View reviewed changes

evelikov mentioned this pull request Mar 14, 2023

Fix building with clang and lld #294

Merged

evelikov mentioned this pull request Jun 12, 2023

Fix CC detection #334

Merged

evelikov mentioned this pull request Nov 7, 2023

Improve 'make' command substitution #360

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically detecting compiler & linker used by kernel #298

Automatically detecting compiler & linker used by kernel #298

xuzhen commented Jan 25, 2023 •

edited

Loading

scaronni commented Feb 3, 2023

xuzhen commented Feb 3, 2023

evelikov commented Feb 7, 2023 •

edited

Loading

xuzhen commented Feb 8, 2023

evelikov commented Feb 8, 2023

xuzhen commented Feb 8, 2023

xuzhen commented Feb 8, 2023

evelikov Mar 12, 2023

evelikov Mar 12, 2023 •

edited

Loading

evelikov Mar 12, 2023

evelikov Mar 12, 2023

evelikov Mar 12, 2023

evelikov Mar 12, 2023

evelikov Mar 12, 2023

evelikov Mar 12, 2023 •

edited

Loading

evelikov Mar 12, 2023

evelikov Mar 12, 2023

evelikov Mar 12, 2023 •

edited

Loading

evelikov commented Mar 12, 2023

evelikov Mar 12, 2023

evelikov commented Jun 12, 2023

Automatically detecting compiler & linker used by kernel #298

Are you sure you want to change the base?

Automatically detecting compiler & linker used by kernel #298

Conversation

xuzhen commented Jan 25, 2023 • edited Loading

scaronni commented Feb 3, 2023

xuzhen commented Feb 3, 2023

evelikov commented Feb 7, 2023 • edited Loading

xuzhen commented Feb 8, 2023

evelikov commented Feb 8, 2023

xuzhen commented Feb 8, 2023

xuzhen commented Feb 8, 2023

Choose a reason for hiding this comment

evelikov Mar 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evelikov Mar 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evelikov Mar 12, 2023 • edited Loading

Choose a reason for hiding this comment

evelikov commented Mar 12, 2023

Choose a reason for hiding this comment

evelikov commented Jun 12, 2023

xuzhen commented Jan 25, 2023 •

edited

Loading

evelikov commented Feb 7, 2023 •

edited

Loading

evelikov Mar 12, 2023 •

edited

Loading

evelikov Mar 12, 2023 •

edited

Loading

evelikov Mar 12, 2023 •

edited

Loading