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

Mon Feb 9 15:01:01 PST 2015

On Mon, 2015-02-09 at 21:42 +0000, Jose Fonseca wrote:
> On 09/02/15 21:29, Jan Vesely wrote:
> > On Mon, 2015-02-09 at 19:49 +0000, Jose Fonseca wrote:
> >> On 09/02/15 19:17, Ian Romanick wrote:
> >>> On 02/06/2015 11:46 AM, 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.
> >>>
> >>> In Mesa, we can change compiler options in subdirectories.  Is it
> >>> possible to do this with cmake?  Like, could we remove -Wvla in
> >>> directories that are only built on Linux?  It seems like that could give
> >>> us the best of both worlds...
> >>
> >> Yes, I just confirmed that MSVC 2013 U4 still fails to compile VLA code
> >> and was about to propose something similar.
> >>
> >> In theory this
> >>
> >>     string (REPLACE "-Wvla" "" CMAKE_C_FLAGS ${CMAKE_C_FLAGS})
> >>
> >> should undo.  And it does work.  But there's a minor snafu:  this line
> >> must be put in the tests/spec/foo/CMakeLists.txt -- if it's put in the
> >> tests/spec/foo/CMakeLists.gl*.txt it has no effect --  CMAKE_C_FLAGS is
> >> changed, but the source files are still build with -Wvla.
> >>
> >> (I'm always amazed that the  CMakeLists.txt -> CMakeLists.gl*.txt system
> >> works!)
> >
> > I think it's a good idea to keep the warnings/errors global for the sake
> > of consistency. The current setup (-Wvla, -Wdeclaration-after-statement)
> > only affects the parts changed in these two patches.
> 
> There are pro/cons both ways.  I'm happy to let the people that most 
> develop on the affected directories to decide based on their preferences.
> 
> 
> Unfortunately it's not possible to tweak CMAKE_C_FLAGS on a per API 
> basis as I explained before.  It has to be the whole directory.
> 
> 
> > The errors reported below are not caught by -Wvla, since they are not
> > variable length arrays. I don't think there's a gcc warning to catch
> > those.
> 
> Note the errors I pasted were with 
> http://cgit.freedesktop.org/piglit/commit/?id=0784e54b75918b94ac974197b736c53ccba69ff1 
> reverted.  This is was newest VLA related MSVC build breakage I could find.

ah, it makes sense now, I spoke too quickly, should have verified the
error codes.
Anyway, all 3 patches are on the list, and given the minimal changes
required I think it makes sense to go global, maybe add -Wsign-compare
to the mix too.
I'll wait to see what others think.

thanks for your time,
jan

> 
> 
> Jose
> 
> >
> > jan
> >
> >>
> >>
> >>>> 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.
> >>>>
> >>>> Jose
> >>>
> >>
> >> For the record these were MSVC errors:
> >>
> >> AILED: C:\PROGRA~2\MICROS~4.0\VC\bin\cl.exe   /nologo /DWIN32 /D_WINDOWS
> >> /W3 /D_DEBUG /MDd /Zi /Ob0 /Od /RTC1 -I..\..\src
> >> -IH:\msvc32\freeglut\include -Itests\util -I..\..\tests\util
> >> -IH:\noarch\glext    -W3 -wd4018 -wd4244 -wd4305 -wd4800 /showIncludes
> >> -DGLAPIENTRY=__stdcall -DNOMINMAX -DPIGLIT_USE_GLUT
> >> -DPIGLIT_USE_GLUT_INIT_ERROR_FUNC -DPIGLIT_USE_OPENGL
> >> -DWIN32_LEAN_AND_MEAN=1 -D_CRT_NONSTDC_NO_WARNINGS
> >> -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_GNU_SOURCE
> >> -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_USE_MATH_DEFINES
> >> /Fotarget_api\gl\tests\spec\gl-1.0\CMakeFiles\gl-1.0-readpixsanity.dir\readpix.c.obj
> >> /Fdtarget_api\gl\tests\spec\gl-1.0\CMakeFiles\gl-1.0-readpixsanity.dir/
> >> /FS -c ..\..\tests\spec\gl-1.0\readpix.c
> >> ..\..\tests\spec\gl-1.0\readpix.c(103) : error C2057: expected constant
> >> expression
> >> ..\..\tests\spec\gl-1.0\readpix.c(103) : error C2466: cannot allocate an
> >> array of constant size 0
> >> ..\..\tests\spec\gl-1.0\readpix.c(103) : error C2087: 'buf' : missing
> >> subscript
> >> ..\..\tests\spec\gl-1.0\readpix.c(103) : error C2133: 'buf' : unknown size
> >> ..\..\tests\spec\gl-1.0\readpix.c(200) : error C2057: expected constant
> >> expression
> >> ..\..\tests\spec\gl-1.0\readpix.c(200) : error C2466: cannot allocate an
> >> array of constant size 0
> >> ..\..\tests\spec\gl-1.0\readpix.c(200) : error C2087: 'buf' : missing
> >> subscript
> >> ..\..\tests\spec\gl-1.0\readpix.c(200) : error C2133: 'buf' : unknown size
> >> ..\..\tests\spec\gl-1.0\readpix.c(281) : error C2057: expected constant
> >> expression
> >> ..\..\tests\spec\gl-1.0\readpix.c(281) : error C2466: cannot allocate an
> >> array of constant size 0
> >> ..\..\tests\spec\gl-1.0\readpix.c(281) : error C2087: 'buf' : missing
> >> subscript
> >> ..\..\tests\spec\gl-1.0\readpix.c(281) : error C2133: 'buf' : unknown size
> >>
> >>
> >> So there's no doubt here: by expecting a constant expression it is clear
> >> that MSVC does not support VLA at all.
> >>
> >>
> >> Jose
> >
> 

-- 
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/20150209/9bde05f3/attachment-0001.sig>