Message-ID: <180b7dfa-268c-14c6-5e71-b5827b209e10@m4x.org>
Date:   Thu, 8 Sep 2016 01:02:37 +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 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

Powered by blists - more mailing lists