Re: [PATCH v2 2/4] compat/regex: include alloca.h before undef it

From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Apr 26, 2020, 16:17 UTC
Message-ID: <e9163e0d-2572-e7e0-6aa3-09ce04750b22@ramsayjones.plus.com>
In-Reply-To: <20200426005451.f7pyoiijgbk4hpsj@danh.dev>

On 26/04/2020 01:54, Danh Doan wrote:

> On 2020-04-25 21:28:05+0100, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:

[snip]

Show 16 quoted lines

>> OK, I had a quick look at the <alloca.h> header file on a glibc
>> system (linux) and new-lib system (cygwin) and they both do
>> (more or less) the same thing: first #undef alloca, and then
>> if being compiled by gcc, define alloca(size) to be __builtin_alloca(size).
> 
> musl people don't do that.
> They just go ahead define it, if any other header file requires
> alloca, they will include alloca.h
> 
>> So, even if <alloca.h> is #included after regex.c:66, it wouldn't
>> be a problem. Since I don't have access to a musl based system,
>> I don't know what that system header is doing.
> 
> musl's alloca.h is available here:
> 
> https://git.musl-libc.org/cgit/musl/tree/include/alloca.h

Hmm, OK, so that partly explains the problem. I wonder if the musl guys would accept a bug report?

Show 12 quoted lines

>> However, I said *even if* above, because I don't see why it is trying
>> to #include <alloca.h> in the first place! ;-)
> 
> I looked into my system again. The inclusion chain is:
> 
> compat/regex/regex.c:77
> `-> compat/regex/regex_internal.h:26
>     `-> /usr/include/stdlib.h:138 [*1*]
> 
> [*1*]: https://git.musl-libc.org/cgit/musl/tree/include/stdlib.h#n137
> 
> I don't know why _GNU_SOURCE and/or _BSD_SOURCE is defined.

... and this explains the main cause. Hmm, as you say, why are one (or both) of those pp variables set. :(

[snip]

>> BTW, why are you compiling with NO_REGEX set anyway?
> 
> Because I use musl-libc, and musl-libc doesn't have StartEnd

Ah, OK. ;-)

Well, even if the musl project accepted a PR and provided a fix, that will not help you in the short term! :D

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', ... ;-)

ATB, Ramsay Jones