bugs -Wsometimes-uninitialized warning

[Quuxplusone]

Bugzilla Link	PR41197
Status	NEW
Importance	P enhancement
Reported by	Arnd Bergmann (arnd@linaro.org)
Reported on	2019-03-22 04:07:38 -0700
Last modified on	2019-03-22 12:55:38 -0700
Version	8.0
Hardware	PC Linux
CC	dblaikie@gmail.com, htmldeveloper@gmail.com, llvm-bugs@lists.llvm.org, ndesaulniers@google.com, neeilans@live.com, richard-llvm@metafoo.co.uk
Fixed by commit(s)
Attachments
Blocks	PR4068
Blocked by
See also

I found a couple of instances in the linux kernel where clang warns about an
uninitialized variable getting used in a condition that is clearly never true:

Here is a simplified test case from https://godbolt.org/z/7brWaN
int f3(int x)
{
    int y3;

    if (x == 1)
        y3 = 1;
    else if (x != 1)
        y3 = 1;

    return y3;
}

gcc does not warn here, but it's unclear whether this is intentional or not,
given that it also does not warn about similar but incorrect code.

Related warnings I see in the kernel are:

**
    drivers/pwm/pwm-img.c:126:13: error: variable 'timebase' is used uninitialized whenever 'if' condition is false
          [-Werror,-Wsometimes-uninitialized]
            } else if (mul > max_timebase * 512) {
                       ^~~~~~~~~~~~~~~~~~~~~~~~
    drivers/pwm/pwm-img.c:132:22: note: uninitialized use occurs here
            duty = DIV_ROUND_UP(timebase * duty_ns, period_ns);
                                ^~~~~~~~
**
    drivers/block/rbd.c:2402:4: error: variable 'ret' is used uninitialized whenever 'if' condition is false
          [-Werror,-Wsometimes-uninitialized]
                            rbd_assert(0);
                            ^~~~~~~~~~~~~
    drivers/block/rbd.c:563:7: note: expanded from macro 'rbd_assert'
                    if (unlikely(!(expr))) {                                \
                        ^~~~~~~~~~~~~~~~~
    include/linux/compiler.h:48:23: note: expanded from macro 'unlikely'
     #  define unlikely(x)   (__branch_check__(x, 0, __builtin_constant_p(x)))
                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    drivers/block/rbd.c:2410:6: note: uninitialized use occurs here
            if (ret) {
                ^~~
    drivers/block/rbd.c:2402:4: note: remove the 'if' if its condition is always true
                            rbd_assert(0);
                            ^
    drivers/block/rbd.c:563:3: note: expanded from macro 'rbd_assert'
                    if (unlikely(!(expr))) {                                \
                    ^
    drivers/block/rbd.c:2376:9: note: initialize the variable 'ret' to silence this warning
            int ret;
**
    drivers/net/wireless/intel/iwlwifi/mvm/sta.c:2114:12: error: variable 'queue' is used uninitialized whenever 'if'
          condition is false [-Werror,-Wsometimes-uninitialized]
                    else if (WARN(1, "Missing required TXQ for adding bcast STA\n"))
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    include/asm-generic/bug.h:130:36: note: expanded from macro 'WARN'
     #define WARN(condition, format...) ({                                   \
                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    drivers/net/wireless/intel/iwlwifi/mvm/sta.c:2119:33: note: uninitialized use occurs here
                    iwl_mvm_enable_txq(mvm, NULL, queue, 0, &cfg, wdg_timeout);
                                                  ^~~~~
**
    fs/btrfs/uuid-tree.c:129:13: error: variable 'eb' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
            } else if (ret < 0) {
                       ^~~~~~~
    fs/btrfs/uuid-tree.c:139:22: note: uninitialized use occurs here
            write_extent_buffer(eb, &subid_le, offset, sizeof(subid_le));
                                ^~
    fs/btrfs/uuid-tree.c:129:9: note: remove the 'if' if its condition is always true
            } else if (ret < 0) {
                   ^~~~~~~~~~~~~
    fs/btrfs/uuid-tree.c:90:26: note: initialize the variable 'eb' to silence this warning
            struct extent_buffer *eb;
**
    net/wireless/util.c:1223:11: error: variable 'result' is used uninitialized whenever 'if' condition is false
          [-Werror,-Wsometimes-uninitialized]
            else if (WARN(1, "invalid HE MCS: bw:%d, ru:%d\n",
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    include/asm-generic/bug.h:130:36: note: expanded from macro 'WARN'
     define WARN(condition, format...) ({                                   \
                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    net/wireless/util.c:1228:8: note: uninitialized use occurs here
            tmp = result;
                  ^~~~~~
    net/wireless/util.c:1223:7: note: remove the 'if' if its condition is always true
            else if (WARN(1, "invalid HE MCS: bw:%d, ru:%d\n",
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    net/wireless/util.c:1187:12: note: initialize the variable 'result' to silence this warning
            u32 result;

I have made workaround for all of the above.

[Quuxplusone]

Another related test case modeled after real kernel code would be this one:

int f4(void)
{
    int x, y;

    switch (0) {
    default:
        x = 1;
    }

    if (x)
        y = 1;

    return y;
}

The original warning is

sound/pci/hda/patch_ca0132.c:7558:6: error: variable 'fw_entry' is used
uninitialized whenever 'if' condition is false
      [-Werror,-Wsometimes-uninitialized]
        if (!spec->alt_firmware_present) {
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~
sound/pci/hda/patch_ca0132.c:7565:42: note: uninitialized use occurs here
        dsp_os_image = (struct dsp_image_seg *)(fw_entry->data);
                                                ^~~~~~~~
sound/pci/hda/patch_ca0132.c:7558:2: note: remove the 'if' if its condition is
always true
        if (!spec->alt_firmware_present) {
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sound/pci/hda/patch_ca0132.c:7521:33: note: initialize the variable 'fw_entry'
to silence this warning
        const struct firmware *fw_entry;
                                       ^
                                        = NULL

[Quuxplusone]

As far as I can see, the warning is correct in both cases. The purpose of this warning is to detect cases where there is either a dead branch or an uninitialized variable, which is the case in both examples.

Are the dead branches being produced by macros? If so, we could probably suppress the warning on such cases. But if the code literally contains branch conditions that are impossible, then that's a case that this warning is intended to catch, and if you don't want to be warned on that, your should disable the warning.

[Quuxplusone]

(In reply to Richard Smith from comment #2)
> As far as I can see, the warning is correct in both cases. The purpose of
> this warning is to detect cases where there is either a dead branch or an
> uninitialized variable, which is the case in both examples.

Ok, I see. The part about dead branches is not obvious here,
but I see what you mean.

Just to clarify: with 'dead branch', do you mean the implied 'else'
portion of the always-true "if (x != 1)" in this?

    if (x == 1)
        y3 = 1;
    else if (x != 1)
        y3 = 1;

> Are the dead branches being produced by macros? If so, we could probably
> suppress the warning on such cases. But if the code literally contains
> branch conditions that are impossible, then that's a case that this warning
> is intended to catch, and if you don't want to be warned on that, your
> should disable the warning.

I saw one that had a "switch (0)" with the 0 generated by a macro
intended to disable all "case" statements. No need to worry about that.

A couple of cases involve a macro that sees if __builtin_expect() annotations
are actually correct:

#define __branch_check__(x, expect, is_constant) ({                     \
                        long ______r;                                   \
                        static struct ftrace_likely_data                \
                                __aligned(4)                            \
                                __section("_ftrace_annotated_branch")   \
                                ______f = {                             \
                                .data.func = __func__,                  \
                                .data.file = __FILE__,                  \
                                .data.line = __LINE__,                  \
                        };                                              \
                        ______r = __builtin_expect(!!(x), expect);      \
                        ftrace_likely_update(&______f, ______r,         \
                                             expect, is_constant);      \
                        ______r;                                        \
                })
#ifdef CONFIG_TRACE_BRANCH_PROFILING
#  define likely(x)     (__branch_check__(x, 1, __builtin_constant_p(x)))
#  define unlikely(x)   (__branch_check__(x, 0, __builtin_constant_p(x)))
#else
#  define likely(x)      __builtin_expect(!!(x), 1)
#  define unlikely(x)    __builtin_expect(!!(x), 0)
#endif

This one also confuses gcc sometimes, but it seems to reliably mess up
clangs dead code detection when combined with constant input (in cases
that behave as expected without CONFIG_TRACE_BRANCH_PROFILING). In a lot of
cases the branch with constant input can be trivially converted to an
unconditional expression, such as replacing BUG_ON(1) with BUG().