[PATCH v2] remove attribute access from regexec

Martin Sebor msebor@gmail.com
Wed Aug 18 15:53:50 GMT 2021


Let me repost an updated patch as v2 to let the patch tester know
about the revision, and to also include the corresponding change
to regexec.c needed to avoid the -Wvla-parameter I mentioned.

Apparently the first patch failed to apply and the second one
wasn't picked up because the subject line hasn't changed.  Thanks
for letting me know, Carlos!

Here's a link to the Patchwork check:
http://patchwork.sourceware.org/project/glibc/patch/15a32181-8060-4135-cb72-9e79831697d5@gmail.com/

On 8/14/21 2:08 PM, Martin Sebor wrote:
> 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: 2305 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20210818/1255f1a8/attachment.bin>


More information about the Libc-alpha mailing list