Strange code in emacs.c

Strange code in emacs.c

[Eli Zaretskii]

We have this in 'main':

      emacs_re_safe_alloca = max
        (min (lim - extra, SIZE_MAX) * (min_ratio / ratio),
         MAX_ALLOCA);

This always yields MAX_ALLOCA because 'ratio' is always greater than
'min_ratio':

      int min_ratio = 20 * sizeof (char *);
      int ratio = min_ratio + min_ratio / 3;

Don't we mean to set emacs_re_safe_alloca like this instead:

      emacs_re_safe_alloca = min (lim - extra, SIZE_MAX) / ratio;
      emacs_re_safe_alloca = max (emacs_re_safe_alloca * min_ratio, MAX_ALLOCA);

Re: Strange code in emacs.c

[Paul Eggert]

Eli Zaretskii wrote:
> We have this in 'main':
> 
>        emacs_re_safe_alloca = max
>          (min (lim - extra, SIZE_MAX) * (min_ratio / ratio),
>           MAX_ALLOCA);
> 
> This always yields MAX_ALLOCA because 'ratio' is always greater than
> 'min_ratio':
> 
>        int min_ratio = 20 * sizeof (char *);
>        int ratio = min_ratio + min_ratio / 3;
> 
> Don't we mean to set emacs_re_safe_alloca like this instead:
> 
>        emacs_re_safe_alloca = min (lim - extra, SIZE_MAX) / ratio;
>        emacs_re_safe_alloca = max (emacs_re_safe_alloca * min_ratio, MAX_ALLOCA);

Yes, that sounds better to me too. Also, that SIZE_MAX should be changed to min 
(SIZE_MAX, PTRDIFF_MAX) to avoid (unlikely) integer-overflow issues.

Should this fix be put into master or into the emacs-26 branch?

Re: Strange code in emacs.c

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Wed, 3 Oct 2018 10:36:35 -0700
> 
> > Don't we mean to set emacs_re_safe_alloca like this instead:
> > 
> >        emacs_re_safe_alloca = min (lim - extra, SIZE_MAX) / ratio;
> >        emacs_re_safe_alloca = max (emacs_re_safe_alloca * min_ratio, MAX_ALLOCA);
> 
> Yes, that sounds better to me too. Also, that SIZE_MAX should be changed to min 
> (SIZE_MAX, PTRDIFF_MAX) to avoid (unlikely) integer-overflow issues.

OK, will do.

> Should this fix be put into master or into the emacs-26 branch?

It's a bit scary to do this on emacs-26, as the effect is a dramatic
increase in stack usage by regex routines on most platforms.  So I
thought about doing this on master.  But I'm eager to be convinced by
good arguments to the contrary ;-)

Re: Strange code in emacs.c

[Paul Eggert]

On 10/3/18 10:42 AM, Eli Zaretskii wrote:
> It's a bit scary to do this on emacs-26, as the effect is a dramatic
> increase in stack usage by regex routines on most platforms.

Ah, sorry, I didn't realize it would be that much of an increase. In 
that case the master branch sounds like a more plausible place for the 
patch to land. Would like to hear from Noam, though.

Re: Strange code in emacs.c

[Noam Postavsky]

On Wed, 3 Oct 2018 at 11:15, Eli Zaretskii <eliz@gnu.org> wrote:
>
> We have this in 'main':
>
>       emacs_re_safe_alloca = max
>         (min (lim - extra, SIZE_MAX) * (min_ratio / ratio),
>          MAX_ALLOCA);
>
> This always yields MAX_ALLOCA because 'ratio' is always greater than
> 'min_ratio':
>
>       int min_ratio = 20 * sizeof (char *);
>       int ratio = min_ratio + min_ratio / 3;

Yes, it seems I forgot how integer math works when I wrote that. I
guess that I meant to write it without the parens around the division
(which is approximately the same as your 2 statement suggestion):

      emacs_re_safe_alloca = max
        (min (lim - extra, SIZE_MAX) * min_ratio / ratio,
         MAX_ALLOCA);

Also, I forgot the word "use" in the comment just above:

      /* If the stack is big enough, let regex.c more of it before
                                                ^
                                                use
         falling back to heap allocation.  */

Re: Strange code in emacs.c

[Paul Eggert]

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

Thanks for checking; I installed the attached into the master branch.


[-- Attachment #2: 0001-Fix-emacs_re_safe_alloca-calculation.patch --]
[-- Type: text/x-patch, Size: 1268 bytes --]

From 8026f11696430bfdf6b38a4eef4cbf9e337cc947 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 3 Oct 2018 15:55:43 -0700
Subject: [PATCH] Fix emacs_re_safe_alloca calculation

Problem and draft fix noted by Eli Zaretskii in:
https://lists.gnu.org/r/emacs-devel/2018-10/msg00022.html
* src/emacs.c (main): Fix arithmetic used in calculation
of emacs_re_safe_alloca.
---
 src/emacs.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/emacs.c b/src/emacs.c
index b1c96d1828..ddaaf3fed5 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -888,11 +888,11 @@ main (int argc, char **argv)
 		lim = newlim;
 	    }
 	}
-      /* If the stack is big enough, let regex-emacs.c more of it before
-         falling back to heap allocation.  */
-      emacs_re_safe_alloca = max
-        (min (lim - extra, SIZE_MAX) * (min_ratio / ratio),
-         MAX_ALLOCA);
+      /* If the stack is big enough, let regex-emacs.c use more of it
+	 before falling back to heap allocation.  */
+      ptrdiff_t max_failures
+	= min (lim - extra, min (PTRDIFF_MAX, SIZE_MAX)) / ratio;
+      emacs_re_safe_alloca = max (max_failures * min_ratio, MAX_ALLOCA);
     }
 #endif /* HAVE_SETRLIMIT and RLIMIT_STACK and not CYGWIN */
 
-- 
2.17.1