linux-kernel - Re: [PATCH] drm: msm: avoid false-positive -Wmaybe-uninitialized warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAK8P3a2WP5x7CcHQ5U=D2wWPNZ=kQHxkphXVv5nLOYqOizvELg@mail.gmail.com>
Date:   Mon, 4 Dec 2017 17:58:50 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Rob Clark <robdclark@...il.com>, David Airlie <airlied@...ux.ie>,
        Archit Taneja <architt@...eaurora.org>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Liviu Dudau <Liviu.Dudau@....com>,
        Gustavo Padovan <gustavo.padovan@...labora.com>,
        linux-arm-msm@...r.kernel.org,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        freedreno@...ts.freedesktop.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm: msm: avoid false-positive -Wmaybe-uninitialized warning

On Mon, Dec 4, 2017 at 5:36 PM, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
> Hi Arnd,
>
> Thank you for the patch.
>
> On Monday, 4 December 2017 16:44:23 EET Arnd Bergmann wrote:
>> gcc-8 -fsanitize-coverage=trace-pc produces a false-positive warning:
>>
>> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c: In function
>> 'mdp5_plane_mode_set.isra.8':
>> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c:1053:3: error: 'crtc_x_r' may be
>> used uninitialized in this function [-Werror=maybe-uninitialized]
>>
>> It's relatively clear from reading the source that this cannot happen,
>> and older compilers get it right. This rearranges the code remove
>> the two affected variables, which reliably avoids the problem.
>>
>> Signed-off-by: Arnd Bergmann <arnd@...db.de>
>
> The patch looks good to me, so
>
> Acked-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
>
> However I think it would also be useful to file a bug report for gcc,
> especially if older versions got this right.

I was rather close to it, and even spent time on a reduced test case
with "creduce", which came down to

int drm_rect_width_r_0, calc_phase_step_src, calc_scalex_steps_ret,
    calc_scalex_steps_dest, calc_scaley_steps_ret, calc_scaley_steps_dest,
    mdp5_plane_mode_set___trans_tmp_2;
struct mdp5_hw_pipe {
  int caps;
} * mdp5_plane_mode_set_right_hwpipe;
int fn1(int p1) {
  if (calc_phase_step_src || p1 == 0)
    return 2;
  if (calc_phase_step_src > p1)
    return 5;
  return 0;
}
int fn2() {
  struct mdp5_hw_pipe hwpipe = hwpipe;
  int src_x_r;
  if (mdp5_plane_mode_set_right_hwpipe)
    src_x_r = drm_rect_width_r_0;
  calc_scalex_steps_ret = fn1(calc_scalex_steps_dest);
  if (calc_scalex_steps_ret)
    return calc_scalex_steps_ret;
  calc_scaley_steps_ret = fn1(calc_scaley_steps_dest);
  if (calc_scaley_steps_ret)
    return calc_scaley_steps_ret;
  if (hwpipe.caps)
    if (mdp5_plane_mode_set_right_hwpipe)
      mdp5_plane_mode_set___trans_tmp_2 = src_x_r;
  return calc_scaley_steps_ret;
}

This is still not something that is "obviously" wrong, it seems rather
that gcc can't keep track of enough state at the same time, which
is a fundamental problem but also a bit unpredictable.

I've seen many false-positive (and also false-negative) -Wmaybe-uninitialized
warnings that are likely easier to fix than this particular one, so I
ended up not reporting it.

        Arnd