bug#12242: Emacs 24.2 RC1 build fails on OpenBSD

[Eli Zaretskii]

> Date: Wed, 22 Aug 2012 12:13:27 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc:   jca@wxcvbn.org,
>       12242@debbugs.gnu.org
> 
> >>>>> On Wed, 22 Aug 2012 06:02:06 +0300, Eli Zaretskii <eliz@gnu.org> said:
> 
> >> Date: Wed, 22 Aug 2012 11:35:26 +0900 From: YAMAMOTO Mitsuharu
> >> <mituharu@math.s.chiba-u.ac.jp> Cc: jca@wxcvbn.org,
> >> 12242@debbugs.gnu.org
> >> 
> >> I took a look at ralloc.c a bit, and I thought that the variable
> >> `use_relocatable_buffers' is not designed to be changed temporarily
> >> in the first place.
> 
> > Why not?  Can you tell what led you to that conclusion?
> 
> > My reading of the code is that inhibiting relocation just means that
> > ralloc.c always asks for more memory when it cannot find a large
> > enough block in the existing heaps.
> 
> For example, `virtual_break_value' is not updated in that case.  It
> can lead to an inconsistent state as reported if r_alloc_sbrk is
> called with a positive argument via malloc when
> use_relocatable_buffers <= 0, and then it is called with a negative
> argument via free when use_relocatable_buffers > 0.

I see your point.

Unfortunately, using r_alloc_freeze/r_alloc_thaw doesn't seem to be
workable in practice, either.  I tried to use it, and the best patch I
could come up with (got me through several bootstraps with different
compiler switches) is below.  It is (a) butt-ugly, and (b) very
fragile, for the reasons I explain below.

Basically, the difficulty is to know which number to pass to
r_alloc_freeze.  The only place that knows how much memory is to be
allocated is the code that actually allocates it.  So I cannot put the
call to r_alloc_freeze in maybe_unify_char, where we now call
r_alloc_inhibit_buffer_relocation, because the memory allocations are
in the functions called from there, and the amount of memory is not
known to the caller in advance.  Moreover, at least one of the
functions called from maybe_unify_char via load_charset allocates
memory in a data-dependent loop, so I think it is in principle
impossible to know how much memory it can ask for.

What's worse, malloc (at least the implementation in gmalloc.c) will
actually allocate more than it was asked for, and sometimes allocates
an additional chunk of memory on top of that to expand its internal
tables where it maintains information about the arena.  The size of
this additional chunk is also data-dependent, so the value I add in
r_alloc_freeze in the patch below is not guaranteed to work, it's
based on what I saw during a bootstrap, with some safety margin added
for good measure.

So it sounds like the only practical way out of this mess is to step
back one notch and talk about bug #11519, which was the cause for
inhibiting relocations while maybe_unify_char is in progress.  At the
time, Handa-san promised to work on removing unify_char, but I guess
that job is not yet done, since it isn't even on the trunk.  Ken'ichi,
what would it take to do this now?

Another alternative would be to rewrite load_charset_map_from_file and
load_charset_map_from_vector so as not to allocate the large structs
they do now.  (These functions also create char-tables, but I think a
char-table is enlarged 256 elements at a time, which doesn't cross the
threshold of "large" allocations that can trigger relocation in
ralloc.c.  Am I missing something?)

Yet another possibility is to disable relocations entirely on all
platforms but MS-Windows (where the crash which triggered this bug
report doesn't happen, probably because there we reserve the memory at
startup in one contiguous block).

Any other suggestions and thoughts are welcome.

Last, but not least: I'm very sorry for this obstacle to making an
emergency release.  It always bewilders me how such problems can lie
dormant for so long, until the most un-opportune moment.

Here's the patch that I DON'T recommend to install:

