crash/hang with gcc 5+ -O2 and --enable-zsh-mem

crash/hang with gcc 5+ -O2 and --enable-zsh-mem

[Mikael Magnusson]

I figured out this was because of -foptimize-strlen which somehow
causes calloc() to recurse infinitely and crash (with -O2 which also
enables -foptimize-sibling-calls it just hangs with no crash because
it doesn't consume extra stack, i presume). What I don't know is how
to fix it, or the causal relationship between strlen optimization and
calloc recursing infinitely. I've tried gcc 5.4, 6.2 and 6.4 with the
same result. Compiling only Src/mem.c with the problematic flag is
enough so it's something in there, but removing the one strlen() call
in the file has no effect.

-- 
Mikael Magnusson

Re: crash/hang with gcc 5+ -O2 and --enable-zsh-mem

[Joey Pabalinas]

[-- Attachment #1: Type: text/plain, Size: 2259 bytes --]

On Sat, Feb 24, 2018 at 08:01:28PM +0100, Mikael Magnusson wrote:
> I figured out this was because of -foptimize-strlen which somehow
> causes calloc() to recurse infinitely and crash (with -O2 which also
> enables -foptimize-sibling-calls it just hangs with no crash because
> it doesn't consume extra stack, i presume). What I don't know is how
> to fix it, or the causal relationship between strlen optimization and
> calloc recursing infinitely. I've tried gcc 5.4, 6.2 and 6.4 with the
> same result. Compiling only Src/mem.c with the problematic flag is
> enough so it's something in there, but removing the one strlen() call
> in the file has no effect.

I am also seeing the same behavior, and to be completely honest I have no
idea how strlen() is related either. Some testing in gdb did show that you
were right; the problem is a loop caused by a repeated jump back to
line 1719, which definitely lines up with the `-foptimize-sibling-calls`
part.

The only fix I could find which didn't requiring substantial
reimplementation of the memory management functions was to replace
the malloc() call in calloc() with realloc() instead. With a NULL `p`
argument realloc() behaves exactly the same as malloc() does, and
(at least on my system) gcc doesn't seem to consider realloc() a
candidate for sibling call optimizations; give this patch a try
and _hopefully_ this is a viable solution.

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>

 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Src/mem.c b/Src/mem.c
index 840bbb6e4a4eb6fd73..8b0dafed6fdae35a61 100644
--- a/Src/mem.c
+++ b/Src/mem.c
@@ -1714,12 +1714,18 @@ MALLOC_RET_T
 calloc(MALLOC_ARG_T n, MALLOC_ARG_T size)
 {
     long l;
-    char *r;
+    char *r = NULL;
 
     if (!(l = n * size))
 	return (MALLOC_RET_T) m_high;
 
-    r = malloc(l);
+    /*
+     * use realloc() (with a NULL `p` argument it behaves exactly the same
+     * as malloc() does) to prevent an infinite loop caused by sibling-call
+     * optimizations (the malloc() call would otherwise be replaced by an
+     * unconditional branch back to line 1719 ad infinitum).
+     */
+    r = realloc(r, l);
 
     memset(r, 0, l);
 
-- 
2.16.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: crash/hang with gcc 5+ -O2 and --enable-zsh-mem

[Joey Pabalinas]

[-- Attachment #1: Type: text/plain, Size: 1513 bytes --]

On Sat, Feb 24, 2018 at 02:13:34PM -1000, Joey Pabalinas wrote:
> The only fix I could find which didn't requiring substantial
> reimplementation of the memory management functions was to replace
> the malloc() call in calloc() with realloc() instead. With a NULL `p`
> argument realloc() behaves exactly the same as malloc() does, and
> (at least on my system) gcc doesn't seem to consider realloc() a
> candidate for sibling call optimizations; give this patch a try
> and _hopefully_ this is a viable solution.

On second thought, doing it this way is probably a *little* bit better; the
needless initialization of `r` to NULL is avoided, and it also makes the
purpose of using realloc() over malloc() a *tiny* bit more explicit:

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>

 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Src/mem.c b/Src/mem.c
index 840bbb6e4a4eb6fd73..f1208197b3ddac2139 100644
--- a/Src/mem.c
+++ b/Src/mem.c
@@ -1719,7 +1719,13 @@ calloc(MALLOC_ARG_T n, MALLOC_ARG_T size)
     if (!(l = n * size))
 	return (MALLOC_RET_T) m_high;
 
-    r = malloc(l);
+    /*
+     * use realloc() (with a NULL `p` argument it behaves exactly the same
+     * as malloc() does) to prevent an infinite loop caused by sibling-call
+     * optimizations (the malloc() call would otherwise be replaced by an
+     * unconditional branch back to line 1719 ad infinitum).
+     */
+    r = realloc(NULL, l);
 
     memset(r, 0, l);
 
-- 
2.16.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: crash/hang with gcc 5+ -O2 and --enable-zsh-mem

[Joey Pabalinas]

[-- Attachment #1: Type: text/plain, Size: 361 bytes --]

On Sat, Feb 24, 2018 at 09:06:37PM -1000, Joey Pabalinas wrote:
> On second thought, doing it this way is probably a *little* bit better; the
> needless initialization of `r` to NULL is avoided, and it also makes the
> purpose of using realloc() over malloc() a *tiny* bit more explicit:

Whoops, sorry about the double send there.

-- 
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: crash/hang with gcc 5+ -O2 and --enable-zsh-mem

[Peter Stephenson]

On Sat, 24 Feb 2018 21:06:37 -1000
Joey Pabalinas <joeypabalinas@gmail.com> wrote:
> diff --git a/Src/mem.c b/Src/mem.c
> index 840bbb6e4a4eb6fd73..f1208197b3ddac2139 100644
> --- a/Src/mem.c
> +++ b/Src/mem.c
> @@ -1719,7 +1719,13 @@ calloc(MALLOC_ARG_T n, MALLOC_ARG_T size)
>      if (!(l = n * size))
>  	return (MALLOC_RET_T) m_high;
>  
> -    r = malloc(l);
> +    /*
> +     * use realloc() (with a NULL `p` argument it behaves exactly the same
> +     * as malloc() does) to prevent an infinite loop caused by sibling-call
> +     * optimizations (the malloc() call would otherwise be replaced by an
> +     * unconditional branch back to line 1719 ad infinitum).
> +     */
> +    r = realloc(NULL, l);
>  
>      memset(r, 0, l);

Was going to object some older realloc()s don't support that behaviour,
but this is quite specifically in zsh memory management where realloc()
is explicitly defined, duh.

Can't see a problem with this apart from a minor performance hit.
calloc isn't used anywhere this would be a big deal, I don't think.  I
committed it, thanks.

I suppose gcc really wants looking at too...

pws

Re: crash/hang with gcc 5+ -O2 and --enable-zsh-mem

[Joey Pabalinas]

[-- Attachment #1: Type: text/plain, Size: 880 bytes --]

On Mon, Feb 26, 2018 at 08:24:46PM +0000, Peter Stephenson wrote:
> Can't see a problem with this apart from a minor performance hit.
> calloc isn't used anywhere this would be a big deal, I don't think.  I
> committed it, thanks.
> 
> I suppose gcc really wants looking at too...

Cheers.

I honestly still am completely confused at how strlen() could in any
way be related to this (and yet, it most certainly is as far as gcc
is concered) and _why_ calloc() is even a viable candidate for malloc()
sibling-call optimization in the first place.

I am also sort of thinking this is a gcc bug, but I have no idea
where to even start looking in that abyss known to some as the
gcc codebase. I'll keep my ear to the ground regardless; I find it
*very* hard to believe that zsh is the only codebase affected
by this optimization peculiarity.

-- 
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]