Re: [Intel-gfx] [PATCH 1/1] drm/i915/dsi: silence a warning about uninitialized return value

[Dave Gordon <david.s.gordon@intel.com>]

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@m4x.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.