Andrew Donnellan <andrew.donnel...@au1.ibm.com> writes:
> (+ Nick)
>
> On 7/2/19 6:49 pm, Segher Boessenkool wrote:
>> On Thu, Feb 07, 2019 at 05:59:48PM +1100, Andrew Donnellan wrote:
>>> On 7/2/19 5:37 pm, Segher Boessenkool wrote:
>>>> On Thu, Feb 07, 2019 at 04:33:23PM +1100, Andrew Donnellan wrote:
>>>>> Some older gccs (<GCC 7), when invoked with -fsanitize-coverage=trace-pc,
>>>>> cause a spurious uninitialised variable warning in dt_cpu_ftrs.c:
>>>>>
>>>>> arch/powerpc/kernel/dt_cpu_ftrs.c: In function
>>>>> ‘cpufeatures_process_feature’:
>>>>> arch/powerpc/kernel/dt_cpu_ftrs.c:686:7: warning: ‘m’ may be used
>>>>> uninitialized in this function [-Wmaybe-uninitialized]
>>>>> if (m->cpu_ftr_bit_mask)
>>>>
>>>> It seems to me the warning is correct? If enable_unknown is false and no
>>>> cpu_feature is found, it will in
>>>>
>>>> if (m->cpu_ftr_bit_mask)
>>>> cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
>>>>
>>>> enable random features (whatever was last in the table), or indeed access
>>>> via NULL if the table is length 0? So maybe this should be
>>>>
>>>> if (known && m->cpu_ftr_bit_mask)
>>>> cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
>>>>
>>>> instead? (The code would be much clearer if all the known vs. !known
>>>> codepath was fully separated here).
>>>
>>> The table is never length 0, it's a statically defined array.
>>
>> Sure, and presumably that is why newer GCC doesn't warn. But what about
>> the other point? Is this code ever correct? Enabling random features
>> (in cur_cpu_spec->cpu_features) when the name isn't found seems wrong.
>
> Now that I'm replying without being 2 minutes before a meeting :)
>
> The warning is still spurious, but the logic looks very suspicious.
>
> I think your solution looks correct, though the whole function could be
> cleaned up a bit.
>
> I also notice that if enable_unknown == false, then I think an unknown
> feature will still print "enabling" and return true, which seems wrong.
>
> How does something like the following look, which I could send instead
> and will probably solve the spurious warnings issues anyway?
I'd prefer a minimal fix that we can backport. How about:
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c
b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 8be3721d9302..a1acccd25839 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -675,12 +675,10 @@ static bool __init cpufeatures_process_feature(struct
dt_cpu_feature *f)
}
}
- if (!known && enable_unknown) {
- if (!feat_try_enable_unknown(f)) {
- pr_info("not enabling: %s (unknown and unsupported by
kernel)\n",
- f->name);
- return false;
- }
+ if (!known && (!enable_unknown || !feat_try_enable_unknown(f))) {
+ pr_info("not enabling: %s (unknown and unsupported by
kernel)\n",
+ f->name);
+ return false;
}
if (m->cpu_ftr_bit_mask)
cheers