--- src/ralloc.c~0      2012-06-29 17:50:48.000000000 +0300
+++ src/ralloc.c        2012-08-22 16:59:32.511794000 +0300
@@ -1022,12 +1022,22 @@ r_re_alloc (POINTER *ptr, SIZE size)
 void
 r_alloc_freeze (long int size)
 {
   if (! r_alloc_initialized)
     r_alloc_init ();
 
   /* If already frozen, we can't make any more room, so don't try.  */
   if (r_alloc_freeze_level > 0)
     size = 0;
+  else
+    {
+      /* malloc will usually ask for more than its callers, so ensure
+        we have some extra room.  */
+      size += (max (__malloc_extra_blocks, 64) + 1) * 4096;
+      /* gmalloc will sometimes need to enlarge its internal tables,
+        which asks for yet more memory.  */
+      size += size / 4;
+    }
   /* If we can't get the amount requested, half is better than nothing.  */
   while (size > 0 && r_alloc_sbrk (size) == 0)
     size /= 2;
--- src/charset.c~0     2012-06-29 17:50:48.000000000 +0300
+++ src/charset.c       2012-08-22 14:41:55.981714000 +0300
@@ -214,6 +214,10 @@ static struct
    text and a string data may be relocated.  */
 int charset_map_loaded;
 
+/* Flag used to signal to load_charset_* that it is unsafe to relocate
+   buffers (indirectly via calls to rel_alloc_* functions in ralloc.c).  */
+static int in_maybe_unify_char;
+
 struct charset_map_entries
 {
   struct {
@@ -293,7 +297,20 @@ load_charset_map (struct charset *charse
       else
        {
          if (! temp_charset_work)
-           temp_charset_work = xmalloc (sizeof (*temp_charset_work));
+           {
+#ifdef REL_ALLOC
+             /* Allocating heap memory screws callers of this
+                function through STRING_CHAR_* macros that hold C
+                pointers to buffer text, if REL_ALLOC is used.  */
+             if (in_maybe_unify_char)
+               r_alloc_freeze (sizeof (*temp_charset_work));
+#endif
+             temp_charset_work = xmalloc (sizeof (*temp_charset_work));
+#ifdef REL_ALLOC
+             if (in_maybe_unify_char)
+               r_alloc_thaw ();
+#endif
+           }
          if (control_flag == 1)
            {
              memset (temp_charset_work->table.decoder, -1,
@@ -498,8 +515,19 @@ load_charset_map_from_file (struct chars
 
   /* Use SAFE_ALLOCA instead of alloca, as `charset_map_entries' is
      large (larger than MAX_ALLOCA).  */
+#ifdef REL_ALLOC
+  /* The calls to SAFE_ALLOCA below can allocate heap memory, which
+     screws callers of this function through STRING_CHAR_* macros that
+     hold C pointers to buffer text, if REL_ALLOC is used.  */
+  if (in_maybe_unify_char)
+    r_alloc_freeze (sizeof (struct charset_map_entries));
+#endif
   SAFE_ALLOCA (head, struct charset_map_entries *,
               sizeof (struct charset_map_entries));
+#ifdef REL_ALLOC
+  if (in_maybe_unify_char)
+    r_alloc_thaw ();
+#endif
   entries = head;
   memset (entries, 0, sizeof (struct charset_map_entries));
 
@@ -530,8 +558,16 @@ load_charset_map_from_file (struct chars
 
       if (n_entries > 0 && (n_entries % 0x10000) == 0)
        {
+#ifdef REL_ALLOC
+         if (in_maybe_unify_char)
+           r_alloc_freeze (sizeof (struct charset_map_entries));
+#endif
          SAFE_ALLOCA (entries->next, struct charset_map_entries *,
                       sizeof (struct charset_map_entries));
+#ifdef REL_ALLOC
+         if (in_maybe_unify_char)
+           r_alloc_thaw ();
+#endif
          entries = entries->next;
          memset (entries, 0, sizeof (struct charset_map_entries));
        }
@@ -566,8 +602,19 @@ load_charset_map_from_vector (struct cha
 
   /* Use SAFE_ALLOCA instead of alloca, as `charset_map_entries' is
      large (larger than MAX_ALLOCA).  */
+#ifdef REL_ALLOC
+  /* The calls to SAFE_ALLOCA below can allocate heap memory, which
+     screws callers of this function through STRING_CHAR_* macros that
+     hold C pointers to buffer text, if REL_ALLOC is used.  */
+  if (in_maybe_unify_char)
+    r_alloc_freeze (sizeof (struct charset_map_entries));
+#endif
   SAFE_ALLOCA (head, struct charset_map_entries *,
               sizeof (struct charset_map_entries));
+#ifdef REL_ALLOC
+  if (in_maybe_unify_char)
+    r_alloc_thaw ();
+#endif
   entries = head;
   memset (entries, 0, sizeof (struct charset_map_entries));
 
@@ -603,8 +650,16 @@ load_charset_map_from_vector (struct cha
 
       if (n_entries > 0 && (n_entries % 0x10000) == 0)
        {
+#ifdef REL_ALLOC
+         if (in_maybe_unify_char)
+           r_alloc_freeze (sizeof (struct charset_map_entries));
+#endif
          SAFE_ALLOCA (entries->next, struct charset_map_entries *,
                       sizeof (struct charset_map_entries));
+#ifdef REL_ALLOC
+         if (in_maybe_unify_char)
+           r_alloc_thaw ();
+#endif
          entries = entries->next;
          memset (entries, 0, sizeof (struct charset_map_entries));
        }
@@ -1641,13 +1696,9 @@ maybe_unify_char (int c, Lisp_Object val
     return c;
 
   CHECK_CHARSET_GET_CHARSET (val, charset);
-#ifdef REL_ALLOC
-  /* The call to load_charset below can allocate memory, whcih screws
-     callers of this function through STRING_CHAR_* macros that hold C
-     pointers to buffer text, if REL_ALLOC is used.  */
-  r_alloc_inhibit_buffer_relocation (1);
-#endif
+  in_maybe_unify_char++;
   load_charset (charset, 1);
+  in_maybe_unify_char--;
   if (! inhibit_load_charset_map)
     {
       val = CHAR_TABLE_REF (Vchar_unify_table, c);
@@ -1662,9 +1713,6 @@ maybe_unify_char (int c, Lisp_Object val
       if (unified > 0)
        c = unified;
     }
-#ifdef REL_ALLOC
-  r_alloc_inhibit_buffer_relocation (0);
-#endif
   return c;
 }