[PATCH] remove attribute access from regexec

[Martin Sebor]

On 8/13/21 4:34 PM, Paul Eggert wrote:
> On 8/13/21 2:30 PM, Martin Sebor wrote:
>> Attached is a revised patch with this approach.
> 
> The revised patch is to include/regex.h but the original patch was to 
> posix/regex.h. Is that intentional?

Yes, they need to be consistent, otherwise GCC issues -Wvla-parameter.
(That's to help detect inadvertent array/VLA mismatches as well as
mismatches in the VLA parameter bounds.)

> 
> We need to check whether __STDC_VERSION__ is defined. Also, no need for 
> parens around arg of 'defined'. Something like this perhaps:
> 
>    #if (defined __STDC_VERSION__ && 199901L <= __STDC_VERSION__ \
>         && !defined __STDC_NO_VLA__)
> 
> Also, the duplication of the declarations make the headers harder to 
> read and encourage typos (I noticed one typo: "_Restrict_arr" without 
> the trailing "_"). Instead, I suggest something like this:
> 
>    #if (defined __STDC_VERSION__ && 199901L <= __STDC_VERSION__ \
>         && !defined __STDC_NO_VLA__)
>    # define _REGEX_VLA(arg) arg
>    #else
>    # define _REGEX_VLA(arg)
>    #endif
> 
> That way, we can simply change "regmatch_t __pmatch[_Restrict_arr_]" to 
> "regmatch_t __pmatch[_Restrict_arr_ _REGEX_VLA (__nmatch)]" without 
> having to duplicate the entire function declaration.

Sounds good.  I've defined the macro in cdefs.h and mamed it _VLA_ARG
to make it usable in other contexts.  Please see the attached revision.

> 
>> PS POSIX says regexec() ignores pnmatch when REG_NOSUB is set, so
>> strictly speaking, warning for such calls to it in that case is
>> also a false positive.
> 
> Ouch, this casts doubt on the entire exercise. It's not simply about 
> warnings: it's about the code being generated for the matcher. For 
> example, for:
> 
> int
> f (_Bool flag, unsigned long n, int a[n])
> {
>    return n == 0 ? 0 : flag ? a[n - 1] : a[0];
> }
> 
> a compiler is allowed to generate code that loads a[n - 1] even when 
> FLAG is false. Similarly, if we add this VLA business to regexec, the 
> generated machine code could dereference pmatch unconditionally even if 
> our source code makes the dereferencing conditional on REG_NOSUB, and 
> the resulting behavior would fail to conform to POSIX.

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

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