Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.03.2021 17:03, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"):
>> In this case the compiler is recognizing that no valid array indexes
>> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid,
>> ...)), but oddly enough isn't really consistent about the checking it
>> does (see the code comment).
> ...
>> -        if (this_cpu == cpu || x2apic_cluster(this_cpu) != 
>> x2apic_cluster(cpu))
>> +        if ( this_cpu == cpu )
>> +            continue;
>> +        /*
>> +         * Guard in particular against the compiler suspecting out-of-bounds
>> +         * array accesses below when NR_CPUS=1 (oddly enough with gcc 10 it
>> +         * is the 1st of these alone which actually helps, not the 2nd, nor
>> +         * are both required together there).
>> +         */
>> +        BUG_ON(this_cpu >= NR_CPUS);
>> +        BUG_ON(cpu >= NR_CPUS);
>> +        if ( x2apic_cluster(this_cpu) != x2apic_cluster(cpu) )
>>              continue;
> 
> Is there some particular reason for not putting the BUG_ON before the
> if test ?  That would avoid the refactoring.

I want it to be as close as possible to the place where the issue
is. I also view the refactoring as a good thing, since it allows
a style correction as a side effect.

> Of course putting it in next to the array indexing would address that
> too.

See my earlier reply to Roger's similar remark (which still was
along the lines of what I've said above).

Jan