GCC v11 32F4: compiler warning depends on optimisation level

[peter-h]

This function produces no warning with -Og or -Os but with most others e.g. -O1 you get

'ipaddr.addr' may be used uninitialized in this function [-Wmaybe-uninitialized] line 182



How does that work?

[Nominal Animal]

Such "maybe" warnings are based on the pattern in the intermediate representation (GIMPLE or GENERIC for GCC) after optimizations are applied, and are heuristic instead of deterministic.

In your case, ipaddr indeed is used uninitialized if the value does not conform to numeric IPv4 dotted notation.  Initializing it to all zeros would make a lot of sense in my opinion.

[SiliconWizard]

As Nominal said, it does depend on the internal representation the compilers makes, but I would see it as a limitation of its static analyzer, which is still not very good for GCC. The CLang one is better.

You simply have execution paths for which ipaddr is not set (if both sscanf fail), and the function returns this variable it in all cases. So the warning is definitely relevant.
The fact it only appears in some optimization levels isn't great IMHO, but would again be explained with how GCC handles it internally.

You should initialize ipaddr to some default value. Not sure what the type is, but simply:
ip_addr_t ipaddr = {};
should do (I think formally it's only correct in recent standard revisions - maybe strictly C2x? - but GCC has been accepting that for a long time for initializing almost anything to '0').

I would suggest avoiding sscanf() too here, ideally. Not only is it relatively expensive (both in terms of code size and execution time), but it's also pretty unsafe.

[coppice]

What platform is this? IP4_ADDR is usually a macro. So, depending exactly what that macro contains, ipaddr.addr might we well be used undefined.

[golden_labels]

Extending what Nominal Animal said about such best-effort warnings, -Og disables dead store elimination which is otherwise enabled. We may speculate, what is going on. Most notably that the difference lies outside ▒▒▒_netconf_get_ip_config_value. Variable ipaddr is allocated not in this function, but in its caller. Something in the caller writes over storage used by ipaddr, which is ignored with DSE enabled (hence seen as uninitialized), but is not ignored with DSE disabled (hence seen as initialized, even if not intentionally).

That may be separate from the logical error in this function itself, where an execution path exists allowing ipaddr to never be set before being returned.

[peter-h]

Thank you all

I can see that if both sscanf calls fail, the IP is undefined in this function. I just could not see how the optimisation level should affect this warning.

I googled on Dead Store Elimination and while I can see why -Og disables it (it enables better single stepping) I can't see why -Os should disable it.

I also can't see how
   unsigned int a, b, c, d;
   if (sscanf(value, "%u.%u.%u.%u", &a, &b, &c, &d) == 4) ...
could make the sscanf fail. Surely any int (without a - sign) will be valid for %u. But yes it might fail if not seeing that x.x.x.x pattern, but file_get_config_value_default() does always return a valid string, in the form "169.254.75.75" (static const definitions).

So maybe the compiler is trying too hard and getting it wrong.

Probably I should have
ip_addr_t ipaddr = {0};
and sure enough that removes the warning with -O1.

BTW, the defs are

typedef struct ip4_addr ip4_addr_t;
typedef ip4_addr_t ip_addr_t;
struct ip4_addr {  u32_t addr; };

For sure sscanf() is slow but this code just gets some IPs from a config file, and returns default values if the value is missing in the file. It seems to work fine. It's like snprintf() - it has its uses in the right places. I just hope no customer ever asks for IPV6

[ejeffrey]

I also can't see how
   unsigned int a, b, c, d;
   if (sscanf(value, "%u.%u.%u.%u", &a, &b, &c, &d) == 4) ...
could make the sscanf fail. Surely any int will be valid for %u. But yes it might fail if not seeing that x.x.x.x pattern, but file_get_config_value_default() does always return a valid string, in the form "169.254.75.75" (static const definitions).

You may be right that sscanf can never fail, but you bothered to put the "if  (sscanf(default_value, "%u.%u.%u.%u", &a, &b, &c, &d) == 4)" test in, so it certainly looks like you weren't *that* confident...

[coppice]

I also can't see how
   unsigned int a, b, c, d;
   if (sscanf(value, "%u.%u.%u.%u", &a, &b, &c, &d) == 4) ...
could make the sscanf fail. Surely any int will be valid for %u. But yes it might fail if not seeing that x.x.x.x pattern, but file_get_config_value_default() does always return a valid string, in the form "169.254.75.75" (static const definitions).

You may be right that sscanf can never fail, but you bothered to put the "if  (sscanf(default_value, "%u.%u.%u.%u", &a, &b, &c, &d) == 4)" test in, so it certainly looks like you weren't *that* confident...

Of course sscanf can fail from the compiler's viewpoint. In his code he might be sure everything is sanitised, and only a minimum of 4 groups of digits may be present at that point, but there is no way the compiler can be expected to know for sure. They are doing pretty well these days with checking whether arithmetic has been sanitised, but I don't think they have any text to integer checking.

[peter-h]

You may be right that sscanf can never fail, but you bothered to put the "if  (sscanf(default_value, "%u.%u.%u.%u", &a, &b, &c, &d) == 4)" test in, so it certainly looks like you weren't *that* confident...

Yes, I agree. FWIW I didn't write this code. It was done by the guy who worked on it a few years ago. May even be open source.

What bothered me most was the effect of the -O level. I still don't actually get it. I am not seeing warnings from elsewhere, and probably should if file_get_config_value_default() could return an undefined IP.

but I don't think they have any text to integer checking.

That would suggest that sscanf() should always be assumed to fail, inside any function where the string is supplied as a parameter to that function. And in this case it is two call depths removed...

[coppice]

but I don't think they have any text to integer checking.

That would suggest that sscanf() should always be assumed to fail, inside any function where the string is supplied as a parameter to that function. And in this case it is two call depths removed...

Unless the string is guaranteed to have been sanitised, of course sscanf can fail. The depth would generally be less important than whether all the code is available in scope during compilation. If you are linking together various separately compiled pieces it seems unlikely such analysis will ever be performed by future compilers.

[peter-h]

Unless the string is guaranteed to have been sanitised

That seems to depend on the optimisation level, and I can't see why.

What I can see is that it would require an incredible amount of code to determine that the string passed to the sscanf() is OK - even though it is defined (several levels up the call hierarchy) with e.g.
#define IP_STATIC_DEFAULT "169.254.75.75"
and the buffer for it is 20 bytes.

This is interesting: https://www.phoronix.com/news/GCC-12-Better-Security re zeroing of stack variables but I am still on GCC v11.

[golden_labels]

Regarding value sanitization and validity checks: there is inet_pton (Linux, Beej’s). That also solves your worry about customers wanting support for IPv6 and any potential future addressing family.

[SiliconWizard]

Unless the string is guaranteed to have been sanitised

That seems to depend on the optimisation level, and I can't see why.

My guess (while I would have to check, though) is that at < -O1 optimization level, the compiler may automatically initialize to zero all unitialized local variables, which would explain why it doesn't give a warning.
Not, again, that I would find this warning behavior sensible. Even if the compiler was  automatically initializing to zero, I do think it should still warn about the possible unitialized variable.
And no, I don't buy either that it would do deep enough analysis to statically detect that the sscanf can never fail, and even so, why optimization would change anything about it. So the more likely reason to me is what I mentioned, although that would need again to be checked (you can look at the generated assembly in both opt levels, or use godbolt.org for that.)

As to using sscanf(), I and others made some points, after that, it's obviously your code and your call.

[peter-h]

At -Og, I see no zeroing code



But I probably don't understand the subtleties of arm32 asm since the code with = { 0 } produces the same asm. The bic.w instruction might be doing the job but the #7 operand makes no sense



and this is with -O1 but not zeroing the IP; same code produced!

[peter-h]

Looks like the mystery remains

Looks to me like ipaddr is getting zeroed in all 3 cases.

[xvr]

My guess (while I would have to check, though) is that at < -O1 optimization level, the compiler may automatically initialize to zero all unitialized local variables, which would explain why it doesn't give a warning.

I guess that all is a much simpler. To produce this kind of warning compiler have to run some data analyzing passes (at least it should build Control Flow Graph and run accessibility analyze on it). Compiler will build these graphs on high optimization level (not to produce warnings, but to perform required optimizations). But on -O0 it will not build them, so no possibility to perform warnings check (just not enough information). And gcc do not bother to run these data analyses passes only to produce warnings.

[Siwastaja]

My guess (while I would have to check, though) is that at < -O1 optimization level, the compiler may automatically initialize to zero all unitialized local variables, which would explain why it doesn't give a warning.

I guess that all is a much simpler. To produce this kind of warning compiler have to run some data analyzing passes (at least it should build Control Flow Graph and run accessibility analyze on it). Compiler will build these graphs on high optimization level (not to produce warnings, but to perform required optimizations). But on -O0 it will not build them, so no possibility to perform warnings check (just not enough information). And gcc do not bother to run these data analyses passes only to produce warnings.

This makes sense; it's well known how higher optimization levels compile slower. Probably not that significant today, but we used to see benchmarks of compilation speeds at different settings. So obviously compilers are doing much more work to analyze the code at higher optimization levels, and as side effect, would also emit more warnings.

Remember, GCC is not a proper/perfect static analysis tool. Warnings are a bonus; but you can't trust they are always generated, even when it's trivially easy for a human being (or better static analyzer) to prove that code has a problem.

[peter-h]

However, several things:

1) I am not using -O0 at all (at some +30%, the code is too big for the product)
2) We are seeing the same code generated for -Og and -O1, but only -O1 generates the warning
3) GCC claims (variously?) that -Og may zero stack variables but -O1 does not, yet the generated code does not confirm this

