Message-ID: <0030b427-a704-e67c-3ca7-3887a8cbaa7f@m4x.org>
Date:   Tue, 6 Sep 2016 22:36:28 +0200
From:   Nicolas Iooss <nicolas.iooss_linux@....org>
To:     Dave Gordon <david.s.gordon@...el.com>,
        Daniel Vetter <daniel.vetter@...el.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [Intel-gfx] [PATCH 1/1] drm/i915/dsi: silence a warning about
 uninitialized return value

On 06/09/16 12:21, Dave Gordon wrote:
> On 04/09/16 19:58, Nicolas Iooss wrote:
>> When building the kernel with clang and some warning flags, the compiler
>> reports that the return value of dcs_get_backlight() may be
>> uninitialized:
>>
>>     drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:53:2: error: variable
>>     'data' is used uninitialized whenever 'for' loop exits because its
>>     condition is false [-Werror,-Wsometimes-uninitialized]
>>             for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {
>>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>     drivers/gpu/drm/i915/intel_dsi.h:126:49: note: expanded from macro
>>     'for_each_dsi_port'
>>     #define for_each_dsi_port(__port, __ports_mask)
>>                                 for_each_port_masked(__port,
>> __ports_mask)
>>                                
>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>     drivers/gpu/drm/i915/i915_drv.h:322:26: note: expanded from macro
>>     'for_each_port_masked'
>>         for ((__port) = PORT_A; (__port) < I915_MAX_PORTS; (__port)++)  \
>>                                 ^~~~~~~~~~~~~~~~~~~~~~~~~
>>     drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:60:9: note:
>>     uninitialized use occurs here
>>             return data;
>>                    ^~~~
>>
>> As intel_dsi->dcs_backlight_ports seems to be always initialized to a
>> non-null value, the content of the for loop is always executed and there
>> is no bug in the current code. Nevertheless the compiler has no way of
>> knowing that assumption, so initialize variable 'data' to silence the
>> warning here.
>>
>> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@....org>
> 
> Interesting ... there are two things that could lead to this (possibly)
> incorrect analysis. Either it thinks the loop could be executed zero
> times, which would be a deficiency in the compiler, as the initialiser
> and loop bound are both known (different) constants:
> 
> enum port {
>         PORT_A = 0,
>         PORT_B,
>         PORT_C,
>         PORT_D,
>         PORT_E,
>         I915_MAX_PORTS
> };
> 
> or, it doesn't understand that because we've passed &data to another
> function, it can have been set by the callee. It may be extra confusing
> that the callee takes (void *); or it may be being ultra-sophisticated
> in its analysis and noted that in one error path data is *not* set (and
> we then discard the error and use data anyway). As an experiment, you
> could try:

The code that the compiler sees is not a simple loop other enum 'port'
but "for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {", which
is expanded [1] to:

    for ((port) = PORT_A; (port) < I915_MAX_PORTS; (port)++)
      if (!((intel_dsi->dcs_backlight_ports) & (1 << (port)))) {} else {

This is why I spoke of intel_dsi->dcs_backlight_ports in my description:
if it is zero, the body of the loop is never run.

As for the analyses of calls using &data, clang does not warn about the
variable being maybe uninitialized following a call. This is quite
expected as this would lead to too many false positives, even though it
may miss some bugs.

> 
> static u8 mipi_dsi_dcs_read1(struct mipi_dsi_device *dsi_device, u8 cmd)
> {
>         u8 data = 0;
> 
>         mipi_dsi_dcs_read(dsi_device, cmd, &data, sizeof(data));
> 
>         return data;
> }
> 
> static u32 dcs_get_backlight(struct intel_connector *connector)
> {
>         struct intel_encoder *encoder = connector->encoder;
>         struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>         struct mipi_dsi_device *dsi_device;
>         enum port port;
>         u8 data;
> 
>         /* FIXME: Need to take care of 16 bit brightness level */
>         for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {
>                 dsi_device = intel_dsi->dsi_hosts[port]->device;
>                 data = mipi_dsi_dcs_read1(dsi_device,
> MIPI_DCS_GET_DISPLAY_BRIGHTNESS);
>                 break;
>         }
> 
>         return data;
> }
> 
> If it complains about that then it's a shortcoming in the loop analysis.

It complains (in dcs_get_backlight), because for_each_dsi_port() still
hides an 'if' condition.

> If not you could try:
> 
> static u8 mipi_dsi_dcs_read1(struct mipi_dsi_device *dsi_device, u8 cmd)
> {
>         u8 data;
>     ssize_t nbytes = sizeof(data);
> 
>     nbytes = mipi_dsi_dcs_read(dsi_device, cmd, &data, nbytes);
>     return nbytes == sizeof(data) ? data : 0;
> }
> 
> and if complains about that then it doesn't understand that passing
> &data allows it to be set. If it doesn't complain about this version,
> then the original error was actually correct, in the sense that data can
> indeed be used uninitialised if certain error paths can be taken.

clang did not complain with this last case.

> Here's an R-b for your fix anyway ...
> 
> Reviewed-by: Dave Gordon <david.s.gordon@...el.com>

Thanks!

Nicolas


[1] I used "make drivers/gpu/drm/i915/intel_dsi_dcs_backlight.i" to see
the output of the preprocessor.

Powered by blists - more mailing lists