While provable that length[0] is always initialized (because symCount
cannot be zero), upcoming gcc13 fails to recognize this and warns about
the unconditional use of the value immediately following the loop.

See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106511.

Reported-by: Martin Liška <martin.liska@suse.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC: We've cloned this code from Linux and the code is unchanged there.
     Therefore the same issue should exist there, and we may better get
     whatever workaround is going to be applied there. But I'm unaware
     of the issue, so far, having been observed in and reported against
     Linux. This may be because they disable the maybe-uninitialized
     warning by default, and they re-enable it only when building with
     W=2.

--- a/xen/common/bunzip2.c
+++ b/xen/common/bunzip2.c
@@ -233,7 +233,7 @@ static int __init get_next_block(struct
 		   becomes negative, so an unsigned inequality catches
 		   it.) */
 		t = get_bits(bd, 5)-1;
-		for (i = 0; i < symCount; i++) {
+		for (length[0] = i = 0; i < symCount; i++) {
 			for (;;) {
 				if (((unsigned)t) > (MAX_HUFCODE_BITS-1))
 					return RETVAL_DATA_ERROR;

On Fri, Mar 3, 2023 at 7:29 AM Jan Beulich <jbeulich@suse.com> wrote:

> While provable that length[0] is always initialized (because symCount
> cannot be zero), upcoming gcc13 fails to recognize this and warns about
> the unconditional use of the value immediately following the loop.
>
> See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106511.
>
> Reported-by: Martin Liška <martin.liska@suse.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> RFC: We've cloned this code from Linux and the code is unchanged there.
>      Therefore the same issue should exist there, and we may better get
>      whatever workaround is going to be applied there. But I'm unaware
>      of the issue, so far, having been observed in and reported against
>      Linux. This may be because they disable the maybe-uninitialized
>      warning by default, and they re-enable it only when building with
>      W=2.
>
> --- a/xen/common/bunzip2.c
> +++ b/xen/common/bunzip2.c
> @@ -233,7 +233,7 @@ static int __init get_next_block(struct
>                    becomes negative, so an unsigned inequality catches
>                    it.) */
>                 t = get_bits(bd, 5)-1;
> -               for (i = 0; i < symCount; i++) {
> +               for (length[0] = i = 0; i < symCount; i++) {
>

My main comment here is that nobody looking at this code will immediately
think, "Oh, I bet this is to work around a gcc bug that can't tell that
length[0] will always be initialized".  I'd put it in a separate line, with
a comment explaining the situation.

 -George

On 08.03.2023 11:37, George Dunlap wrote:
> On Fri, Mar 3, 2023 at 7:29 AM Jan Beulich <jbeulich@suse.com> wrote:
> 
>> While provable that length[0] is always initialized (because symCount
>> cannot be zero), upcoming gcc13 fails to recognize this and warns about
>> the unconditional use of the value immediately following the loop.
>>
>> See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106511.
>>
>> Reported-by: Martin Liška <martin.liska@suse.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> RFC: We've cloned this code from Linux and the code is unchanged there.
>>      Therefore the same issue should exist there, and we may better get
>>      whatever workaround is going to be applied there. But I'm unaware
>>      of the issue, so far, having been observed in and reported against
>>      Linux. This may be because they disable the maybe-uninitialized
>>      warning by default, and they re-enable it only when building with
>>      W=2.
>>
>> --- a/xen/common/bunzip2.c
>> +++ b/xen/common/bunzip2.c
>> @@ -233,7 +233,7 @@ static int __init get_next_block(struct
>>                    becomes negative, so an unsigned inequality catches
>>                    it.) */
>>                 t = get_bits(bd, 5)-1;
>> -               for (i = 0; i < symCount; i++) {
>> +               for (length[0] = i = 0; i < symCount; i++) {
>>
> 
> My main comment here is that nobody looking at this code will immediately
> think, "Oh, I bet this is to work around a gcc bug that can't tell that
> length[0] will always be initialized".  I'd put it in a separate line, with
> a comment explaining the situation.

I can certainly do so. The main question though needs answering first:
Are we okay to diverge from Linux here?

Jan