Date:   Sun, 11 Sep 2016 23:16:46 +0200
From:   Nicolas Iooss <nicolas.iooss_linux@....org>
To:     Dave Gordon <david.s.gordon@...el.com>
Cc:     "Vetter, Daniel" <daniel.vetter@...el.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        "intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [Intel-gfx] [PATCH 1/1] drm/i915/dsi: silence a warning about
 uninitialized return value

On 08/09/16 16:31, Dave Gordon wrote:
> On 08/09/16 00:02, Nicolas Iooss wrote:
>> On 07/09/16 18:03, Dave Gordon wrote:
>>> On 06/09/16 21:36, Nicolas Iooss wrote:
>>>> 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.
>>>
>>> So it does, In that case the complaint is really quite reasonable.
>>>
>>>>> 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.
>>>
>>> It probably should have, since the (hidden) if() could still result in
>>> this function never being called. Oh well ...
>>
>> Sorry, my message was not clear enough. The compiler did not complain in
>> mipi_dsi_dcs_read1() in the last case, but the -Wsometimes-uninitialized
>> warning was still there for variable 'data' in dcs_get_backlight(), as
>> expected because of the "hidden if".
>>
>> Nicolas
> 
> OK, thanks.
> 
> BTW do you see any "may be used uninitialised" warnings in
> gen{6,8}_ggtt_insert_entries()? In particular
> 
>     for_each_sgt_dma(addr, sgt_iter, st) {
>         gtt_entry = gen8_pte_encode(addr, level, true);
>         gen8_set_pte(&gtt_entries[i++], gtt_entry);
>     }
> 
> [snip]
> 
>     if (i != 0)
>         WARN_ON(readq(&gtt_entries[i-1]) != gtt_entry);
> 
> Or maybe clang is smart enough to realise that the WARN_ON() is
> reachable only if the gen8_set_pte() has already been executed at least
> once?
> 
> .Dave.

The clang I am using (version 3.8.1) does not report any
-Wsometimes-uninitialized warning in
drivers/gpu/drm/i915/i915_gem_gtt.c. Even when I introduce a statement
which uses gtt_entry right after the for_each_sgt_dma loop (like
printk("%llu", gtt_entry);) no warning is produced.
Therefore the lack of the warning does not mean the compiler is finding
that i != 0 only when the loop got executed once, I guess the loop
condition is too complex for the compiler to decide whether the loop
body is always executed.

Nicolas

Powered by blists - more mailing lists