Re: [PATCH v2 2/4] compat/regex: include alloca.h before undef it
- From
Ramsay Jones <ramsay@ramsayjones.plus.com>
- Date
- Apr 26, 2020, 19:38 UTC
- Message-ID
- <1a0c2b25-e283-9936-1fa2-ce51df1404dc@ramsayjones.plus.com>
- In-Reply-To
- <e9163e0d-2572-e7e0-6aa3-09ce04750b22@ramsayjones.plus.com>
On 26/04/2020 17:17, Ramsay Jones wrote: [snip]
Show 27 quoted lines
> Hmm, whatever patch you decide to send (even the original one) I think > you need to add an explanation of the problem to the commit message, > including why the patch provides a solution. (You don't have to type > a novel - see commit bd8f005583 :-P ). > > I haven't thought about this too much, but some options: > > - iff the musl library sets some kind of identifying pp variable > (_MUSL_LIBC_ or somesuch - I haven't looked), then you could > make the '#include <alloca.h>' conditional on that variable. > This has the benefit of making it obvious to people reading the > code that this is specific to musl-libc. > > - you could simply remove that '#ifdef GAWK' block completely (Lines > 64->67). We set GAWK and NO_MBSUPPORT unconditionally in the Makefile > so that it compiles (see commit a997bf423d), but these particular > lines simply reflect the gawk projects dislike of alloca (along with > the desire to catch any attempts to add new calls which are not protected > by HAVE_ALLOCA). Given that we are very unlikely to add new calls ... > > - change the conditional on this block to (totally untested, just typing > into my email client) '#if defined(GAWK) && defined(HAVE_ALLOCA)'. > This should work, but it does disable the 'catch any attempts to add > new _unintentional_ calls' aspect of that block; so you may as well > remove it ... > > Just some 'off the top of my head ideas', ... ;-)
Another option I thought about, but didn't mention above, is given by the patch below. I didn't mention it because it has the potential to cause problems on non musl-libc systems (and I was feeling too lazy at the time to go and test ...). Again, see commit bd8f005583.
So, I have now tested this patch on (glibc) linux and it seems to work just fine; compile, sparse, test-suite passed. It also compiles clean (and sparse clean) on cygwin (new-lib) - I didn't run the test-suite on cygwin, since it takes about 3.5 to 4 hours to run.
I don't have any other systems to test this on, so I can't say that it won't cause problems somewhere. In practice, I think the chances of that are rather low, but don't quote me on that! :-P
ATB, Ramsay Jones
-- >8 -- Subject: [PATCH] regex: fix up musl-libc builds
--- compat/regex/regex.c | 1 + compat/regex/regex_internal.h | 1 - 2 files changed, 1 insertion(+), 1 deletion(-)
diff --git a/compat/regex/regex.c b/compat/regex/regex.c index f3e03a9eab..e6f4a5d177 100644 --- a/compat/regex/regex.c +++ b/compat/regex/regex.c @@ -60,6 +60,7 @@ #undefs RE_DUP_MAX and sets it to the right value. */ #include <limits.h> #include <stdint.h> +#include <stdlib.h> #ifdef GAWK #undef alloca diff --git a/compat/regex/regex_internal.h b/compat/regex/regex_internal.h index 3ee8aae59d..0bad8b841e 100644 --- a/compat/regex/regex_internal.h +++ b/compat/regex/regex_internal.h @@ -23,7 +23,6 @@ #include <assert.h> #include <ctype.h> #include <stdio.h> -#include <stdlib.h> #include <string.h> #if defined HAVE_LANGINFO_H || defined HAVE_LANGINFO_CODESET || defined _LIBC
-- 2.26.2