[PATCH] drm/i915: Zero initialize this_cpu in busywait_stop

[PATCH] drm/i915: Zero initialize this_cpu in busywait_stop

[Nathan Chancellor]

When building with -Wsometimes-uninitialized, Clang warns:

drivers/gpu/drm/i915/i915_request.c:1032:6: warning: variable 'this_cpu'
is used uninitialized whenever '&&' condition is false
[-Wsometimes-uninitialized]

time_after expands to use two typecheck with logical ANDs between them.
typecheck evaluates to 1 but Clang clearly gets confused with the logic
that as semantic analysis happens early in the pipeline. Fix this by
just zero initializing this_cpu as it will always be properly
initialized before the comparison below.

Link: https://github.com/ClangBuiltLinux/linux/issues/415
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---

Alternatively, this can be solved by having the return value of
local_clock_us(&this_cpu) be a local variable but this seems less
controversial.

 drivers/gpu/drm/i915/i915_request.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index c2a5c48c7541..06c0c952191f 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -1027,7 +1027,7 @@ static unsigned long local_clock_us(unsigned int *cpu)
 
 static bool busywait_stop(unsigned long timeout, unsigned int cpu)
 {
-	unsigned int this_cpu;
+	unsigned int this_cpu = 0;
 
 	if (time_after(local_clock_us(&this_cpu), timeout))
 		return true;
-- 
2.21.0

Re: [Intel-gfx] [PATCH] drm/i915: Zero initialize this_cpu in busywait_stop

[Chris Wilson]

Quoting Nathan Chancellor (2019-03-08 01:20:24)
> When building with -Wsometimes-uninitialized, Clang warns:
> 
> drivers/gpu/drm/i915/i915_request.c:1032:6: warning: variable 'this_cpu'
> is used uninitialized whenever '&&' condition is false
> [-Wsometimes-uninitialized]
> 
> time_after expands to use two typecheck with logical ANDs between them.
> typecheck evaluates to 1 but Clang clearly gets confused with the logic
> that as semantic analysis happens early in the pipeline. Fix this by
> just zero initializing this_cpu as it will always be properly
> initialized before the comparison below.
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/415
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> 
> Alternatively, this can be solved by having the return value of
> local_clock_us(&this_cpu) be a local variable but this seems less
> controversial.

I'll just wait for clang to be fixed, as this severely undermines any
respect I have for its semantic analysis.
-Chris

Re: [Intel-gfx] [PATCH] drm/i915: Zero initialize this_cpu in busywait_stop

[Nick Desaulniers]

On Fri, Mar 8, 2019 at 12:27 AM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Quoting Nathan Chancellor (2019-03-08 01:20:24)
> > When building with -Wsometimes-uninitialized, Clang warns:
> >
> > drivers/gpu/drm/i915/i915_request.c:1032:6: warning: variable 'this_cpu'
> > is used uninitialized whenever '&&' condition is false
> > [-Wsometimes-uninitialized]
> >
> > time_after expands to use two typecheck with logical ANDs between them.
> > typecheck evaluates to 1 but Clang clearly gets confused with the logic
> > that as semantic analysis happens early in the pipeline. Fix this by
> > just zero initializing this_cpu as it will always be properly
> > initialized before the comparison below.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/415
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >
> > Alternatively, this can be solved by having the return value of
> > local_clock_us(&this_cpu) be a local variable but this seems less
> > controversial.
>
> I'll just wait for clang to be fixed, as this severely undermines any
> respect I have for its semantic analysis.
> -Chris

I'm still playing around with this in Godbolt (my hunch is that GNU C
statement expressions are maybe inlined as part of GCC's early
inlining phase).  For example:
https://godbolt.org/z/G54s5z

If you change `typecheck(unsigned long, a)` and `typecheck(unsigned
long, b)` in `time_after()` both to `1` (what `typecheck` would
evaluate to), then the warning goes away.  But a further
simplification shows that GNU C statement expressions are not the
problem:
https://godbolt.org/z/KzCN8m

I need to keep investigating, but there may be more we can do on the
compiler side.

It seems that another workaround that avoid default initialization is
to just create another local for the temporary expression that
provably initialized this_cpu, ie.

diff --git a/drivers/gpu/drm/i915/i915_request.c
b/drivers/gpu/drm/i915/i915_request.c
index c2a5c48c7541..5b90b5c35c8b 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -1028,8 +1028,9 @@ static unsigned long local_clock_us(unsigned int *cpu)
 static bool busywait_stop(unsigned long timeout, unsigned int cpu)
 {
        unsigned int this_cpu;
+       unsigned long local_clock = local_clock_us(&this_cpu);

-       if (time_after(local_clock_us(&this_cpu), timeout))
+       if (time_after(local_clock, timeout))
                return true;

        return this_cpu != cpu;

-- 
Thanks,
~Nick Desaulniers