From: Dave Gordon <david.s.gordon@intel.com>
To: Nicolas Iooss <nicolas.iooss_linux@m4x.org>,
"Vetter, Daniel" <daniel.vetter@intel.com>,
Jani Nikula <jani.nikula@linux.intel.com>,
"intel-gfx@lists.freedesktop.org"
<intel-gfx@lists.freedesktop.org>,
"dri-devel@lists.freedesktop.org"
<dri-devel@lists.freedesktop.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [Intel-gfx] [PATCH 1/1] drm/i915/dsi: silence a warning about uninitialized return value
Date: Wed, 7 Sep 2016 17:03:49 +0100 [thread overview]
Message-ID: <bc188050-ddd9-1d61-013c-7735748f9671@intel.com> (raw)
In-Reply-To: <0030b427-a704-e67c-3ca7-3887a8cbaa7f@m4x.org>
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 ...
.Dave.
>> Here's an R-b for your fix anyway ...
>>
>> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
>
> Thanks!
> Nicolas
>
> [1] I used "make drivers/gpu/drm/i915/intel_dsi_dcs_backlight.i" to see
> the output of the preprocessor.
next prev parent reply other threads:[~2016-09-07 16:03 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-04 18:58 [PATCH 1/1] drm/i915/dsi: silence a warning about uninitialized return value Nicolas Iooss
2016-09-06 10:21 ` [Intel-gfx] " Dave Gordon
2016-09-06 20:36 ` Nicolas Iooss
2016-09-07 16:03 ` Dave Gordon [this message]
2016-09-07 23:02 ` Nicolas Iooss
2016-09-08 14:31 ` Dave Gordon
2016-09-11 21:16 ` Nicolas Iooss
2016-10-23 16:55 ` Nicolas Iooss
2016-10-23 17:22 ` [Intel-gfx] " Chris Wilson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bc188050-ddd9-1d61-013c-7735748f9671@intel.com \
--to=david.s.gordon@intel.com \
--cc=daniel.vetter@intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=nicolas.iooss_linux@m4x.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).