bug-gnulib@gnu.org mirror (unofficial)
 help / color / mirror / Atom feed
* Gnulib's alloca.h used even when there is a system header
@ 2019-02-17 17:20 Eli Zaretskii
  2019-02-17 20:06 ` Paul Eggert
  2019-02-17 23:28 ` Bruno Haible
  0 siblings, 2 replies; 15+ messages in thread
From: Eli Zaretskii @ 2019-02-17 17:20 UTC (permalink / raw)
  To: bug-gnulib

Building the latest version of Texinfo, I see that Gnulib decides to
generate its own alloca.h header even though there's a system header
alloca.h (and HAVE_ALLOCA_H is 1 in config.h).  Why does it do that?
Isn't that dangerous? e.g., it could conflict with how the system
header defies 'alloca', no?


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Gnulib's alloca.h used even when there is a system header
  2019-02-17 17:20 Gnulib's alloca.h used even when there is a system header Eli Zaretskii
@ 2019-02-17 20:06 ` Paul Eggert
  2019-02-17 20:24   ` Eli Zaretskii
  2019-02-17 23:28 ` Bruno Haible
  1 sibling, 1 reply; 15+ messages in thread
From: Paul Eggert @ 2019-02-17 20:06 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: bug-gnulib

Eli Zaretskii wrote:
> Gnulib decides to
> generate its own alloca.h header even though there's a system header
> alloca.h (and HAVE_ALLOCA_H is 1 in config.h).  Why does it do that?
> Isn't that dangerous? e.g., it could conflict with how the system
> header defies 'alloca', no?

This stuff is rickety, as it's built atop Autoconf's AC_FUNC_ALLOCA and that 
stuff hasn't changed in at least a decade. In the old days, some alloca 
implementations were broken and when in doubt it was better to replace the 
system alloca than to use it. Nowadays I doubt whether it matters much. In any 
event Autoconf and/or Gnulib patches to be more cautious about replacing 
alloca.h would be welcome.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Gnulib's alloca.h used even when there is a system header
  2019-02-17 20:06 ` Paul Eggert
@ 2019-02-17 20:24   ` Eli Zaretskii
  2019-02-17 21:09     ` Paul Eggert
  0 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2019-02-17 20:24 UTC (permalink / raw)
  To: Paul Eggert; +Cc: bug-gnulib

> Cc: bug-gnulib@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 17 Feb 2019 12:06:53 -0800
> 
> This stuff is rickety, as it's built atop Autoconf's AC_FUNC_ALLOCA and that 
> stuff hasn't changed in at least a decade. In the old days, some alloca 
> implementations were broken and when in doubt it was better to replace the 
> system alloca than to use it. Nowadays I doubt whether it matters much. In any 
> event Autoconf and/or Gnulib patches to be more cautious about replacing 
> alloca.h would be welcome.

Thanks.  I'm currently testing the below, will get back when I'm sure
it's TRT.  But if you think it might not be right for reasons related
to other platforms or some global Gnulib considerations, please tell.

--- gnulib/lib/alloca.h~0	2019-02-17 18:00:40.589250000 +0200
+++ gnulib/lib/alloca.h	2019-02-17 18:22:50.933000000 +0200
@@ -37,7 +37,11 @@
 
 #ifndef alloca
 # ifdef __GNUC__
-#  define alloca __builtin_alloca
+#  if HAVE_ALLOCA_H
+#   include_next <alloca.h>
+#  else
+#   define alloca __builtin_alloca
+#  endif
 # elif defined _AIX
 #  define alloca __alloca
 # elif defined _MSC_VER


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Gnulib's alloca.h used even when there is a system header
  2019-02-17 20:24   ` Eli Zaretskii
@ 2019-02-17 21:09     ` Paul Eggert
  2019-02-17 23:32       ` Bruno Haible
  2019-02-18 16:59       ` Eli Zaretskii
  0 siblings, 2 replies; 15+ messages in thread
From: Paul Eggert @ 2019-02-17 21:09 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: bug-gnulib

Eli Zaretskii wrote:
>   #ifndef alloca
>   # ifdef __GNUC__
> -#  define alloca __builtin_alloca
> +#  if HAVE_ALLOCA_H
> +#   include_next <alloca.h>
> +#  else
> +#   define alloca __builtin_alloca
> +#  endif
>   # elif defined _AIX

Why do the 'include_next' only for GCC? Why not do it for all compilers?

Also, what platforms don't work with the current Gnulib alloca module, and why?


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Gnulib's alloca.h used even when there is a system header
  2019-02-17 17:20 Gnulib's alloca.h used even when there is a system header Eli Zaretskii
  2019-02-17 20:06 ` Paul Eggert
@ 2019-02-17 23:28 ` Bruno Haible
  1 sibling, 0 replies; 15+ messages in thread
From: Bruno Haible @ 2019-02-17 23:28 UTC (permalink / raw)
  To: bug-gnulib; +Cc: Eli Zaretskii

Hi Eli,

> Building the latest version of Texinfo, I see that Gnulib decides to
> generate its own alloca.h header even though there's a system header
> alloca.h (and HAVE_ALLOCA_H is 1 in config.h).  Why does it do that?

