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

[Christophe Fergeau]

On Fri, Feb 12, 2016 at 06:14:06AM -0500, Frediano Ziglio wrote:
> > 
> > [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;

What about const JpegEncoderImageType jpeg_in_type = get_jpeg_in_type(); ?
ie a mix of the previous patch and this one. Alternatively, we could
just go with the previous one. Though we are only cheating with gcc
here, so that it stops thinking things can be modified.

Christophe

> +
>      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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160212/3c9174ab/attachment.sig>