Re: [PATCH] remove attribute access from regexec

[Paul Eggert <eggert@cs.ucla.edu>]

On 8/14/21 1:08 PM, Martin Sebor wrote:
> The VLA bound by itself doesn't affect codegen.  I suspect you're
> thinking of a[static n]?  With just a[n], without static, there
> is no requirement that a point to an array with n elements.  It
> simply declares an ordinary pointer, same as [] or *.

Thanks for clarifying.

I tried using a patch like that on coreutils, but it caused the build to 
fail like this:

   In file included from lib/exclude.c:35:
   ./lib/regex.h:661:7: error: ISO C90 forbids variable length array 
'__pmatch' [-Werror=vla]
     661 |       regmatch_t __pmatch[_Restrict_arr_ _VLA_ARG (__nmatch)],
         |       ^~~~~~~~~~
   cc1: all warnings being treated as errors
   make[2]: *** [Makefile:10648: lib/exclude.o] Error 1

This is because coreutils is compiled with -Wvla -Werror, to catch 
inadvertent attempts to use VLAs in local variables (this helps avoid 
stack-overflow problems). It'd be unfortunate if we had to give that 
useful diagnostic up simply due to this declaration, as there's no 
stack-overflow problem here.

If you can think of a way around this issue, here are some other things 
I ran into while trying this idea out on Coreutils.

* Other cdefs.h macros (__NTH, __REDIRECT, etc.) start with two 
underscores, so shouldn't this new macro too?

* Come to think of it, the name _VLA_ARG could be improved, as its 
argument is not actually a VLA; it's the number of elements in a 
VLA-like array. Also, its formal-parameter "arg" is confusingly-named, 
as it's an arbitrary integer expression and need not be a function 
parameter name. How about naming the macro __ARG_NELTS instead?

* regex.h cannot use __ARG_NELTS directly, for the same reason it can't 
use __restrict_arr directly: regex.h is shared with Gnulib and can't 
assume that a glibc-like sys/cdefs.h is present. I suppose regex.h would 
need something like this:

   #ifndef _ARG_NELTS_
   # ifdef __ARG_NELTS
   #  define _ARG_NELTS_(arg) __ARG_NELTS (arg)
   # elif (defined __STDC_VERSION__ && 199901L <= __STDC_VERSION__ \
	  && !defined __STDC_NO_VLA__)
   #  define _ARG_NELTS_(n) n
   # else
   #  define _ARG_NELTS_(n)
   # endif
   #endif

and then use _ARG_NELTS_ later.

* The cdefs.h comment has a stray 'n', its wording could be improved (I 
misread "variable bound" as a variable that's bound to something), 
there's a stray empty line, and it's nicer to put the comment in front 
of all the lines that define the macro. Perhaps something like this:

   /* Specify the number of elements of a function's array parameter,
      as in 'int f (int n, int a[__ARG_NELTS (n)]);'.  */
   #if (defined __STDC_VERSION__ && 199901L <= __STDC_VERSION__ \
        && !defined __STDC_NO_VLA__)
   # define __ARG_NELTS(n) n
   #else
   # define __ARG_NELTS(n)
   #endif

Though again, it's not clear to me that this idea will fly at all, due 
to the -Wvla issue.

Maybe GCC's -Wvla should be fixed to not report an error in this case? 
It's actually not a VLA if you ask me (the C standard is unclear).