Generally speaking, we (gnulib developers) put effort into generating
zero code in the object files on platforms which implement the POSIX
standard plus the desired GNU extensions. We don't put as much effort
into using avoiding a gnulib-generated .h file because
  - whether the gnulib-generated .h file exists or not, the code size
    in the generated binaries is the same,
  - when we have extra code that decides when to omit the .h file, we
    have to update it each time we add a new workaround for one of the
    types/macros/functions defined in this .h file.
In other words, it costs maintenance effort and provides little gain
(except for purists).

> Isn't that dangerous? e.g., it could conflict with how the system
> header defies 'alloca', no?

One of the problems the current code avoids is that on old AIX versions,
<alloca.h> had a #pragma which was only effective when <alloca.h> was
the first system include file to get included. This was a very annoying
restriction (must always include <alloca.h> first, after <config.h>),
and IIRC this is one of the things that the current gnulib code fixes.

Bruno



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Gnulib's alloca.h used even when there is a system header
  2019-02-17 21:09     ` Paul Eggert
@ 2019-02-17 23:32       ` Bruno Haible
  2019-02-18 16:59       ` Eli Zaretskii
  1 sibling, 0 replies; 15+ messages in thread
From: Bruno Haible @ 2019-02-17 23:32 UTC (permalink / raw)
  To: bug-gnulib; +Cc: Eli Zaretskii, Paul Eggert

Hi Paul,

> Eli Zaretskii wrote:
> >   #ifndef alloca
> >   # ifdef __GNUC__
> > -#  define alloca __builtin_alloca
> > +#  if HAVE_ALLOCA_H
> > +#   include_next <alloca.h>
> > +#  else
> > +#   define alloca __builtin_alloca
> > +#  endif
> >   # elif defined _AIX
> 
> Why do the 'include_next' only for GCC? Why not do it for all compilers?

'include_next' needs absolute file name for some compilers, and our mechanics
for determining the absolute file name of a .h file does not work when the
.h file defines only macros. See m4/include_next.m4:
  # Note: This macro assumes that the header file is not empty after
  # preprocessing, i.e. it does not only define preprocessor macros but also
  # provides some type/enum definitions or function/variable declarations.

alloca.h unfortunately may be in this category.

> Also, what platforms don't work with the current Gnulib alloca module, and why?

Yes, Eli, I'd like to know this as well: what is the proposed change doing
to fix/improve?

Bruno



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Gnulib's alloca.h used even when there is a system header
  2019-02-17 21:09     ` Paul Eggert
  2019-02-17 23:32       ` Bruno Haible
@ 2019-02-18 16:59       ` Eli Zaretskii
  2019-02-18 23:32         ` Bruno Haible
  1 sibling, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2019-02-18 16:59 UTC (permalink / raw)
  To: Paul Eggert; +Cc: bug-gnulib

> Cc: bug-gnulib@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 17 Feb 2019 13:09:06 -0800
> 
> Eli Zaretskii wrote:
> >   #ifndef alloca
> >   # ifdef __GNUC__
> > -#  define alloca __builtin_alloca
> > +#  if HAVE_ALLOCA_H
> > +#   include_next <alloca.h>
> > +#  else
> > +#   define alloca __builtin_alloca
> > +#  endif
> >   # elif defined _AIX
> 
> Why do the 'include_next' only for GCC? Why not do it for all compilers?

I didn't know other compilers supported include_next.

> Also, what platforms don't work with the current Gnulib alloca module, and why?

I bumped into this with mingw.org's MinGW, which introduced alloca.h
in its recent versions.  (MinGW64 doesn't have this header.)  This new
MinGW alloca.h does this:

  __CRT_ALIAS void *alloca( size_t __n ){ return __builtin_alloca( __n ); }

which makes alloca an always-inline function by virtue of

  #  define __CRT_INLINE   extern __inline__
  #  define __CRT_ALIAS  __CRT_INLINE __attribute__((__always_inline__))

The other part of the puzzle is that stdlib.h does this:

  # include "alloca.h"

so Gnulib's alloca.h is bypassed.  Which causes this when compiling
Gnulib's vasnprintf.c:

     In file included from vasnprintf.c:57:0:
     d:\usr\include\alloca.h: In function 'vasnprintf':
     ./alloca.h:40:18: error: inlining failed in call to always_inline '__builtin_alloca': function not considered for inlining
      #  define alloca __builtin_alloca
		       ^
     In file included from d:\usr\include\stdlib.h:499:0,
		      from vasnprintf.c:71:
     d:\usr\include\alloca.h:62:48: note: called from here
      __CRT_ALIAS void *alloca( size_t __n ){ return __builtin_alloca( __n ); }
						     ^~~~~~~~~~~~~~~~~~~~~~~
     Makefile:945: recipe for target `vasnprintf.lo' failed


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Gnulib's alloca.h used even when there is a system header
  2019-02-18 16:59       ` Eli Zaretskii
@ 2019-02-18 23:32         ` Bruno Haible
  2019-02-19 17:18           ` Eli Zaretskii
  0 siblings, 1 reply; 15+ messages in thread
From: Bruno Haible @ 2019-02-18 23:32 UTC (permalink / raw)
  To: bug-gnulib; +Cc: Eli Zaretskii, Paul Eggert

Hi Eli,

>      ./alloca.h:40:18: error: inlining failed in call to always_inline '__builtin_alloca': function not considered for inlining
>       #  define alloca __builtin_alloca
> 		       ^

GCC never inlines a function that calls alloca() or __builtin_alloca().
The reason is that if you call this function in a loop, then without
inlining it will consume a bounded amount of stack whereas with inlining
it might cause a stack overflow.

The mingw people have declared their new alloca() function as "always
inline", and GCC is decent enough to point us to the dilemma that it
cannot resolve.

Of course, removing the always-inline marker would not help: alloca(N)
would then return an address that is approximately N bytes below the
stack pointer, that is, which points to an area that gets randomly
overwritten (when a signal comes in).

The upshot is: you can't define an alloca() function this way.
Either define it as a macro - which is what the gnulib override does,
if it gets activated -, or define it as a function in assembly-language.

Since you say that this is a fresh bug from mingw, it's probably best
that it gets reported to the mingw people. Then we don't need a Gnulib
workaround.

Bruno




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Gnulib's alloca.h used even when there is a system header
  2019-02-18 23:32         ` Bruno Haible
