Re: duplicated user destructor for lookahead

[Joel E. Denny]

On Thu, 15 Sep 2005, Joel E. Denny wrote:

> On Wed, 14 Sep 2005, Joel E. Denny wrote:
>
> > User destructors are called on only one stack.  See:
> >
> >   http://lists.gnu.org/archive/html/bison-patches/2005-08/msg00029.html
>
> <snip>
>
> > Because the lookahead needs to remain available to be shifted onto the
> > other stacks.
>
> I believe it's clear there's a potential memory leak.  I'll revise.

On third thought, I'm not sure I did create a new memory leak.  If the
lookahead has been shifted onto any stack, it's been shifted onto stack 0,
and that's the stack for which user destructors are called in the event of
memory exhaustion.

In any case, I believe the following patch is cleaner than my last one.
Its approach is simply to synchronize the shift for all stacks.  Embedded
comments should explain the rest.  Let me know if it's unclear.

I also discovered what appears to be another bug in the code, but I
haven't written a test case. In the event of memory exhaustion, if stack 0
happens to be marked for deletion (guaranteeing that in a test case is not
something I want to think about), no user destructors are called on any
symbols.  At first glance, it appears that perhaps another stack could be
iterated in its place in order to call user destructors.  However, that
`If stack is well-formed' comment has me scared to do this.  Can anyone
explain this comment?

Joel

Index: glr.c
===================================================================
RCS file: /cvsroot/bison/bison/data/glr.c,v
retrieving revision 1.124
diff -p -u -r1.124 glr.c
--- glr.c       16 Sep 2005 22:54:21 -0000      1.124
+++ glr.c       20 Sep 2005 05:28:58 -0000
@@ -671,10 +671,10 @@ typedef short int yySymbol;
 typedef short int yyItemNum;

 typedef struct yyGLRState yyGLRState;
+typedef struct yyGLRStateSet yyGLRStateSet;
 typedef struct yySemanticOption yySemanticOption;
 typedef union yyGLRStackItem yyGLRStackItem;
 typedef struct yyGLRStack yyGLRStack;
-typedef struct yyGLRStateSet yyGLRStateSet;

 struct yyGLRState {
   /** Type tag: always true. */
@@ -1808,16 +1808,7 @@ yyprocessOneStack (yyGLRStack* yystack,
            }

          if (yyisShiftAction (yyaction))
-           {
-             YYDPRINTF ((stderr, "On stack %lu, ", (unsigned long int) yyk));
-             YY_SYMBOL_PRINT ("shifting", *yytokenp, yylvalp, yyllocp);
-             yyglrShift (yystack, yyk, yyaction, yyposn+1,
-                         *yylvalp, yyllocp);
-             YYDPRINTF ((stderr, "Stack %lu now in state #%d\n",
-                         (unsigned long int) yyk,
-                         yystack->yytops.yystates[yyk]->yylrState));
-             break;
-           }
+           break;
          else if (yyisErrorAction (yyaction))
            {
              YYDPRINTF ((stderr, "Stack %lu dies.\n",
@@ -2165,14 +2156,56 @@ b4_syncline(address@hidden@], address@hidden@])])dnl

       while (yytrue)
        {
+         yySymbol yytoken_to_shift;
          size_t yys;
          size_t yyn = yystack.yytops.yysize;
+         /*
+          There are 3 cases in which yyprocessOneStack() returns:
+            - Returns error flag.  If the caller is yyprocessOneStack(), it
+              immediately returns as well.  When the caller is finally
+              yyparse(), it jumps to an error label via YYCHK1().
+            - Returns yyok but has invoked yymarkStackDeleted (&yystack, yys),
+              which sets the top state of yys to NULL.  Thus, yyparse()'s
+              following invocation of yyremoveDeletes() will remove the
+              stack.
+            - Returns yyok when ready to shift a token.
+          Except in case 1, yyparse() will invoke yyremoveDeletes() and then
+          shift the next token onto all remaining stacks.  This
+          synchronization of the shift (that is, after all preceding
+          reductions on all stacks) helps prevents double destructor calls on
+          yylval in the event of memory exhaustion.
+         */
          for (yys = 0; yys < yyn; yys += 1)
            YYCHK1 (yyprocessOneStack (&yystack, yys, yyposn,
                                       yylvalp, yyllocp]b4_lpure_args[));
+         yyremoveDeletes (&yystack);
+         yyn = yystack.yytops.yysize;
+         /*
+           If any yyglrShift() will fail, it will fail after shifting.  Thus,
+           a copy of yylval will already be on stack 0 in the event of a
+           failure in the following loop.  Thus, yytoken is set to YYEMPTY
+           before the loop to make sure the user destructor for yylval isn't
+           called twice.
+         */
+         yytoken_to_shift = yytoken;
          yytoken = YYEMPTY;
          yyposn += 1;
-         yyremoveDeletes (&yystack);
+         for (yys = 0; yys < yyn; yys += 1)
+            {
+             int yyaction;
+              const short int* yyconflicts;
+             yyStateNum yystate = yystack.yytops.yystates[yys]->yylrState;
+             yygetLRActions (yystate, yytoken_to_shift, &yyaction,
+                             &yyconflicts);
+             /* Note that yyconflicts were handled by yyprocessOneStack(). */
+             YYDPRINTF ((stderr, "On stack %lu, ", (unsigned long int) yys));
+             YY_SYMBOL_PRINT ("shifting", yytoken_to_shift, yylvalp, yyllocp);
+             yyglrShift (&yystack, yys, yyaction, yyposn,
+                         *yylvalp, yyllocp);
+             YYDPRINTF ((stderr, "Stack %lu now in state #%d\n",
+                         (unsigned long int) yys,
+                         yystack.yytops.yystates[yys]->yylrState));
+            }
          if (yystack.yytops.yysize == 0)
            {
              yyundeleteLastStack (&yystack);