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

[Arnd Bergmann]

On Mon, Dec 4, 2017 at 5:36 PM, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> 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@xxxxxxxx>
>
> The patch looks good to me, so
>
> Acked-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> 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