Can anyone confirm that ipaddr is actually being zeroed? I cannot find an asm language reference explaining some of the code.

It would be quite useful to find out the reason for the warning, but I guess this is not possible?

[Siwastaja]

It would be quite useful to find out the reason for the warning, but I guess this is not possible?

Reason for warning is that the C code does not zero ipaddr and may use it uninitialized.

Reason for not always getting warning can be complex, it makes no sense to delve deep into rabbit hole of GCC internals to understand exactly why. Unless you are a gcc developer and want to improve it.

[peter-h]

What does this asm do?



It is the same if I use ipaddr={0} - the above screenshots show that.

[rhodges]

What does this asm do?

It rounds r1 up to a 64-bit word, moves the stack pointer down for local variable space, pointed to by r5.
This code does not clear any variable.

[peter-h]

This code does not clear any variable.

Right, so why did my ipaddr = {0} produce the same asm?

It is not as if ipaddr was a static and thus in BSS and thus zeroed at startup. This stuff is on the stack so could be anything.

[Tation]

Sure it is on the stack? Fitting in 32 bit it may well be in a register.

[ejeffrey]

You need to look at the whole function.  A very simple optimization would be to move the zero initialization into the conditional branch where scanf fails, since otherwise the initialized values are dead stores.

One other complication: in most C ABIs, some struct return values are caller allocated with a pointer provided as a hidden parameter.   In arm32 I think it's up to 32 bit size objects?  I think ip_addr_t is large enough to hold either a v4 or v6 address so it probably qualifies.  In addition, copy elision is performed whenever possible so the function doesn't create its own local variable then copy it to the caller provided buffer, instead it works on the provided buffer directly.  In C++ where copy/move constructors can have side effects this behavior is required at any optimization level, so I wouldn't be surprised if gcc is doing this for C regardless of the optimization level.

