> > On 01/23/2018 01:27 PM, Frediano Ziglio wrote: > > ping > > > > Currently master is failing to compiler, see > > https://gitlab.com/spice/spice-gtk/-/jobs > > > > This is supposed to be the patch in spice-gtk that take the spice-common > > changes: > > https://lists.freedesktop.org/archives/spice-devel/2018-January/041472.html > > > > Frediano > > > >> > >> Due to different warning setting some GCC reports: > >> > >> In file included from ../spice-common/common/sw_canvas.c:27:0, > >> from client_sw_canvas.c:20: > >> ../spice-common/common/canvas_base.c: In function ‘canvas_get_lz’: > >> ../spice-common/common/canvas_base.c:768:13: error: ‘palette’ may be used > >> uninitialized in this function [-Werror=maybe-uninitialized] > >> free(palette); > >> ^~~~~~~~~~~~~ > >> ../spice-common/common/canvas_base.c:764:9: error: variable ‘free_palette’ > >> might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered] > >> int free_palette = FALSE; > >> ^~~~~~~~~~~~ > >> cc1: all warnings being treated as errors > >> > >> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx> > >> --- > >> common/canvas_base.c | 8 +++++--- > >> 1 file changed, 5 insertions(+), 3 deletions(-) > >> > >> diff --git a/common/canvas_base.c b/common/canvas_base.c > >> index 42f0eea..90d1d38 100644 > >> --- a/common/canvas_base.c > >> +++ b/common/canvas_base.c > >> @@ -754,14 +754,14 @@ static pixman_image_t *canvas_get_lz(CanvasBase > >> *canvas, SpiceImage *image, > >> uint8_t *decomp_buf = NULL; > >> pixman_format_code_t pixman_format; > >> LzImageType type, as_type; > >> - SpicePalette *palette; > >> + SpicePalette *palette = NULL; > >> int n_comp_pixels; > >> int width; > >> int height; > >> int top_down; > >> int stride_encoded; > >> int stride; > >> - int free_palette = FALSE; > >> + volatile int free_palette = FALSE; > > Hi Frediano, > Why does free_palette have to be volatile ? Is quite complicated, has to do with life time optimizations and internal compiler details. Basically the compiler is not much sure if free_palette can be optimized out but the pointer is still alive. Using volatile fix this logic. I suppose is more a "maybe is a disaster" than a real issue. volatile make sure the variable is life (and the pointer to it too) while the jmpbuf can be used. > Isn't it enough to initialize it ? > What's special with free_palette compared to other > variables ? > No idea exactly. Some compiler version reports the warning others not. But in this case looking at how is used and how many time is not a big issue (cannot be optimized out much or should not and cannot be put in a register). > You may want to remove the free_palette = FALSE; line that > exists few lines below in the code (not shown here). > Sorry, I cannot find this line you are mentioning. > Do we need to add an if (free_palette) free() to > the if setjmp() block ? > Yes, potentially longjmp can occur after the palette is allocated and you want to free it to avoid the leak (fixed by "canvas: Remove possible leak on LZ decompression failure"). > Thanks, > Uri. > > >> > >> if (setjmp(lz_data->jmp_env)) { > >> if (free_palette) { > >> @@ -781,7 +781,9 @@ static pixman_image_t *canvas_get_lz(CanvasBase > >> *canvas, > >> SpiceImage *image, > >> spice_return_val_if_fail(image->u.lz_plt.data->num_chunks == 1, > >> NULL); /* TODO: Handle chunks */ > >> comp_buf = image->u.lz_plt.data->chunk[0].data; > >> comp_size = image->u.lz_plt.data->chunk[0].len; > >> - palette = canvas_get_localized_palette(canvas, > >> image->u.lz_plt.palette, image->u.lz_plt.palette_id, > >> image->u.lz_plt.flags, > >> &free_palette); > >> + palette = canvas_get_localized_palette(canvas, > >> image->u.lz_plt.palette, > >> + > >> image->u.lz_plt.palette_id, > >> image->u.lz_plt.flags, > >> + (int*) &free_palette); > >> } else { > >> spice_warn_if_reached(); > >> return NULL; > Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel