[PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage

[Michael Ellerman]

Andrew Donnellan <andrew.donnellan at 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