[Spice-devel] [PATCH 11/11] dcc: remove not necessary volatile specifications

[Frediano Ziglio]

> 
> [NB: this did not go to the ML]
> On Fri, Feb 12, 2016 at 05:12:57AM -0500, Frediano Ziglio wrote:
> > > 
> > > This causes:
> > > make[4]: Entering directory '/home/teuf/redhat/spice/server'
> > >   CC       dcc.lo
> > > dcc.c: In function 'dcc_compress_image_jpeg':
> > > dcc.c:852:26: error: variable 'jpeg_in_type' might be clobbered by
> > > 'longjmp'
> > > or 'vfork' [-Werror=clobbered]
> > >      JpegEncoderImageType jpeg_in_type;
> > >                           ^
> > > dcc.c:854:9: error: variable 'has_alpha' might be clobbered by 'longjmp'
> > > or
> > > 'vfork' [-Werror=clobbered]
> > >      int has_alpha = FALSE;
> > >          ^
> > > dcc.c: In function 'dcc_compress_image_quic.isra.10':
> > > dcc.c:1020:19: error: variable 'type' might be clobbered by 'longjmp' or
> > > 'vfork' [-Werror=clobbered]
> > >      QuicImageType type;
> > >                    ^
> > > 
> > > not sure why...
> > > 
> > 
> > Sometimes compilers have these limits :)
> > 
> > I think in this case it assumes (for strange reasons) that a longjmp can
> > go back to a point and change the variable without code noticing.
> > 
> > From man
> > 
> >        The values of automatic variables are unspecified after a call to
> >        longjmp() if they meet all the following criteria:
> >        ·  they are local to the function that made the corresponding
> >        setjmp(3) call;
> >        ·  their values are changed between the calls to setjmp(3) and
> >        longjmp(); and
> >        ·  they are not declared as volatile.
> > 
> > So compiler did detect a possible condition 2 ("their values are changed
> > "...)
> 
> hmm but they are not actually changed between the setjmp/longjmp calls,
> are they? Could be a compiler issue then? Though they are saying 'might'
> 
> Christophe
> 

This (ugly) patch solve the issue...


diff --git a/server/dcc.c b/server/dcc.c
index bd25069..afd01e5 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -849,9 +849,9 @@ static int dcc_compress_image_jpeg(DisplayChannelClient *dcc, SpiceImage *dest,
     LzData *lz_data = &dcc->lz_data;
     JpegEncoderContext *jpeg = dcc->jpeg;
     LzContext *lz = dcc->lz;
-    JpegEncoderImageType jpeg_in_type;
+    JpegEncoderImageType jpeg_in_type_tmp;
     int jpeg_size = 0;
-    int has_alpha = FALSE;
+    int has_alpha_tmp = FALSE;
     int alpha_lz_size = 0;
     int comp_head_filled;
     int comp_head_left;
@@ -862,22 +862,25 @@ static int dcc_compress_image_jpeg(DisplayChannelClient *dcc, SpiceImage *dest,
 
     switch (src->format) {
     case SPICE_BITMAP_FMT_16BIT:
-        jpeg_in_type = JPEG_IMAGE_TYPE_RGB16;
+        jpeg_in_type_tmp = JPEG_IMAGE_TYPE_RGB16;
         break;
     case SPICE_BITMAP_FMT_24BIT:
-        jpeg_in_type = JPEG_IMAGE_TYPE_BGR24;
+        jpeg_in_type_tmp = JPEG_IMAGE_TYPE_BGR24;
         break;
     case SPICE_BITMAP_FMT_32BIT:
-        jpeg_in_type = JPEG_IMAGE_TYPE_BGRX32;
+        jpeg_in_type_tmp = JPEG_IMAGE_TYPE_BGRX32;
         break;
     case SPICE_BITMAP_FMT_RGBA:
-        jpeg_in_type = JPEG_IMAGE_TYPE_BGRX32;
-        has_alpha = TRUE;
+        jpeg_in_type_tmp = JPEG_IMAGE_TYPE_BGRX32;
+        has_alpha_tmp = TRUE;
         break;
     default:
         return FALSE;
     }
 
+    const JpegEncoderImageType jpeg_in_type = jpeg_in_type_tmp;
+    const int has_alpha = has_alpha_tmp;
+
     encoder_data_init(&jpeg_data->data, dcc);
 
     if (setjmp(jpeg_data->data.jmp_env)) {
@@ -1017,28 +1020,30 @@ static int dcc_compress_image_quic(DisplayChannelClient *dcc, SpiceImage *dest,
 {
     QuicData *quic_data = &dcc->quic_data;
     QuicContext *quic = dcc->quic;
-    QuicImageType type;
+    QuicImageType type_tmp;
     int size, stride;
     stat_start_time_t start_time;
     stat_start_time_init(&start_time, &DCC_TO_DC(dcc)->quic_stat);
 
     switch (src->format) {
     case SPICE_BITMAP_FMT_32BIT:
-        type = QUIC_IMAGE_TYPE_RGB32;
+        type_tmp = QUIC_IMAGE_TYPE_RGB32;
         break;
     case SPICE_BITMAP_FMT_RGBA:
-        type = QUIC_IMAGE_TYPE_RGBA;
+        type_tmp = QUIC_IMAGE_TYPE_RGBA;
         break;
     case SPICE_BITMAP_FMT_16BIT:
-        type = QUIC_IMAGE_TYPE_RGB16;
+        type_tmp = QUIC_IMAGE_TYPE_RGB16;
         break;
     case SPICE_BITMAP_FMT_24BIT:
-        type = QUIC_IMAGE_TYPE_RGB24;
+        type_tmp = QUIC_IMAGE_TYPE_RGB24;
         break;
     default:
         return FALSE;
     }
 
+    const QuicImageType type = type_tmp;
+
     encoder_data_init(&quic_data->data, dcc);
 
     if (setjmp(quic_data->data.jmp_env)) {



No, I'm not proposing it, just saying that perhaps the compiler is too cautious.
On the other side volatile == keep in memory and don't optimize... weird!

Frediano