| Message ID | 20190308012023.5709-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Zero initialize this_cpu in busywait_stop | expand |
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
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;
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;
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(-)