Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190308012023.5709-1-natechancellor@gmail.com> <155203363088.27405.11887329278848171622@skylake-alporthouse-com>
In-Reply-To: <155203363088.27405.11887329278848171622@skylake-alporthouse-com>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Fri, 8 Mar 2019 11:26:10 -0800
Message-ID: <CAKwvOd=mpKR2V9rOEDDytctxT7uSwREyqfDVrYEEessWL=DY8Q@mail.gmail.com>
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Zero initialize this_cpu in busywait_stop
To:     Chris Wilson <chris@chris-wilson.co.uk>
Cc:     Jani Nikula <jani.nikula@linux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Nathan Chancellor <natechancellor@gmail.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        intel-gfx@lists.freedesktop.org,
        LKML <linux-kernel@vger.kernel.org>,
        dri-devel@lists.freedesktop.org,
        clang-built-linux@googlegroups.com,
        Greg KH <gregkh@linuxfoundation.org>,
        Sandeep Patil <sspatil@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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