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

Andrew Donnellan [off-list ref] writes:

(+ Nick)

On 7/2/19 6:49 pm, Segher Boessenkool wrote:

quoted

On Thu, Feb 07, 2019 at 05:59:48PM +1100, Andrew Donnellan wrote:

quoted

On 7/2/19 5:37 pm, Segher Boessenkool wrote:

quoted

On Thu, Feb 07, 2019 at 04:33:23PM +1100, Andrew Donnellan wrote:

quoted

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