@ 2019-02-19 17:18           ` Eli Zaretskii
  2019-02-28 18:40             ` Eli Zaretskii
  2019-03-09 19:25             ` Keith Marshall
  0 siblings, 2 replies; 15+ messages in thread
From: Eli Zaretskii @ 2019-02-19 17:18 UTC (permalink / raw)
  To: Bruno Haible; +Cc: Keith Marshall, eggert, bug-gnulib

> From: Bruno Haible <bruno@clisp.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 19 Feb 2019 00:32:24 +0100
> 
> Hi Eli,
> 
> >      ./alloca.h:40:18: error: inlining failed in call to always_inline '__builtin_alloca': function not considered for inlining
> >       #  define alloca __builtin_alloca
> > 		       ^
> 
> GCC never inlines a function that calls alloca() or __builtin_alloca().
> The reason is that if you call this function in a loop, then without
> inlining it will consume a bounded amount of stack whereas with inlining
> it might cause a stack overflow.
> 
> The mingw people have declared their new alloca() function as "always
> inline", and GCC is decent enough to point us to the dilemma that it
> cannot resolve.

The GCC diagnostics is somewhat cryptic, so it's hard for me to be
sure what it means.  My original interpretation was that due to
Gnulib's #define, what GCC sees is this:

  extern inline __attribute__((__always_inline__))
    void *__builtin_alloca( size_t __n ){ return __builtin_alloca( __n ); }

which AFAIU cannot possibly compile.

As for not ever inlining, please see a toy function I compiled and its
disassembly at the end of this message.  Does this confirm what you
say above?  I'm not sure.

> The upshot is: you can't define an alloca() function this way.
> Either define it as a macro - which is what the gnulib override does,
> if it gets activated -, or define it as a function in assembly-language.
> 
> Since you say that this is a fresh bug from mingw, it's probably best
> that it gets reported to the mingw people. Then we don't need a Gnulib
> workaround.

CC'ing Keith, who maintains the MinGW runtime.

Regardless of whether there is or isn't a bug in MinGW, I urge you to
reconsider whether unconditionally overriding a system header in such
a way is a good idea, at least for platforms that use GCC.  I think
this is a fragile arrangement, even if it seems to work.

Here's the toy source code I used:

#include <alloca.h>
#include <string.h>

char *
make_string (int c, size_t n)
{
  char *local_string = alloca (n + 1);
  int i;

  for (i = 0; i < n; i++)
    local_string[i] = c;
  local_string[n] = '\0';
  return strdup (local_string);
}

And here's the disassembly of the object file generated by MinGW GCC:


al.o:     file format pe-i386


Disassembly of section .text:

00000000 <_make_string>:
#include <alloca.h>
#include <string.h>

char *
make_string (int c, size_t n)
{
   0:	55                   	push   %ebp
   1:	89 e5                	mov    %esp,%ebp
   3:	83 ec 28             	sub    $0x28,%esp
  char *local_string = alloca (n + 1);
   6:	8b 45 0c             	mov    0xc(%ebp),%eax
   9:	83 c0 01             	add    $0x1,%eax
   c:	89 45 ec             	mov    %eax,-0x14(%ebp)

#if defined _GNU_SOURCE || ! defined _NO_OLDNAMES
/* This is the GNU standard API; it is also compatible with Microsoft's
 * original, but now deprecated, naming convention.
 */
__CRT_ALIAS void *alloca( size_t __n ){ return __builtin_alloca( __n ); }
   f:	8b 45 ec             	mov    -0x14(%ebp),%eax
  12:	8d 50 0f             	lea    0xf(%eax),%edx
  15:	b8 10 00 00 00       	mov    $0x10,%eax
  1a:	83 e8 01             	sub    $0x1,%eax
  1d:	01 d0                	add    %edx,%eax
  1f:	b9 10 00 00 00       	mov    $0x10,%ecx
  24:	ba 00 00 00 00       	mov    $0x0,%edx
  29:	f7 f1                	div    %ecx
  2b:	6b c0 10             	imul   $0x10,%eax,%eax
  2e:	e8 00 00 00 00       	call   33 <_make_string+0x33>
  33:	29 c4                	sub    %eax,%esp
  35:	8d 44 24 04          	lea    0x4(%esp),%eax
  39:	83 c0 0f             	add    $0xf,%eax
  3c:	c1 e8 04             	shr    $0x4,%eax
  3f:	c1 e0 04             	shl    $0x4,%eax
  42:	89 45 f0             	mov    %eax,-0x10(%ebp)
  int i;

  for (i = 0; i < n; i++)
  45:	c7 45 f4 00 00 00 00 	movl   $0x0,-0xc(%ebp)
  4c:	eb 11                	jmp    5f <_make_string+0x5f>
    local_string[i] = c;
  4e:	8b 55 f4             	mov    -0xc(%ebp),%edx
  51:	8b 45 f0             	mov    -0x10(%ebp),%eax
  54:	01 d0                	add    %edx,%eax
  56:	8b 55 08             	mov    0x8(%ebp),%edx
  59:	88 10                	mov    %dl,(%eax)
make_string (int c, size_t n)
{
  char *local_string = alloca (n + 1);
  int i;

  for (i = 0; i < n; i++)
  5b:	83 45 f4 01          	addl   $0x1,-0xc(%ebp)
  5f:	8b 45 f4             	mov    -0xc(%ebp),%eax
  62:	39 45 0c             	cmp    %eax,0xc(%ebp)
  65:	77 e7                	ja     4e <_make_string+0x4e>
    local_string[i] = c;
  local_string[n] = '\0';
  67:	8b 55 f0             	mov    -0x10(%ebp),%edx
  6a:	8b 45 0c             	mov    0xc(%ebp),%eax
  6d:	01 d0                	add    %edx,%eax
  6f:	c6 00 00             	movb   $0x0,(%eax)
  return strdup (local_string);
  72:	8b 45 f0             	mov    -0x10(%ebp),%eax
  75:	89 04 24             	mov    %eax,(%esp)
  78:	e8 00 00 00 00       	call   7d <_make_string+0x7d>
}
  7d:	c9                   	leave  
  7e:	c3                   	ret    
  7f:	90                   	nop


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Gnulib's alloca.h used even when there is a system header
  2019-02-19 17:18           ` Eli Zaretskii
