[PATCH v3] remove attribute access from regexec

[Martin Sebor <msebor@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4283 bytes --]

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

[-- Attachment #2: glibc-28170.diff --]
[-- Type: text/x-patch, Size: 2828 bytes --]

diff --git a/include/regex.h b/include/regex.h
index 24eca2c297..76fa798861 100644
--- a/include/regex.h
+++ b/include/regex.h
@@ -36,8 +36,24 @@ extern void __re_set_registers
 extern int __regcomp (regex_t *__preg, const char *__pattern, int __cflags);
 libc_hidden_proto (__regcomp)
 
+
+#ifndef __ARG_NELTS
+# ifdef __ARG_NELTS
+/* Same as the corresponding cdefs.h macro.  Defined for builds with
+   no cdefs.h.  */
+#  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
+
 extern int __regexec (const regex_t *__preg, const char *__string,
-		      size_t __nmatch, regmatch_t __pmatch[], int __eflags);
+		      size_t __nmatch,
+		      regmatch_t __pmatch[__ARG_NELTS (__nmatch)],
+		      int __eflags);
 libc_hidden_proto (__regexec)
 
 extern size_t __regerror (int __errcode, const regex_t *__preg,
diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
index e490fc1aeb..64e46df190 100644
--- a/misc/sys/cdefs.h
+++ b/misc/sys/cdefs.h
@@ -632,4 +632,17 @@ _Static_assert (0, "IEEE 128-bits long double requires redirection on this platf
 # define __attribute_returns_twice__ /* Ignore.  */
 #endif
 
+#ifndef __ARG_NELTS
+# ifdef __ARG_NELTS
+/* Used to specify a variable bound in a declaration of a function
+   VLA-like parameter, as in 'int f (int n, int[__ARG_NELTS (n)]);'  */
+#  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
+
 #endif	 /* sys/cdefs.h */
diff --git a/posix/regex.h b/posix/regex.h
index 14fb1d8364..5b44f8e52b 100644
--- a/posix/regex.h
+++ b/posix/regex.h
@@ -654,9 +654,8 @@ extern int regcomp (regex_t *_Restrict_ __preg,
 
 extern int regexec (const regex_t *_Restrict_ __preg,
 		    const char *_Restrict_ __String, size_t __nmatch,
-		    regmatch_t __pmatch[_Restrict_arr_],
-		    int __eflags)
-    __attr_access ((__write_only__, 4, 3));
+		    regmatch_t __pmatch[_Restrict_arr_ __ARG_NELTS (__nmatch)],
+		    int __eflags);
 
 extern size_t regerror (int __errcode, const regex_t *_Restrict_ __preg,
 			char *_Restrict_ __errbuf, size_t __errbuf_size)
diff --git a/posix/regexec.c b/posix/regexec.c
index f7b4f9cfc3..bec2fdfe39 100644
--- a/posix/regexec.c
+++ b/posix/regexec.c
@@ -190,7 +190,7 @@ static reg_errcode_t extend_buffers (re_match_context_t *mctx, int min_len);
 
 int
 regexec (const regex_t *__restrict preg, const char *__restrict string,
-	 size_t nmatch, regmatch_t pmatch[], int eflags)
+	 size_t nmatch, regmatch_t pmatch[__ARG_NELTS (nmatch)], int eflags)
 {
   reg_errcode_t err;
   Idx start, length;