Linux: __bad_udelay in drivers/hwmon/applesmc.c

[tpgxyz]

Trying to compile kernel 5.3.1 with LLVM/clang-9.0.0 on x86_64

BUILDSTDERR: ERROR: "__bad_udelay" [drivers/hwmon/applesmc.ko] undefined!
BUILDSTDERR: make[1]: *** [scripts/Makefile.modpost:103: modules-modpost] Error 1
BUILDSTDERR: make: *** [Makefile:1306: modules] Error 2

[nathanchance]

I don't see this on Linux 5.3.1 with LLVM 9.0.0:

diff --git a/arch/x86/configs/x86_64_defconfig b/arch/x86/configs/x86_64_defconfig
index d0a5ffeae8df..ce670a9ed726 100644
--- a/arch/x86/configs/x86_64_defconfig
+++ b/arch/x86/configs/x86_64_defconfig
@@ -297,3 +297,5 @@ CONFIG_SECURITY_SELINUX_DISABLE=y
 CONFIG_EFI_STUB=y
 CONFIG_EFI_MIXED=y
 CONFIG_ACPI_BGRT=y
+CONFIG_INPUT=y
+CONFIG_SENSORS_APPLESMC=m

What kernel source and config are you using?

[nathanchance]

% readelf -p.comment ${KBF}/drivers/hwmon/applesmc.ko

String dump of section '.comment':
  [     1]  ClangBuiltLinux clang version 9.0.0 (git://github.com/llvm/llvm-project 0399d5a9682b3cef71c653373e38890c63c4c365) (based on LLVM 9.0.0)
  [    8a]  ClangBuiltLinux clang version 9.0.0 (git://github.com/llvm/llvm-project 0399d5a9682b3cef71c653373e38890c63c4c365) (based on LLVM 9.0.0)

[tpgxyz]

Here are the sources:
https://github.com/OpenMandrivaAssociation/kernel-release/tree/clang
I've already disabled CONFIG_SENSORS_APPLESMC

Here is the failed build with full logs
https://abf.openmandriva.org/build_lists/610442

[nickdesaulniers]

I've already disabled CONFIG_SENSORS_APPLESMC

Then it would be impossible to have the error "__bad_udelay" [drivers/hwmon/applesmc.ko] undefined! since the .ko would not be built.

That linkage failure occurs though when there's an issue in __builtin_constant_p. cc @gwelymernans

grep -n udelay drivers/hwmon/applesmc.c
162:        udelay(us);
184:        udelay(us);
196:        udelay(APPLESMC_RETRY_WAIT);
245:        udelay(APPLESMC_MIN_WAIT);

shows a few places where maybe udelay is not given an ICE.

[nathanchance]

This is related to your use of -O3:

% make -j$(nproc) -s CC=clang O=out distclean allyesconfig drivers/hwmon/applesmc.ko && nm out/drivers/hwmon/applesmc.ko | grep __bad_udelay
WARNING: modpost: missing MODULE_LICENSE() in drivers/hwmon/applesmc.o
see include/linux/module.h for more information

% make -j$(nproc) -s CC=clang KCFLAGS=-O3 O=out distclean allyesconfig drivers/hwmon/applesmc.ko && nm out/drivers/hwmon/applesmc.ko | grep __bad_udelay
WARNING: modpost: missing MODULE_LICENSE() in drivers/hwmon/applesmc.o
see include/linux/module.h for more information
                 U __bad_udelay

[nickdesaulniers]

@nathanchance probably an interesting test case for LLVM.

[nathanchance]

I'll see what creduce spits out

[nickdesaulniers]

@tpgxyz if you're setting -O3 or have other non-standard kernel patches, please put that in the initial report.

[nathanchance]

a() {
  int b = 10;
  for (; b; b <<= 1)
    if (__builtin_constant_p(b))
      __bad_udelay();
    else
      c();
}

creduce results (files). Corresponds to wait_read and send_byte.

[nickdesaulniers]

super optimizer! https://godbolt.org/z/upRwsr
Clang's loop unroller figured out that the loop will execute 27 iterations before 0x10 <<= i overflows an int32. Unfortunately, it seems that it did not figure out BCP correctly. (cc @jyknight )

[nickdesaulniers]

https://bugs.llvm.org/show_bug.cgi?id=43420

[nickdesaulniers]

So the LLVM bug was closed as WAI, and @stephenhines points out that left-shifting a signed integer is UB.

Both loops should perform bitwise arithmetic on unsigned ints, and have a break like https://github.com/torvalds/linux/blob/9f7582d15f82e86b2041ab22327b7d769e061c1f/drivers/hwmon/applesmc.c#L193-L194 to exit the loop.

Clang may be right here; udelay results in a link error when its parameter is larger than 20000. wait_read iterates up to APPLESMC_MAX_WAIT, which is 0x20000 (HEX!!! == 131072 in decimal).

https://github.com/torvalds/linux/blob/9f7582d15f82e86b2041ab22327b7d769e061c1f/drivers/hwmon/applesmc.c#L45

/* wait up to 128 ms for a status change. */

https://www.kernel.org/doc/Documentation/timers/timers-howto.rst mentions that mdelay is the preferred API to sleep 10ms+

[nickdesaulniers]

https://lkml.org/lkml/2019/9/24/795
v2: https://lkml.org/lkml/2019/9/24/802

[stephenhines]

Left-shifting a signed integer isn't necessarily UB, but this particular case is, since you are overflowing the calculation (sign bit changes unexpectedly in this case).

[nathanchance]

Arnd sent a patch that was picked up: https://git.kernel.org/groeck/linux-staging/c/fff2d0f701e6753591609739f8ab9be1c8e80ebb

[nickdesaulniers]

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fff2d0f701e6753591609739f8ab9be1c8e80ebb