unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Strange code in emacs.c
@ 2018-10-03 15:15 Eli Zaretskii
  2018-10-03 17:36 ` Paul Eggert
  2018-10-03 22:12 ` Noam Postavsky
  0 siblings, 2 replies; 6+ messages in thread
From: Eli Zaretskii @ 2018-10-03 15:15 UTC (permalink / raw)
  To: Noam Postavsky; +Cc: emacs-devel

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);



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

* Re: Strange code in emacs.c
  2018-10-03 15:15 Strange code in emacs.c Eli Zaretskii
@ 2018-10-03 17:36 ` Paul Eggert
  2018-10-03 17:42   ` Eli Zaretskii
  2018-10-03 22:12 ` Noam Postavsky
  1 sibling, 1 reply; 6+ messages in thread
From: Paul Eggert @ 2018-10-03 17:36 UTC (permalink / raw)
  To: Eli Zaretskii, Noam Postavsky; +Cc: emacs-devel

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?



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

* Re: Strange code in emacs.c
  2018-10-03 17:36 ` Paul Eggert
@ 2018-10-03 17:42   ` Eli Zaretskii
  2018-10-03 18:29     ` Paul Eggert
  0 siblings, 1 reply; 6+ messages in thread
From: Eli Zaretskii @ 2018-10-03 17:42 UTC (permalink / raw)
  To: Paul Eggert; +Cc: npostavs, emacs-devel

> 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 ;-)



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

* Re: Strange code in emacs.c
  2018-10-03 17:42   ` Eli Zaretskii
@ 2018-10-03 18:29     ` Paul Eggert
  0 siblings, 0 replies; 6+ messages in thread
From: Paul Eggert @ 2018-10-03 18:29 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: npostavs, emacs-devel

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.




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

* Re: Strange code in emacs.c
  2018-10-03 15:15 Strange code in emacs.c Eli Zaretskii
  2018-10-03 17:36 ` Paul Eggert
@ 2018-10-03 22:12 ` Noam Postavsky
  2018-10-03 22:59   ` Paul Eggert
  1 sibling, 1 reply; 6+ messages in thread
From: Noam Postavsky @ 2018-10-03 22:12 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Emacs developers

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.  */



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

* Re: Strange code in emacs.c
  2018-10-03 22:12 ` Noam Postavsky
@ 2018-10-03 22:59   ` Paul Eggert
  0 siblings, 0 replies; 6+ messages in thread
From: Paul Eggert @ 2018-10-03 22:59 UTC (permalink / raw)
  To: Noam Postavsky, Eli Zaretskii; +Cc: Emacs developers

[-- 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


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

end of thread, other threads:[~2018-10-03 22:59 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-03 15:15 Strange code in emacs.c Eli Zaretskii
2018-10-03 17:36 ` Paul Eggert
2018-10-03 17:42   ` Eli Zaretskii
2018-10-03 18:29     ` Paul Eggert
2018-10-03 22:12 ` Noam Postavsky
2018-10-03 22:59   ` Paul Eggert

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

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).