From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752399AbcIGXCv (ORCPT ); Wed, 7 Sep 2016 19:02:51 -0400 Received: from mail-wm0-f65.google.com ([74.125.82.65]:32859 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751334AbcIGXCm (ORCPT ); Wed, 7 Sep 2016 19:02:42 -0400 Subject: Re: [Intel-gfx] [PATCH 1/1] drm/i915/dsi: silence a warning about uninitialized return value To: Dave Gordon References: <20160904185809.31916-1-nicolas.iooss_linux@m4x.org> <7d5589b2-d59c-8092-ba38-5e791ea7ca7d@intel.com> <0030b427-a704-e67c-3ca7-3887a8cbaa7f@m4x.org> Cc: "Vetter, Daniel" , Jani Nikula , "intel-gfx@lists.freedesktop.org" , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" From: Nicolas Iooss Message-ID: <180b7dfa-268c-14c6-5e71-b5827b209e10@m4x.org> Date: Thu, 8 Sep 2016 01:02:37 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>> >>> 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