@ 2019-02-28 18:40             ` Eli Zaretskii
  2019-03-09 19:25             ` Keith Marshall
  1 sibling, 0 replies; 15+ messages in thread
From: Eli Zaretskii @ 2019-02-28 18:40 UTC (permalink / raw)
  To: bruno; +Cc: keith, eggert, bug-gnulib

Ping!  This discussion seems to have stalled, without reaching any
firms conclusion.  Can we please finish discussing this?  Or maybe it
is OK to install the patch I proposed up-thread?

TIA

> Date: Tue, 19 Feb 2019 19:18:15 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> CC: bug-gnulib@gnu.org, eggert@cs.ucla.edu,
> 	Keith Marshall <keith@users.osdn.me>
> 
> > From: Bruno Haible <bruno@clisp.org>
> > Cc: Eli Zaretskii <eliz@gnu.org>, Paul Eggert <eggert@cs.ucla.edu>
> > Date: Tue, 19 Feb 2019 00:32:24 +0100
> > 
> > Hi Eli,
> > 
> > >      ./alloca.h:40:18: error: inlining failed in call to always_inline '__builtin_alloca': function not considered for inlining
> > >       #  define alloca __builtin_alloca
> > > 		       ^
> > 
> > GCC never inlines a function that calls alloca() or __builtin_alloca().
> > The reason is that if you call this function in a loop, then without
> > inlining it will consume a bounded amount of stack whereas with inlining
> > it might cause a stack overflow.
> > 
> > The mingw people have declared their new alloca() function as "always
> > inline", and GCC is decent enough to point us to the dilemma that it
> > cannot resolve.
> 
> The GCC diagnostics is somewhat cryptic, so it's hard for me to be
> sure what it means.  My original interpretation was that due to
> Gnulib's #define, what GCC sees is this:
> 
>   extern inline __attribute__((__always_inline__))
>     void *__builtin_alloca( size_t __n ){ return __builtin_alloca( __n ); }
> 
> which AFAIU cannot possibly compile.
> 
> As for not ever inlining, please see a toy function I compiled and its
> disassembly at the end of this message.  Does this confirm what you
> say above?  I'm not sure.
> 
> > The upshot is: you can't define an alloca() function this way.
> > Either define it as a macro - which is what the gnulib override does,
> > if it gets activated -, or define it as a function in assembly-language.
> > 
> > Since you say that this is a fresh bug from mingw, it's probably best
> > that it gets reported to the mingw people. Then we don't need a Gnulib
> > workaround.
> 
> CC'ing Keith, who maintains the MinGW runtime.
> 
> Regardless of whether there is or isn't a bug in MinGW, I urge you to
> reconsider whether unconditionally overriding a system header in such
> a way is a good idea, at least for platforms that use GCC.  I think
> this is a fragile arrangement, even if it seems to work.
> 
> Here's the toy source code I used:
> 
> #include <alloca.h>
> #include <string.h>
> 
> char *
> make_string (int c, size_t n)
> {
>   char *local_string = alloca (n + 1);
>   int i;
> 
>   for (i = 0; i < n; i++)
>     local_string[i] = c;
>   local_string[n] = '\0';
>   return strdup (local_string);
> }
> 
> And here's the disassembly of the object file generated by MinGW GCC:
> 
> 
> al.o:     file format pe-i386
> 
> 
> Disassembly of section .text:
> 
> 00000000 <_make_string>:
> #include <alloca.h>
> #include <string.h>
> 
> char *
> make_string (int c, size_t n)
> {
>    0:	55                   	push   %ebp
>    1:	89 e5                	mov    %esp,%ebp
>    3:	83 ec 28             	sub    $0x28,%esp
>   char *local_string = alloca (n + 1);
>    6:	8b 45 0c             	mov    0xc(%ebp),%eax
>    9:	83 c0 01             	add    $0x1,%eax
>    c:	89 45 ec             	mov    %eax,-0x14(%ebp)
> 
> #if defined _GNU_SOURCE || ! defined _NO_OLDNAMES
> /* This is the GNU standard API; it is also compatible with Microsoft's
>  * original, but now deprecated, naming convention.
>  */
> __CRT_ALIAS void *alloca( size_t __n ){ return __builtin_alloca( __n ); }
>    f:	8b 45 ec             	mov    -0x14(%ebp),%eax
>   12:	8d 50 0f             	lea    0xf(%eax),%edx
>   15:	b8 10 00 00 00       	mov    $0x10,%eax
>   1a:	83 e8 01             	sub    $0x1,%eax
>   1d:	01 d0                	add    %edx,%eax
>   1f:	b9 10 00 00 00       	mov    $0x10,%ecx
>   24:	ba 00 00 00 00       	mov    $0x0,%edx
>   29:	f7 f1                	div    %ecx
>   2b:	6b c0 10             	imul   $0x10,%eax,%eax
>   2e:	e8 00 00 00 00       	call   33 <_make_string+0x33>
>   33:	29 c4                	sub    %eax,%esp
>   35:	8d 44 24 04          	lea    0x4(%esp),%eax
>   39:	83 c0 0f             	add    $0xf,%eax
>   3c:	c1 e8 04             	shr    $0x4,%eax
>   3f:	c1 e0 04             	shl    $0x4,%eax
>   42:	89 45 f0             	mov    %eax,-0x10(%ebp)
>   int i;
> 
>   for (i = 0; i < n; i++)
>   45:	c7 45 f4 00 00 00 00 	movl   $0x0,-0xc(%ebp)
>   4c:	eb 11                	jmp    5f <_make_string+0x5f>
>     local_string[i] = c;
>   4e:	8b 55 f4             	mov    -0xc(%ebp),%edx
>   51:	8b 45 f0             	mov    -0x10(%ebp),%eax
>   54:	01 d0                	add    %edx,%eax
>   56:	8b 55 08             	mov    0x8(%ebp),%edx
>   59:	88 10                	mov    %dl,(%eax)
> make_string (int c, size_t n)
> {
>   char *local_string = alloca (n + 1);
>   int i;
> 
>   for (i = 0; i < n; i++)
>   5b:	83 45 f4 01          	addl   $0x1,-0xc(%ebp)
>   5f:	8b 45 f4             	mov    -0xc(%ebp),%eax
>   62:	39 45 0c             	cmp    %eax,0xc(%ebp)
>   65:	77 e7                	ja     4e <_make_string+0x4e>
>     local_string[i] = c;
>   local_string[n] = '\0';
>   67:	8b 55 f0             	mov    -0x10(%ebp),%edx
>   6a:	8b 45 0c             	mov    0xc(%ebp),%eax
>   6d:	01 d0                	add    %edx,%eax
>   6f:	c6 00 00             	movb   $0x0,(%eax)
>   return strdup (local_string);
>   72:	8b 45 f0             	mov    -0x10(%ebp),%eax
>   75:	89 04 24             	mov    %eax,(%esp)
>   78:	e8 00 00 00 00       	call   7d <_make_string+0x7d>
> }
>   7d:	c9                   	leave  
>   7e:	c3                   	ret    
>   7f:	90                   	nop
> 
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Gnulib's alloca.h used even when there is a system header
  2019-02-19 17:18           ` Eli Zaretskii
  2019-02-28 18:40             ` Eli Zaretskii
@ 2019-03-09 19:25             ` Keith Marshall
  2019-03-10 19:03               ` Bruno Haible
  1 sibling, 1 reply; 15+ messages in thread
From: Keith Marshall @ 2019-03-09 19:25 UTC (permalink / raw)
  To: Eli Zaretskii, Bruno Haible; +Cc: eggert, bug-gnulib


[-- Attachment #1.1: Type: text/plain, Size: 2427 bytes --]

On 19/02/19 17:18, Eli Zaretskii wrote:
>> From: Bruno Haible <bruno@clisp.org>
>> Cc: Eli Zaretskii <eliz@gnu.org>, Paul Eggert <eggert@cs.ucla.edu>
>> Date: Tue, 19 Feb 2019 00:32:24 +0100
>>
>> Hi Eli,
>>
>>>      ./alloca.h:40:18: error: inlining failed in call to always_inline '__builtin_alloca': function not considered for inlining
>>>       #  define alloca __builtin_alloca
>>> 		       ^
>>
>> GCC never inlines a function that calls alloca() or
>> __builtin_alloca().

With respect, while that may be true of ancient historical versions of
GCC, my own testing shows it to be untrue for modern releases; I cannot
reproduce[1] anything resembling the above diagnostic, unless I resort
to archaeological investigation, compiling with mingw32-gcc-3.4.5.  The
next version, which I still have available, is mingw32-gcc-4.8.2; with
that, and with all subsequent releases up to mingw32-gcc-8.2.0, other
than at -O0, GCC generates identical code for alloca() expressed as an
in-line function, to that which is generated when it is expressed as a
macro.  (Furthermore, the difference in code generated at -O0 is
inconsequential, and will have no effect on behaviour).

>> The reason is that if you call this function in a loop, then
>> without inlining it will consume a bounded amount of stack whereas
>> with inlining it might cause a stack overflow.

I'm sorry to say this, but that just seems disingenuous.  For every
version of GCC which I have available, *including* mingw32-gcc-3.4.5, if
I place a call to alloca(), *implemented as a macro*, within a loop,
then the stack grows on each iteration of the loop.  The growth is not
bounded, even for your macro implementation, with mingw32-gcc-3.4.5, but
for all versions from mingw32-gcc-4.8.2 onwards it *is* bounded, (by
calling __chkstk_ms(), before adjusting the stack pointer), for *both*
your macro implementation, and for in-line function expansion.

[1] Well, I actually can reproduce it, even with mingw32-gcc-8.2.0, but
*only* when I deliberately introduce the very bug which Eli has noted,
*and* I compile at -O0; at any other optimization level, this gnulib bug
seems to be optimized away, with __builtin_alloca() itself being
expanded to in-line intrinsic code, as intended.

-- 
Regards,
Keith.

Public key available from keys.gnupg.net
Key fingerprint: C19E C018 1547 DE50 E1D4 8F53 C0AD 36C6 347E 5A3F


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Gnulib's alloca.h used even when there is a system header
  2019-03-09 19:25             ` Keith Marshall
@ 2019-03-10 19:03               ` Bruno Haible
  2019-03-10 19:39                 ` Eli Zaretskii
  0 siblings, 1 reply; 15+ messages in thread
From: Bruno Haible @ 2019-03-10 19:03 UTC (permalink / raw)
  To: Keith Marshall; +Cc: Eli Zaretskii, eggert, bug-gnulib

Hi Keith,

1)
> >> GCC never inlines a function that calls alloca() or
> >> __builtin_alloca().
> 
> With respect, while that may be true of ancient historical versions of
> GCC, my own testing shows it to be untrue for modern releases

Indeed. My statement above is true for gcc 2.95.3, but wrong for gcc
3.1 to 8.3.0. Tested with this program:

=============================================================
#include <alloca.h>
#include <stdio.h>
#include <stdlib.h>

static __attribute__((__always_inline__)) int
square (int x)
{
  alloca (1);
  return x * x;
}

int
main (int argc, char *argv[])
{
  int n = atoi (argv[1]);
  int x = atoi (argv[2]);
  int i;

  for (i = 0; i < n; i++)
    printf ("%d -> %d\n", x, square (x));

  return 0;
}
=============================================================
compiled with "-S -Winline -O -finline-functions foo.c".

2)
> >> The reason is that if you call this function in a loop, then
> >> without inlining it will consume a bounded amount of stack whereas
> >> with inlining it might cause a stack overflow.
> 
> I'm sorry to say this, but that just seems disingenuous.

Don't call me "disingenuous", because this is a personal attack.
Just because I make a statement based on my understanding of the
semantics of inline functions, and you make a different statement
based on testing with GCC, that just makes my statement wrong.
Don't assume bad intentions!!!

> For every
> version of GCC which I have available, *including* mingw32-gcc-3.4.5, if
> I place a call to alloca(), *implemented as a macro*, within a loop,
> then the stack grows on each iteration of the loop.

The stack grows at each iteration of the loop, if
  - alloca is implemented as a macro,
  - or if alloca is implemented as an inline function AND the compiler is GCC.

The stack does NOT grow at each iteration of the loop, if
  - alloca is implemented as an inline function AND the compiler is clang.

Tested with this program:
===========================================================
#include <alloca.h>
#include <stdio.h>
#include <stdlib.h>

#ifdef AS_MACRO
#define my_alloca(x) __builtin_alloca (x)
#else
static __attribute__((__always_inline__)) inline char *
my_alloca (int x)
{
  return __builtin_alloca (x);
}
#endif

int
main (int argc, char *argv[])
{
  int n = atoi (argv[1]);
  int i;

  for (i = 0; i < n; i++)
    {
      char *y = my_alloca (1000);
      printf ("%p\n", y);
    }

  return 0;
}
===========================================================

These invocations show 10 different values for variables on
the stack:
$ gcc-version 8.3.0 -m32 -Winline -O2 -finline-functions foo.c && ./a.out 10
$ gcc-version 8.3.0 -m32 -Winline -O2 -finline-functions -DAS_MACRO foo.c && ./a.out 10
$ gcc-version 4.2.4 -m32 -Winline -O2 -finline-functions foo.c && ./a.out 10                
$ gcc-version 4.2.4 -m32 -Winline -O2 -finline-functions -DAS_MACRO foo.c && ./a.out 10       
$ gcc-version 8.3.0 -m32 -Winline -O0 foo.c && ./a.out 10                                          
$ gcc-version 8.3.0 -m32 -Winline -O0 -DAS_MACRO foo.c && ./a.out 10                               
$ gcc-version 4.2.4 -m32 -Winline -O0 foo.c && ./a.out 10
$ gcc-version 4.2.4 -m32 -Winline -O0 -DAS_MACRO foo.c && ./a.out 10
$ clang -m32 -O0 -DAS_MACRO foo.c && ./a.out 10
$ clang -m32 -O2 -finline-functions -DAS_MACRO foo.c && ./a.out 10

On the other hand:

$ clang -m32 -O0 foo.c && ./a.out 10                                                                  
0xff992010
0xff992010
0xff992010
0xff992010
0xff992010
0xff992010
0xff992010
0xff992010
0xff992010
0xff992010
$ clang -m32 -O2 -finline-functions foo.c && ./a.out 10                                                            
0xffeb513c
0xffeb513c
0xffeb513c
0xffeb513c
0xffeb513c
0xffeb513c
0xffeb513c
0xffeb513c
0xffeb513c
0xffeb513c

In summary, your inline-function based implementation of alloca
works as long as the compiler is GCC. It will break when someone
attempts to use clang with mingw.

3) Back to Eli's problem.

> [1] Well, I actually can reproduce it, even with mingw32-gcc-8.2.0, but
> *only* when I deliberately introduce the very bug which Eli has noted,
> *and* I compile at -O0; at any other optimization level, this gnulib bug
> seems to be optimized away, with __builtin_alloca() itself being
> expanded to in-line intrinsic code, as intended.

Thanks for the additional detail that it depends on the optimization
level.

> The other part of the puzzle is that stdlib.h does this:
>
>   # include "alloca.h"
>
> so Gnulib's alloca.h is bypassed.

So, Gnulib's alloca.h and the system's alloca.h are both included, and
the system's alloca.h comes last. To avoid this kind of trouble, we
need to make use of '#include_next <alloca.h>'. I think this patch
should do it. Can you please review it, Eli?
(Since it includes <alloca.h> only when GCC or clang is present,
it can assume include_next. Since it can assume include_next, it
does not need the absolute file name of the system's <alloca.h>,
and therefore it is irrelevant whether the file is empty after
preprocessing or not.)


diff --git a/lib/alloca.in.h b/lib/alloca.in.h
index 8aaf64d..84d92e6 100644
--- a/lib/alloca.in.h
+++ b/lib/alloca.in.h
@@ -36,6 +36,12 @@
 
 #ifndef alloca
 # ifdef __GNUC__
+   /* Some version of mingw have an <alloca.h> that causes trouble when
+      included after 'alloca' gets defined as a macro.  As a workaround, include
+      this <alloca.h> first and define 'alloca' as a macro afterwards.  */
+#  if (defined _WIN32 && ! defined __CYGWIN__) && @HAVE_ALLOCA_H@
+#   include_next <alloca.h>
+#  endif
 #  define alloca __builtin_alloca
 # elif defined _AIX
 #  define alloca __alloca
diff --git a/m4/alloca.m4 b/m4/alloca.m4
index 46d60f9..29bd289 100644
--- a/m4/alloca.m4
+++ b/m4/alloca.m4
@@ -1,4 +1,4 @@
-# alloca.m4 serial 14
+# alloca.m4 serial 15
 dnl Copyright (C) 2002-2004, 2006-2007, 2009-2019 Free Software Foundation,
 dnl Inc.
 dnl This file is free software; the Free Software Foundation
@@ -37,6 +37,13 @@ AC_DEFUN([gl_FUNC_ALLOCA],
   fi
   AC_SUBST([ALLOCA_H])
   AM_CONDITIONAL([GL_GENERATE_ALLOCA_H], [test -n "$ALLOCA_H"])
+
+  if test $ac_cv_working_alloca_h = yes; then
+    HAVE_ALLOCA_H=1
+  else
+    HAVE_ALLOCA_H=0
+  fi
+  AC_SUBST([HAVE_ALLOCA_H])
 ])
 
 # Prerequisites of lib/alloca.c.
diff --git a/modules/alloca-opt b/modules/alloca-opt
index d4468de..53bb28d 100644
--- a/modules/alloca-opt
+++ b/modules/alloca-opt
@@ -21,7 +21,7 @@ if GL_GENERATE_ALLOCA_H
 alloca.h: alloca.in.h $(top_builddir)/config.status
 	$(AM_V_GEN)rm -f $@-t $@ && \
 	{ echo '/* DO NOT EDIT! GENERATED AUTOMATICALLY! */'; \
-	  cat $(srcdir)/alloca.in.h; \
+	  sed -e 's|@''HAVE_ALLOCA_H''@|$(HAVE_ALLOCA_H)|g' < $(srcdir)/alloca.in.h; \
 	} > $@-t && \
 	mv -f $@-t $@
 else



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: Gnulib's alloca.h used even when there is a system header
  2019-03-10 19:03               ` Bruno Haible
@ 2019-03-10 19:39                 ` Eli Zaretskii
  2019-03-10 19:54                   ` Bruno Haible
  0 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2019-03-10 19:39 UTC (permalink / raw)
  To: Bruno Haible; +Cc: bug-gnulib, eggert, keith

> From: Bruno Haible <bruno@clisp.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, bug-gnulib@gnu.org, eggert@cs.ucla.edu
> Date: Sun, 10 Mar 2019 20:03:33 +0100
> 
> So, Gnulib's alloca.h and the system's alloca.h are both included, and
> the system's alloca.h comes last. To avoid this kind of trouble, we
> need to make use of '#include_next <alloca.h>'. I think this patch
> should do it. Can you please review it, Eli?

It LGTM, since I already succeeded to build that package with this:

> +#  if (defined _WIN32 && ! defined __CYGWIN__) && @HAVE_ALLOCA_H@

replaced by just "#if HAVE_ALLOCA_H", and because the 2 additional
conditions you propose are obviously true for MinGW.

> +  if test $ac_cv_working_alloca_h = yes; then
> +    HAVE_ALLOCA_H=1
> +  else
> +    HAVE_ALLOCA_H=0
> +  fi
> +  AC_SUBST([HAVE_ALLOCA_H])
>  ])

