New user self-registration is disabled due to spam. For an account please email bugs-admin@lists.llvm.org with your e-mail address and full name.

Bug 41197 - bugs -Wsometimes-uninitialized warning
Summary: bugs -Wsometimes-uninitialized warning
Status: NEW
Alias: None
Product: clang
Classification: Unclassified
Component: -New Bugs (show other bugs)
Version: 8.0
Hardware: PC Linux
: P enhancement
Assignee: Unassigned Clang Bugs
URL:
Keywords:
Depends on:
Blocks: 4068
  Show dependency tree
 
Reported: 2019-03-22 04:07 PDT by Arnd Bergmann
Modified: 2019-03-22 12:55 PDT (History)
6 users (show)

See Also:
Fixed By Commit(s):


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Arnd Bergmann 2019-03-22 04:07:38 PDT
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.
Comment 1 Arnd Bergmann 2019-03-22 06:03:10 PDT
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
Comment 2 Richard Smith 2019-03-22 10:12:21 PDT
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.
Comment 3 Arnd Bergmann 2019-03-22 12:55:38 PDT
(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().