[PATCH v3] remove attribute access from regexec

[Martin Sebor]

On 8/21/21 11:06 PM, Paul Eggert wrote:
> On 8/19/21 4:50 PM, Martin Sebor wrote:
> 
>> Is <regex.h> for some reason not considered a system header in your
>> test environment?
> 
> Yes, because Coreutils etc. currently use Gnulib regex.h instead of 
> /usr/include/regex.h, as Glibc regex has a bug and the fix (in Gnulib) 
> hasn't made its way back to Glibc yet.
> 
> This sort of thing is all too common, as regex development (such as it 
> is) often occurs in Gnulib first, which means it's common for 
> 'configure' to discover that the current Glibc has a bug and to 
> substitute Gnulib regex instead. And when we compile Gnulib headers we 
> don't consider them to be system headers, because we want the extra 
> static checking that gcc does for non system headers.
> 
> I think the simplest workaround for the warning is to disable -Wvla for 
> that particular declaration. Please see attached Gnulib patch for how I 
> suggest doing this.
> 
>> 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).
> 
> Yes, it is a bit awkward. I am tempted to clean it up but that really 
> should be a separate patch and so I will focus only on this thread's issue.
> 
>> +#ifndef __ARG_NELTS
>> +# ifdef __ARG_NELTS
> 
> A clear typo. The suggestion was to define and use _ARG_NELTS_ instead.

__ARG_NELTS is  the name of macro in cdefs.h that you suggested so
this should presumably be the same, no?  (I.e., it's not a typo but
I could be missing something.)

> 
> Some other thoughts.
> 
> If we're going to change regexec, we should change __compat_regex to be 
> consistent.
> 
> We should also change regexec.c's internal functions to be consistent 
> with regexec. This includes re_search_internal, update_regs, etc.

That sounds like a good idea for a follow-up.  My immediate goal is
to avoid the current warning so Glibc can build, and I prefer to make
that change independently of other improvements.

I plan to commit the last revision of the patch as is unless there's
some reason I'm missing that the name of the regex.h macro shouldn't
be the same as that in cdefs.h.  I see you have both __ARG_NELTS and
_ARG_NELTS_ in your Gnulib patch though I don't understand why.

Thanks for all your comments!

Martin

> 
> These internal functions don't have regexec's quirk that pmatch is 
> ignored when not substituting, so their parameters can use the syntax 
> "regmatch_t pmatch[__ARG_NELTS (static nmatch)]" to let the compiler 
> know that the array must be of the given size.
> 
> Proposed Gnulib patch attached. It should be easy to aplly to glibc 
> though glibc also needs its own regex.h patched. I have not installed 
> this into Gnulib yet.
> 
> At some point we should sync Gnulib and Glibc regex of course.