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

From: Danh Doan <congdanhqx@gmail.com>
To: Ramsay Jones <ramsay@ramsayjones.plus.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2 2/4] compat/regex: include alloca.h before undef it
Date: Mon, 27 Apr 2020 23:46:49 +0700	[thread overview]
Message-ID: <20200427164649.GA31366@danh.dev> (raw)
In-Reply-To: <f3af49c7-6c35-d2f6-c87c-e7342b1c2b6f@ramsayjones.plus.com>

On 2020-04-27 17:28:03+0100, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> No alloca() is not in either the POSIX or C standard(s).
> This was an extension from the early days of BSD Unix.

Okay, I saw it was mentioned in GNU manpage for alloca(3)

> For some reason, I thought you had to explicitly '#include <alloca.h>'
> to use it, but it appears that (by default) you get a bonus include
> from the <stdlib.h> header, unless you restrict the headers using the
> various macros and/or compiler command-line options.

It looks like <alloca.h> is GNU's invention.

*BSD defined it in <stdlib.h>:
https://www.freebsd.org/cgi/man.cgi?alloca
https://man.openbsd.org/alloca
https://netbsd.gw.com/cgi-bin/man-cgi?alloca+3+NetBSD-current

Even GNU's alloca(3) says:

       Normally, gcc(1) translates calls to alloca() with inlined code.  This
       is not done when either the -ansi, -std=c89, -std=c99, or the -std=c11
       option is given and the header <alloca.h> is not included.  Otherwise,
       (without an -ansi or -std=c* option) the glibc version of <stdlib.h>
       includes <alloca.h> 

> As it happens, even on glibc systems, the <alloca.h> header is included
> by the <stdlib.h> header, unless you take steps to suppress it. So, we
> would have had the same issue, if it wasn't for the aforementioned
> '#undef alloca' the the glibc header file.

From above information, I think it's fine to include <stdlib.h> first.
It's AT&T Unix's invention and everyone seems to follow it (except Windows,
but the lack of complains from our Windows friends may signify that
their alloca is fine already).

I've sent it already for v3.

> When I need to look at pp output, while debugging things like this,
> I cherry-pick a patch to the Makefile:
> 
>   $ git diff
>   diff --git a/Makefile b/Makefile
>   index 6d5cee270c..cd8753bf54 100644
>   --- a/Makefile
>   +++ b/Makefile
>   @@ -2423,6 +2423,9 @@ $(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs)
>    %.s: %.c GIT-CFLAGS FORCE
>           $(QUIET_CC)$(CC) -o $@ -S $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
>    
>   +%.i: %.c GIT-CFLAGS FORCE
>   +       $(QUIET_CC)$(CC) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) -E $< >$*.i
>   +
>    ifdef USE_COMPUTED_HEADER_DEPENDENCIES
>    # Take advantage of gcc's on-the-fly dependency generation
>    # See <http://gcc.gnu.org/gcc-3.0/features.html>.
>   @@ -2474,7 +2477,7 @@ http-walker.sp http-walker.s http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT
>    endif
>    
>    ifdef NO_REGEX
>   -compat/regex/regex.sp compat/regex/regex.o: EXTRA_CPPFLAGS = \
>   +compat/regex/regex.i compat/regex/regex.sp compat/regex/regex.o: EXTRA_CPPFLAGS = \
>           -DGAWK -DNO_MBSUPPORT
>    endif
>    
>   $ 

I think it's worth to have this included. `.s` rules is there, anyway.
Personally, I've run this to trace the inclusion:

	make V=1 CFLAGS=-E compat/regex/regex.o compat/regex/regex.sp

-- 
Danh