This cannot hurt, but in the case in point the package was already
testing for alloca.h, and defined HAVE_ALLOCA_H by itself.

Thanks.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Gnulib's alloca.h used even when there is a system header
  2019-03-10 19:39                 ` Eli Zaretskii
@ 2019-03-10 19:54                   ` Bruno Haible
  2019-03-10 20:00                     ` Eli Zaretskii
  0 siblings, 1 reply; 15+ messages in thread
From: Bruno Haible @ 2019-03-10 19:54 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: bug-gnulib, eggert, keith

Eli Zaretskii wrote:
> It LGTM, since I already succeeded to build that package with this:

Thanks for the confirmation.

> > +#  if (defined _WIN32 && ! defined __CYGWIN__) && @HAVE_ALLOCA_H@
> 
> replaced by just "#if HAVE_ALLOCA_H"

Right, AC_FUNC_ALLOCA already defines HAVE_ALLOCA_H as a preprocessor
macro, under the same conditions. But we have the habit, when we
create a .h file from a .in.h file, to substitute values found at
configure time directly. This
  - makes it possible to reference the same variables in the
    module description,
  - increases the probability that the .h file works even when the
    user has forgotten to #include <config.h> or adds or removes
    a macro definition through #define or #undef,
  - increases transparency in case of a compilation error.

Pushed.

Bruno



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Gnulib's alloca.h used even when there is a system header
  2019-03-10 19:54                   ` Bruno Haible
