[Piglit] [PATCH 2/3] Use alloca instead of variable length arrays

Wed Feb 11 08:12:03 PST 2015

On Tue, 2015-02-10 at 10:28 -0800, Dylan Baker wrote:
> I just want to be clear I was asking a question, I don't really care one
> way or another, I would just rather not see code churn if it doesn't
> actually buy us anything.

Not sure what the question is here. The idea is to force msvc like
limitations on other compilers by using -Werror= for unsupported
features. It should result in fewer commits like [0].
We can do it
a) globally even for stuff that is never built using msvc
b) per directory

I think a) is better given that the required changes are minimal (only
the two posted patches), and using alloca instead gives identical
behavior. it makes codestyle consistent across files, and Jose's way of
removing the flags using string function seems a bit hacky to me

jan

[0]
http://cgit.freedesktop.org/piglit/commit/?id=0784e54b75918b94ac974197b736c53ccba69ff1

> 
> Dylan
> 
> On Mon, Feb 09, 2015 at 10:57:31AM -0500, Jan Vesely wrote:
> > On Fri, 2015-02-06 at 19:46 +0000, Jose Fonseca wrote:
> > > I haven't tried this sort of code with MSVC 2013 U4, but at least in the 
> > > past, MSVC 2013 was refusing certain kinds of variable length arrays.
> > > 
> > > And Jan's patch is a stepping stone to -Wvla option  when compiling 
> > > (option which apply to the whole tree, including parts that are not 
> > > built with MSVC), in order to preemptively spot uses of VLA in places 
> > > where MSVC would break.
> > > 
> > > So let me do the following: I'll see if MSVC 2013 accepts or refuses the 
> > > VLA code that caused problems the first time, and then we'll take it 
> > > from there.
> > 
> > thank you. getting rid of -Wvla entirely would be preferable.
> > 
> > Dylan, I have attached a follow up patch that I planned to post after
> > these get in. I probably should have included it in the original series
> > to avoid the confusion.
> > 
> > jan
> > 
> > > 
> > > Jose
> > > 
> > > 
> > > On 06/02/15 19:19, Dylan Baker wrote:
> > > > Is this actually necessary? I though that we required MSVC 2013 u4,
> > > > because it has c99 support.
> > > >
> > > > Also, dma_buf tests require libdrm_intel, which doesn't exist/work on
> > > > windows, so I don't think anyone will be building them with msvc anytime
> > > > soon.
> > > >
> > > > Dylan
> > > >
> > > > On Fri, Feb 06, 2015 at 11:49:08AM -0500, Jan Vesely wrote:
> > > >> ping
> > > >>
> > > >> On Fri, 2014-12-12 at 19:13 -0500, Jan Vesely wrote:
> > > >>> On Fri, 2014-12-12 at 14:49 -0800, Matt Turner wrote:
> > > >>>> On Fri, Dec 12, 2014 at 2:13 PM, Jan Vesely <jan.vesely at rutgers.edu> wrote:
> > > >>>>> On Fri, 2014-12-12 at 13:37 -0800, Matt Turner wrote:
> > > >>>>>> On Fri, Dec 12, 2014 at 1:23 PM, Jan Vesely <jan.vesely at rutgers.edu> wrote:
> > > >>>>>>> On Fri, 2014-12-12 at 12:58 -0800, Matt Turner wrote:
> > > >>>>>>>> I'm curious what the motivation for removing variably-sized arrays is,
> > > >>>>>>>> but if I accept that that's a good thing to do then the first patch
> > > >>>>>>>> makes sense, but I don't understand this one.
> > > >>>>>>>>
> > > >>>>>>>> How is a variably-size array different from using alloca()?
> > > >>>>>>>
> > > >>>>>>> variable size arrays are a c99 feature not supported by msvc (that's why
> > > >>>>>>> there is a warning). I don't know which parts actually do need to build
> > > >>>>>>> using msvc, but it seemed like a good idea to reduce warning output (and
> > > >>>>>>> improve consistency with code that needs to build using msvc).
> > > >>>>>>>
> > > >>>>>>> In the first patch I used alloca+free, because it looked nicer than
> > > >>>>>>> doing size arithmetic. The other cases allocate byte arrays, and the
> > > >>>>>>> only difference is that alloca (_alloca) is supported by msvc.
> > > >>>>>>
> > > >>>>>> Okay, then this patch doesn't do anything useful, since these tests
> > > >>>>>> shouldn't be built with MSVC. dma_bufs are a Linux thing.
> > > >>>>>
> > > >>>>> yes, I understand that, the point was not to build them using msvc.
> > > >>>>>
> > > >>>>> the patch usefulness is in enabling switch Wvla to error instead of
> > > >>>>> warning. other than that, it just reduces warning output.
> > > >>>>
> > > >>>> Ah, I see. Okay.
> > > >>>>
> > > >>>> For my own curiosity, does this actually change the compiled code?
> > > >>>
> > > >>> Looks like the vla version uses fewer instructions but the code size is
> > > >>> the same (for -O3).
> > > >>> I'm using gcc 4.9.2 that comes with F21
> > > >>>
> > > >>> I have attached release and debug versions of
> > > >>> ext_image_dma_buf_import-ownership_transfer piglit_display()
> > > >>> if you're interested
> > > >>>
> > > >>> jan
> > > >>>
> > > >>>
> > > >>
> > > >> --
> > > >> Jan Vesely <jan.vesely at rutgers.edu>
> > > >
> > > >
> > > >
> > > >> _______________________________________________
> > > >> Piglit mailing list
> > > >> Piglit at lists.freedesktop.org
> > > >> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_piglit&d=AwIFAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=axDfDuzBMbe8puZjmv2H6-wYbajqcdPo4tsJ4fdm6is&s=FICxtLdz8KWw9tX2yLTiJHEYEof0TPKcUz25ze35mTc&e=
> > > >>
> > > >>
> > > >> _______________________________________________
> > > >> Piglit mailing list
> > > >> Piglit at lists.freedesktop.org
> > > >> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_piglit&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=axDfDuzBMbe8puZjmv2H6-wYbajqcdPo4tsJ4fdm6is&s=FICxtLdz8KWw9tX2yLTiJHEYEof0TPKcUz25ze35mTc&e=
> > > 
> > 
> > -- 
> > Jan Vesely <jan.vesely at rutgers.edu>
> 
> > From 4634b6f72f5b10d66c1a4e2f5e506c1e14143150 Mon Sep 17 00:00:00 2001
> > From: Jan Vesely <jan.vesely at rutgers.edu>
> > Date: Thu, 4 Dec 2014 14:10:50 -0500
> > Subject: [Piglit][PATCH 1/1] Error on MSVC compatibility warnings
> > 
> > Signed-off-by: Jan Vesely <jan.vesely at rutgers.edu>
> > ---
> >  CMakeLists.txt | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/CMakeLists.txt b/CMakeLists.txt
> > index 12323f0..e218a48 100644
> > --- a/CMakeLists.txt
> > +++ b/CMakeLists.txt
> > @@ -212,13 +212,13 @@ if (NOT MSVC)
> >  	# Unfortunately MSVC does not support C99.  Among all features enabled
> >  	# by C99, declarations after statements is the most frequently used.
> >  	# For portability sake, we request gcc to warn when this is used.
> > -	CHECK_C_COMPILER_FLAG("-Wdeclaration-after-statement" C_COMPILER_FLAG_WDECL_AFTER_STMT)
> > +	CHECK_C_COMPILER_FLAG("-Werror=declaration-after-statement" C_COMPILER_FLAG_WEDECL_AFTER_STMT)
> >  	IF (C_COMPILER_FLAG_WDECL_AFTER_STMT)
> > -		SET(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wdeclaration-after-statement")
> > +		SET(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Werror=declaration-after-statement")
> >  	ENDIF ()
> > -	CHECK_C_COMPILER_FLAG("-Wvla" C_COMPILER_FLAG_WVLA)
> > -	IF (C_COMPILER_FLAG_WVLA)
> > -		SET (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wvla")
> > +	CHECK_C_COMPILER_FLAG("-Werror=vla" C_COMPILER_FLAG_WEVLA)
> > +	IF (C_COMPILER_FLAG_WEVLA)
> > +		SET (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Werror=vla")
> >  	ENDIF ()
> >  
> >  	CHECK_C_COMPILER_FLAG("-Wno-deprecated-declarations" C_COMPILER_FLAG_WNO_DEPRECATED_DECLARATIONS)
> > -- 
> > 2.1.0
> > 
> 
> 
> 

-- 
Jan Vesely <jan.vesely at rutgers.edu>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20150211/70955d07/attachment.sig>