On 29.01.2021 19:18, Andrew Cooper wrote:
> On 29/01/2021 09:46, Jan Beulich wrote:
>> On 29.01.2021 00:44, Andrew Cooper wrote:
>>> On 15/01/2021 16:12, Jan Beulich wrote:
>>>> On 12.01.2021 20:48, Andrew Cooper wrote:
>>>>> --- a/xen/arch/x86/mm.c
>>>>> +++ b/xen/arch/x86/mm.c
>>>>> @@ -4628,7 +4628,6 @@ int arch_acquire_resource(struct domain *d,
>>>>> unsigned int type,
>>>>> if ( id != (unsigned int)ioservid )
>>>>> break;
>>>>>
>>>>> - rc = 0;
>>>>> for ( i = 0; i < nr_frames; i++ )
>>>>> {
>>>>> mfn_t mfn;
>>>> How "good" are our chances that older gcc won't recognize that
>>>> without this initialization ...
>>>>
>>>>> @@ -4639,6 +4638,9 @@ int arch_acquire_resource(struct domain *d,
>>>>> unsigned int type,
>>>>>
>>>>> mfn_list[i] = mfn_x(mfn);
>>>>> }
>>>>> + if ( i == nr_frames )
>>>>> + /* Success. Passed nr_frames back to the caller. */
>>>>> + rc = nr_frames;
>>>> ... rc will nevertheless not remain uninitialized when nr_frames
>>>> is zero?
>>> I don't anticipate this function getting complicated enough for us to
>>> need to rely on tricks like that to spot bugs.
>>>
>>> AFAICT, it would take a rather larger diffstat to make it "uninitialised
>>> clean" again.
>> Well, we'll see how it goes then. We still allow rather ancient
>> gcc, and I will eventually run into a build issue here once
>> having rebased accordingly, if such old gcc won't cope.
>
> But those problematic compilers have problems spotting that a variable
> is actually initialised on all paths.
>
> This case is opposite. It is unconditionally initialised to -EINVAL
> (just outside of the context above), which breaks some "the compiler
> would warn us about error paths" logic that we try to use.
Oh, right you are. I'm sorry for the noise then.
Jan