@ 2019-03-10 20:00                     ` Eli Zaretskii
  0 siblings, 0 replies; 15+ messages in thread
From: Eli Zaretskii @ 2019-03-10 20:00 UTC (permalink / raw)
  To: Bruno Haible; +Cc: bug-gnulib, eggert, keith

> From: Bruno Haible <bruno@clisp.org>
> Cc: keith@users.osdn.me, bug-gnulib@gnu.org, eggert@cs.ucla.edu
> Date: Sun, 10 Mar 2019 20:54:20 +0100
> 
> Pushed.

Thank you.

And thanks to Keith for his thorough research.


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2019-03-10 20:00 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-17 17:20 Gnulib's alloca.h used even when there is a system header Eli Zaretskii
2019-02-17 20:06 ` Paul Eggert
2019-02-17 20:24   ` Eli Zaretskii
2019-02-17 21:09     ` Paul Eggert
2019-02-17 23:32       ` Bruno Haible
2019-02-18 16:59       ` Eli Zaretskii
2019-02-18 23:32         ` Bruno Haible
2019-02-19 17:18           ` Eli Zaretskii
2019-02-28 18:40             ` Eli Zaretskii
2019-03-09 19:25             ` Keith Marshall
2019-03-10 19:03               ` Bruno Haible
2019-03-10 19:39                 ` Eli Zaretskii
2019-03-10 19:54                   ` Bruno Haible
2019-03-10 20:00                     ` Eli Zaretskii
2019-02-17 23:28 ` Bruno Haible

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).