Re: Bison 3.0.3 (stable)

[Akim Demaille]

> Le 20 janv. 2015 à 17:01, Thomas Jahns <address@hidden> a écrit :
> 
> Dear Akim,

Hi Thomas!

> On 01/19/15 19:05, Akim Demaille wrote:
>> 
>> Now I know what would be most interesting: debug traces!
>> 
>> Could you please run the same test with YYDEBUG=1 set in the environment?
>> 
>>   YYDEBUG=1 make check  TESTSUITEFLAGS='-d -v -x 426'
>> 
>> The test will sure fail even more, but we should also get a better
>> chance to compare my traces with yours.  And spot when the spurious
>> constructor was called.
> 
> I've created a run with -O3 and the 1.6 Java now. Please find the full log + 
> testsuite.dir for the regular make check at

Great, thanks!  For the records, I appended your stderr to this message.

The diff with mine (mine is -, yours is +) is:

--- _build/49s/tests/testsuite.dir/426/stderr   2015-01-20 18:22:46.000000000 
+0100
+++ /Users/akim/Downloads/tests/testsuite.dir/426/stderr.txt    2015-01-20 
16:55:12.000000000 +0100
@@ -14,20 +14,21 @@
 -> $$ = nterm list ((0))
 Destroy: "0"
 Stack now 0
 Entering state 4
 Reading a token: Next token is token "," ()
 Shifting token "," ()
 Destroy: (0)
 Entering state 8
 Reading a token: Next token is token NUMBER (1)
 Shifting token NUMBER (1)
+Destroy: (0)
 Entering state 2
 Reducing stack by rule 6 (line 91):
    $1 = token NUMBER (1)
 -> $$ = nterm item (1)
 Destroy: 1
 Stack now 8 4 0
 Entering state 9
 Reducing stack by rule 3 (line 85):
    $1 = nterm list ((0))
    $2 = token "," ()


This took me an embarrassingly long time to understand.  As a matter of fact
I did not understand why there was any "Destroy: (0)" at all.  The expected
output is:

Destroy: ""
Destroy: "0"
Destroy: (0)
Destroy: 1
Destroy: "1"
Destroy: ()
Destroy: ""
Destroy: "2"
Destroy: ()
Destroy: ""
Destroy: 3
Destroy: ()
Destroy: ""
Destroy: "4"
Destroy: ()
Destroy: ()
Destroy: 5
Destroy: ()
Destroy: ""
Destroy: "6"
Destroy: ()
Destroy: (0, 1, 2, 4, 6)

The lines "Destroy: "0"" or "Destroy: 1" make sense: we destroy
the element to push a copy in the list.  The "Destroy: ()" also
make sense: our rule:

list: list "," item { std::swap ($$, $1); $$.push_back ($3); }

first create an empty list in $$, then swaps it with $1.  So
when $1 is cleared, it is this fresh empty list that was initially
in $$ that is cleared.  All is well, again.

Of course at the end, the final "Destroy: (0, 1, 2, 4, 6)" is
expected: we discard the result of the whole process.

What does not make sense was the "Destroy: (0)" (and Thomas,
you actually have two such lines).  Where was this list coming
from?  This is actually because the stack on which the parser
pushes its value was too small, so the C++ library enlarged
the stack, which means allocating a new one, copying the content
of the first one, and then clearing the initial one.  _That_'s
the origin of this "Destroy: (0)".

I'm not sure why you have a second, however, let's not start
with a ridiculously small stack.  The C parsers start with
200 slots ready.  This appended patch does that.  Could you
check it on your machine?  It's also on next.

Thanks!


commit 573654ca9e8bc3ad9f5a00820417edcbca6e14dc
Author: Akim Demaille <address@hidden>
Date:   Tue Jan 20 20:41:57 2015 +0100

    c++: reserve 200 slots in the parser's stack
    
    This is consistent with what is done with yacc.c and glr.c.  Because
    it also avoids that the stack needs to be resized very soon, it should
    help keeping tests about destructors more reliable.
    
    Indeed, if the stack is created too small, very soon the C++ library
    needs to enlarge it, which means creating a new one, copying the
    elements from the initial one onto it, and then destroy the elements
    of the initial stack: that would be a spurious call to a destructor.
    
    Reported by Thomas Jahns.
    http://lists.gnu.org/archive/html/bug-bison/2015-01/msg00059.html
    
    * data/stack.hh (stack::stack): Reserve 200 slots.
    * tests/c++.at: Remove traces of stack expansions.

diff --git a/data/stack.hh b/data/stack.hh
index e7ff040..a56e6c7 100644
--- a/data/stack.hh
+++ b/data/stack.hh
@@ -32,12 +32,12 @@ m4_define([b4_stack_define],
     stack ()
       : seq_ ()
     {
+      seq_.reserve (200);
     }
 
     stack (unsigned int n)
       : seq_ (n)
-    {
-    }
+    {}
 
     inline
     T&
@@ -114,8 +114,7 @@ m4_define([b4_stack_define],
     slice (const S& stack, unsigned int range)
       : stack_ (stack)
       , range_ (range)
-    {
-    }
+    {}
 
     inline
     const T&
diff --git a/tests/c++.at b/tests/c++.at
index 062e5f0..55d7d40 100644
--- a/tests/c++.at
+++ b/tests/c++.at
@@ -342,7 +342,6 @@ AT_PARSER_CHECK([./list], 0,
 ]],
 [[Destroy: ""
 Destroy: "0"
-Destroy: (0)
 Destroy: 1
 Destroy: "1"
 Destroy: ()

stderr.txt
Description: Text document