Re: [PATCH net-next] net: ethernet: rmnet: Restructure if checks to avoid uninitialized warning

Hi Subash,

On 6/3/2021 10:15 PM, subashab@codeaurora.org wrote:

On 2021-06-03 16:40, patchwork-bot+netdevbpf@kernel.org wrote:

quoted

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Thu,  3 Jun 2021 10:34:10 -0700 you wrote:

quoted

Clang warns that proto in rmnet_map_v5_checksum_uplink_packet() might be
used uninitialized:

drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:283:14: warning:
variable 'proto' is used uninitialized whenever 'if' condition is false
[-Wsometimes-uninitialized]
                } else if (skb->protocol == htons(ETH_P_IPV6)) {
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:295:36: note:
uninitialized use occurs here
                check = rmnet_map_get_csum_field(proto, trans);
                                                 ^~~~~
drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:283:10: note:
remove the 'if' if its condition is always true
                } else if (skb->protocol == htons(ETH_P_IPV6)) {
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:270:11: note:
initialize the variable 'proto' to silence this warning
                u8 proto;
                        ^
                         = '\0'
1 warning generated.

[...]

Here is the summary with links:
  - [net-next] net: ethernet: rmnet: Restructure if checks to avoid
uninitialized warning
    https://git.kernel.org/netdev/net-next/c/118de6106735

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Hi Nathan

Can you tell why CLANG detected this error.
Does it require a bug fix.

As far as I understand it, clang does not remember the conditions of previous if statements when generating this warning. Basically:

void bar(int x)
{
}

int foo(int a, int b)
{
	int x;

	if (!a && !b)
		goto out;

	if (a)
		x = 1;
	else if (b)
		x = 2;

	bar(x);

out:
	return 0;
}

clang will warn that x is uninitialized when neither of the second if statement's conditions are true, even though we as humans know that is not possible due to the first if statement. I am guessing this has something to do with how clang generates its control flow graphs. While this is a false positive, I do not personally see this as a bug in the compiler. The code is more clear to both the compiler and humans if it is written as:

	if (a)
		x = 1;
	else if (b)
		x = 2;
	else
		goto out;

Cheers,
Nathan