| Message ID | 20171204144436.272626-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@arndb.de> The patch looks good to me, so Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> However I think it would also be useful to file a bug report for gcc, especially if older versions got this right. > --- > drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c index > be50445f9901..c50449882037 100644 > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > @@ -964,8 +964,6 @@ static int mdp5_plane_mode_set(struct drm_plane *plane, > uint32_t src_x, src_y; > uint32_t src_w, src_h; > uint32_t src_img_w, src_img_h; > - uint32_t src_x_r; > - int crtc_x_r; > int ret; > > nplanes = fb->format->num_planes; > @@ -1010,9 +1008,6 @@ static int mdp5_plane_mode_set(struct drm_plane > *plane, crtc_w /= 2; > src_w /= 2; > src_img_w /= 2; > - > - crtc_x_r = crtc_x + crtc_w; > - src_x_r = src_x + src_w; > } > > ret = calc_scalex_steps(plane, pix_format, src_w, crtc_w, step.x); > @@ -1052,9 +1047,9 @@ static int mdp5_plane_mode_set(struct drm_plane > *plane, if (right_hwpipe) > mdp5_hwpipe_mode_set(mdp5_kms, right_hwpipe, fb, &step, &pe, > config, hdecm, vdecm, hflip, vflip, > - crtc_x_r, crtc_y, crtc_w, crtc_h, > + crtc_x + crtc_w, crtc_y, crtc_w, crtc_h, > src_img_w, src_img_h, > - src_x_r, src_y, src_w, src_h); > + src_x + src_w, src_y, src_w, src_h); > > plane->fb = fb;
On Mon, Dec 4, 2017 at 5:36 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.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@arndb.de> > > The patch looks good to me, so > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c index be50445f9901..c50449882037 100644 --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c @@ -964,8 +964,6 @@ static int mdp5_plane_mode_set(struct drm_plane *plane, uint32_t src_x, src_y; uint32_t src_w, src_h; uint32_t src_img_w, src_img_h; - uint32_t src_x_r; - int crtc_x_r; int ret; nplanes = fb->format->num_planes; @@ -1010,9 +1008,6 @@ static int mdp5_plane_mode_set(struct drm_plane *plane, crtc_w /= 2; src_w /= 2; src_img_w /= 2; - - crtc_x_r = crtc_x + crtc_w; - src_x_r = src_x + src_w; } ret = calc_scalex_steps(plane, pix_format, src_w, crtc_w, step.x); @@ -1052,9 +1047,9 @@ static int mdp5_plane_mode_set(struct drm_plane *plane, if (right_hwpipe) mdp5_hwpipe_mode_set(mdp5_kms, right_hwpipe, fb, &step, &pe, config, hdecm, vdecm, hflip, vflip, - crtc_x_r, crtc_y, crtc_w, crtc_h, + crtc_x + crtc_w, crtc_y, crtc_w, crtc_h, src_img_w, src_img_h, - src_x_r, src_y, src_w, src_h); + src_x + src_w, src_y, src_w, src_h); plane->fb = fb;
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@arndb.de> --- drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)