bug#24358: 25.1.50; re-search-forward errors with "Variable binding depth exceeds max-specpdl-size"

[Eli Zaretskii <eliz@gnu.org>]

> From: npostavs@users.sourceforge.net
> Cc: 24358@debbugs.gnu.org, Sam Halliday <sam.halliday@gmail.com>
> Date: Tue, 18 Oct 2016 23:11:45 -0400
> 
> > Then yes, you will need to somehow pass down the object from which the
> > text comes.  It can be in some global variable, for instance, if
> > changing the function's signature is undesirable.
> 
> I decided that a global variable would be a bad idea since it would
> still effectively be part of the calling convention, and too easy for
> callers to forget.

That's true, but doing this is a standard Emacs coding practice, used
in quite a few other places.

> But just after finishing the patch I noticed this:
> 
>     /* In Emacs, this is the string or buffer in which we
>        are matching.  It is used for looking up syntax properties.  */
>     Lisp_Object re_match_object;

Indeed.

> So now I'm thinking it might be better to reuse that variable instead.
> Although it only seems to get to Qt, Qnil, and string objects; I'm not
> sure what the meaning of Qt and Qnil are.

nil means current buffer, t means a C string.  (This is standard Emacs
convention, used in other places as well, but you can verify it is
used in this case by looking at all the places where these values are
assigned.)  The last case, of a C string, doesn't need to bother about
relocation, of course (but you could ignore that distinction for
simplicity of the code, if you want, it will never do any harm).

I guess you will be rewriting your patch to use re_match_object?  I
expect it to be much simpler and smaller.  re_match_object is already
staticpro'd, btw, so you don't need to worry about GC for the Lisp
object (a string) that gets put in its value.  However, I think we
should assign Qnil to re_match_object as soon as re_match_2 returns,
to avoid having the string protected from GC for too long.

A couple of comments to the patch:

> +#ifdef emacs
> +#define STR_BASE_PTR(obj)                       \
> +    (BUFFERP (obj)? XBUFFER (obj)->text->beg :  \
> +     STRINGP (obj)? SDATA (obj) :               \
> +     NULL)

There's an unnecessary complication here: since, when regex functions
are invoked to search a buffer, that buffer is always the current
buffer, you don't need to pass the buffer object and use XBUFFER,
because the current buffer is always available in the global variable
current_buffer (which is a C struct, not a Lisp object).  So you could
adopt the usual semantics of passing Qnil to mean the current buffer
and a string object to mean that string.  Then the only test in the
macro should be STRINGP.

>  #define ENSURE_FAIL_STACK(space)					\
>  while (REMAINING_AVAIL_SLOTS <= space) {				\
> +  re_char* orig_base = STR_BASE_PTR (string_base);                      \
>    if (!GROW_FAIL_STACK (fail_stack))					\
> -    return -2;								\
> +    return -2;                                                          \
> +  /* GROW_FAIL_STACK may call malloc and relocate the string */         \
> +  /* pointers.  */                                                      \
> +  ptrdiff_t delta = STR_BASE_PTR (string_base) - orig_base;             \
> +  if (string1)                                                          \
> +    {                                                                   \
> +      string1 += delta;                                                 \
> +      end1 += delta;                                                    \
> +      end_match_1 += delta;                                             \
> +    }                                                                   \
> +  if (string2)                                                          \
> +    {                                                                   \
> +      string2 += delta;                                                 \
> +      end2 += delta;                                                    \
> +      end_match_2 += delta;                                             \
> +    }                                                                   \
> +  d += delta;                                                           \
> +  dend += delta;                                                        \
> +  dfail += delta;                                                       \

I think doing this via buffer and string positions instead would yield
slightly more portable code, since Standard C says subtraction of
pointers to different objects yields undefined behavior.

Thanks.