[peter-h]

OK here is the asm for a non-zeroed ipaddr

Code: [Select]

0805088c <netconf_get_ip_config_value>:
static ip_addr_t netconf_get_ip_config_value(
	char *name,				// In	Config option name to read
	char *default_value,	// In	The default value of the option as a string
	char *filename,			// In	The filename of the config file
	int number
) {
 805088c:	b5f0      	push	{r4, r5, r6, r7, lr}
 805088e:	b087      	sub	sp, #28
 8050890:	af02      	add	r7, sp, #8
 8050892:	460e      	mov	r6, r1
	ip_addr_t ipaddr;
	unsigned int a, b, c, d;
	char value[number];
 8050894:	1dd9      	adds	r1, r3, #7
 8050896:	f021 0107 	bic.w	r1, r1, #7
 805089a:	ebad 0d01 	sub.w	sp, sp, r1
 805089e:	466d      	mov	r5, sp
	//char value[20];

	file_get_config_value_default(name, value, default_value, filename, sizeof(value));
 80508a0:	b29b      	uxth	r3, r3
 80508a2:	f845 3b08 	str.w	r3, [r5], #8
 80508a6:	4613      	mov	r3, r2
 80508a8:	4632      	mov	r2, r6
 80508aa:	4629      	mov	r1, r5
 80508ac:	f7fc f95e 	bl	804cb6c <file_get_config_value_default>

	if (sscanf(value, "%u.%u.%u.%u", &a, &b, &c, &d) == 4) {
 80508b0:	9701      	str	r7, [sp, #4]
 80508b2:	1d3b      	adds	r3, r7, #4
 80508b4:	9300      	str	r3, [sp, #0]
 80508b6:	f107 0308 	add.w	r3, r7, #8
 80508ba:	f107 020c 	add.w	r2, r7, #12
 80508be:	4924      	ldr	r1, [pc, #144]	; (8050950 <netconf_get_ip_config_value+0xc4>)
 80508c0:	4628      	mov	r0, r5
 80508c2:	f008 f943 	bl	8058b4c <sscanf>
 80508c6:	2804      	cmp	r0, #4
 80508c8:	d11b      	bne.n	8050902 <netconf_get_ip_config_value+0x76>
		IP4_ADDR(&ipaddr, a, b, c, d);
 80508ca:	68bb      	ldr	r3, [r7, #8]
 80508cc:	041b      	lsls	r3, r3, #16
 80508ce:	f403 037f 	and.w	r3, r3, #16711680	; 0xff0000
 80508d2:	68fa      	ldr	r2, [r7, #12]
 80508d4:	ea43 6302 	orr.w	r3, r3, r2, lsl #24
 80508d8:	687a      	ldr	r2, [r7, #4]
 80508da:	0212      	lsls	r2, r2, #8
 80508dc:	b292      	uxth	r2, r2
 80508de:	4313      	orrs	r3, r2
 80508e0:	783a      	ldrb	r2, [r7, #0]
 80508e2:	4313      	orrs	r3, r2
 80508e4:	021c      	lsls	r4, r3, #8
 80508e6:	f404 047f 	and.w	r4, r4, #16711680	; 0xff0000
 80508ea:	ea44 6402 	orr.w	r4, r4, r2, lsl #24
 80508ee:	0a1a      	lsrs	r2, r3, #8
 80508f0:	f402 427f 	and.w	r2, r2, #65280	; 0xff00
 80508f4:	4314      	orrs	r4, r2
 80508f6:	ea44 6413 	orr.w	r4, r4, r3, lsr #24
			IP4_ADDR(&ipaddr, a, b, c, d);
		}
	}

	return ipaddr;
}

and here is the asm for the zeroed ipaddr:

Code: [Select]

0805088c <netconf_get_ip_config_value>:
static ip_addr_t netconf_get_ip_config_value(
	char *name,				// In	Config option name to read
	char *default_value,	// In	The default value of the option as a string
	char *filename,			// In	The filename of the config file
	int number
) {
 805088c:	b5f0      	push	{r4, r5, r6, r7, lr}
 805088e:	b087      	sub	sp, #28
 8050890:	af02      	add	r7, sp, #8
 8050892:	460e      	mov	r6, r1
	ip_addr_t ipaddr = {0};
	unsigned int a, b, c, d;
	char value[number];
 8050894:	1dd9      	adds	r1, r3, #7
 8050896:	f021 0107 	bic.w	r1, r1, #7
 805089a:	ebad 0d01 	sub.w	sp, sp, r1
 805089e:	466d      	mov	r5, sp
	//char value[20];

	file_get_config_value_default(name, value, default_value, filename, sizeof(value));
 80508a0:	b29b      	uxth	r3, r3
 80508a2:	f845 3b08 	str.w	r3, [r5], #8
 80508a6:	4613      	mov	r3, r2
 80508a8:	4632      	mov	r2, r6
 80508aa:	4629      	mov	r1, r5
 80508ac:	f7fc f95e 	bl	804cb6c <file_get_config_value_default>

	if (sscanf(value, "%u.%u.%u.%u", &a, &b, &c, &d) == 4) {
 80508b0:	9701      	str	r7, [sp, #4]
 80508b2:	1d3b      	adds	r3, r7, #4
 80508b4:	9300      	str	r3, [sp, #0]
 80508b6:	f107 0308 	add.w	r3, r7, #8
 80508ba:	f107 020c 	add.w	r2, r7, #12
 80508be:	4924      	ldr	r1, [pc, #144]	; (8050950 <netconf_get_ip_config_value+0xc4>)
 80508c0:	4628      	mov	r0, r5
 80508c2:	f008 f943 	bl	8058b4c <sscanf>
 80508c6:	2804      	cmp	r0, #4
 80508c8:	d11b      	bne.n	8050902 <netconf_get_ip_config_value+0x76>
		IP4_ADDR(&ipaddr, a, b, c, d);
 80508ca:	68bb      	ldr	r3, [r7, #8]
 80508cc:	041b      	lsls	r3, r3, #16
 80508ce:	f403 037f 	and.w	r3, r3, #16711680	; 0xff0000
 80508d2:	68fa      	ldr	r2, [r7, #12]
 80508d4:	ea43 6302 	orr.w	r3, r3, r2, lsl #24
 80508d8:	687a      	ldr	r2, [r7, #4]
 80508da:	0212      	lsls	r2, r2, #8
 80508dc:	b292      	uxth	r2, r2
 80508de:	4313      	orrs	r3, r2
 80508e0:	783a      	ldrb	r2, [r7, #0]
 80508e2:	4313      	orrs	r3, r2
 80508e4:	021c      	lsls	r4, r3, #8
 80508e6:	f404 047f 	and.w	r4, r4, #16711680	; 0xff0000
 80508ea:	ea44 6402 	orr.w	r4, r4, r2, lsl #24
 80508ee:	0a1a      	lsrs	r2, r3, #8
 80508f0:	f402 427f 	and.w	r2, r2, #65280	; 0xff00
 80508f4:	4314      	orrs	r4, r2
 80508f6:	ea44 6413 	orr.w	r4, r4, r3, lsr #24
			IP4_ADDR(&ipaddr, a, b, c, d);
		}
	}

	return ipaddr;
}

A comparison shows no change.

Above is with -Og. With -O1 I do see a difference. Here are the two cases, same order as above

Code: [Select]

0804f40c <netconf_get_ip_config_value>:
static ip_addr_t netconf_get_ip_config_value(
	char *name,				// In	Config option name to read
	char *default_value,	// In	The default value of the option as a string
	char *filename,			// In	The filename of the config file
	int number
) {
 804f40c:	b5f0      	push	{r4, r5, r6, r7, lr}
 804f40e:	b087      	sub	sp, #28
 804f410:	af02      	add	r7, sp, #8
 804f412:	460e      	mov	r6, r1
	ip_addr_t ipaddr;
	unsigned int a, b, c, d;
	char value[number];
 804f414:	1dd9      	adds	r1, r3, #7
 804f416:	f021 0107 	bic.w	r1, r1, #7
 804f41a:	ebad 0d01 	sub.w	sp, sp, r1
 804f41e:	466d      	mov	r5, sp
	//char value[20];

	file_get_config_value_default(name, value, default_value, filename, sizeof(value));
 804f420:	b29b      	uxth	r3, r3
 804f422:	f845 3b08 	str.w	r3, [r5], #8
 804f426:	4613      	mov	r3, r2
 804f428:	4632      	mov	r2, r6
 804f42a:	4629      	mov	r1, r5
 804f42c:	f7fc fa04 	bl	804b838 <file_get_config_value_default>

	if (sscanf(value, "%u.%u.%u.%u", &a, &b, &c, &d) == 4) {
 804f430:	9701      	str	r7, [sp, #4]
 804f432:	1d3b      	adds	r3, r7, #4
 804f434:	9300      	str	r3, [sp, #0]
 804f436:	f107 0308 	add.w	r3, r7, #8
 804f43a:	f107 020c 	add.w	r2, r7, #12
 804f43e:	491a      	ldr	r1, [pc, #104]	; (804f4a8 <netconf_get_ip_config_value+0x9c>)
 804f440:	4628      	mov	r0, r5
 804f442:	f008 f819 	bl	8057478 <sscanf>
 804f446:	2804      	cmp	r0, #4
 804f448:	d111      	bne.n	804f46e <netconf_get_ip_config_value+0x62>
		IP4_ADDR(&ipaddr, a, b, c, d);
 804f44a:	7839      	ldrb	r1, [r7, #0]
 804f44c:	68bb      	ldr	r3, [r7, #8]
 804f44e:	021b      	lsls	r3, r3, #8
 804f450:	687a      	ldr	r2, [r7, #4]
 804f452:	0212      	lsls	r2, r2, #8
 804f454:	b292      	uxth	r2, r2
 804f456:	7b3c      	ldrb	r4, [r7, #12]
 804f458:	ea44 6401 	orr.w	r4, r4, r1, lsl #24
 804f45c:	ea44 2402 	orr.w	r4, r4, r2, lsl #8
 804f460:	f403 437f 	and.w	r3, r3, #65280	; 0xff00
 804f464:	431c      	orrs	r4, r3
			IP4_ADDR(&ipaddr, a, b, c, d);
		}
	}

	return ipaddr;
}

Code: [Select]

0804f40c <netconf_get_ip_config_value>:
static ip_addr_t netconf_get_ip_config_value(
	char *name,				// In	Config option name to read
	char *default_value,	// In	The default value of the option as a string
	char *filename,			// In	The filename of the config file
	int number
) {
 804f40c:	b5b0      	push	{r4, r5, r7, lr}
 804f40e:	b086      	sub	sp, #24
 804f410:	af02      	add	r7, sp, #8
 804f412:	460d      	mov	r5, r1
	ip_addr_t ipaddr = {0};
	unsigned int a, b, c, d;
	char value[number];
 804f414:	1dd9      	adds	r1, r3, #7
 804f416:	f021 0107 	bic.w	r1, r1, #7
 804f41a:	ebad 0d01 	sub.w	sp, sp, r1
 804f41e:	466c      	mov	r4, sp
	//char value[20];

	file_get_config_value_default(name, value, default_value, filename, sizeof(value));
 804f420:	b29b      	uxth	r3, r3
 804f422:	f844 3b08 	str.w	r3, [r4], #8
 804f426:	4613      	mov	r3, r2
 804f428:	462a      	mov	r2, r5
 804f42a:	4621      	mov	r1, r4
 804f42c:	f7fc fa04 	bl	804b838 <file_get_config_value_default>

	if (sscanf(value, "%u.%u.%u.%u", &a, &b, &c, &d) == 4) {
 804f430:	9701      	str	r7, [sp, #4]
 804f432:	1d3b      	adds	r3, r7, #4
 804f434:	9300      	str	r3, [sp, #0]
 804f436:	f107 0308 	add.w	r3, r7, #8
 804f43a:	f107 020c 	add.w	r2, r7, #12
 804f43e:	491a      	ldr	r1, [pc, #104]	; (804f4a8 <netconf_get_ip_config_value+0x9c>)
 804f440:	4620      	mov	r0, r4
 804f442:	f008 f819 	bl	8057478 <sscanf>
 804f446:	2804      	cmp	r0, #4
 804f448:	d110      	bne.n	804f46c <netconf_get_ip_config_value+0x60>
		IP4_ADDR(&ipaddr, a, b, c, d);
 804f44a:	7839      	ldrb	r1, [r7, #0]
 804f44c:	68bb      	ldr	r3, [r7, #8]
 804f44e:	021b      	lsls	r3, r3, #8
 804f450:	687a      	ldr	r2, [r7, #4]
 804f452:	0212      	lsls	r2, r2, #8
 804f454:	b292      	uxth	r2, r2
 804f456:	7b38      	ldrb	r0, [r7, #12]
 804f458:	ea40 6001 	orr.w	r0, r0, r1, lsl #24
 804f45c:	ea40 2002 	orr.w	r0, r0, r2, lsl #8
 804f460:	f403 437f 	and.w	r3, r3, #65280	; 0xff00
 804f464:	4318      	orrs	r0, r3
			IP4_ADDR(&ipaddr, a, b, c, d);
		}
	}

	return ipaddr;
}

The code looks similar but registers are used differently. The difference starts with
sub   sp, #28
sub   sp, #24

and as posted before, the non-zeroing version with -O1 produces that warning.