[PATCH v3] remove attribute access from regexec

[Martin Sebor]

On 8/18/21 1:52 PM, Paul Eggert wrote:
> 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.

Thanks the for the additional testing!  I wouldn't expect to see
-Wvla for a Glibc declaration outside of a Glibc build.   As
a lexical warning, -Wvla shouldn't (and in my tests doesn't) trigger
for code in system headers unless it's enabled by -Wsystem-headers.
Is <regex.h> for some reason not considered a system header in your
test environment?

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

They're both reserved but I'm happy to go with whatever convention
is preferred in Glibc.

> 
> * 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?

That works for me.

> 
> * 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.

I didn't know mixing and matching two implementations like this
was even possible.  Thanks for explaining it (though it seems
like a pretty cumbersome arrangement).  I've made the suggested
change.

> 
> * 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

I've changed the macro to the above.

> 
> 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).

I agree.  Someone else made the same suggestion in GCC bug 98217 (and
I even offered to handle it).  I'll try to remember to get to it but
as I said above, I don't think it should be necessary for this change.

Attached is yet another revision of this patch (v3 to let the patch
tester pick it up).

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: glibc-28170.diff
Type: text/x-patch
Size: 2828 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20210819/d3906359/attachment.bin>