Re: assertion failure / double destruction triggered by parser::symbol_type's move constructor

[Akim Demaille]

Hi Wolfgang,

Thanks a lot for this very precise (and up to date!) bug report!

> Le 23 déc. 2018 à 12:21, Wolfgang Thaller <wolfgang.thal...@gmx.net> a écrit :
> 
> In bison 3.2.3, given a parser definition that starts like this:
> [...]
> Assertion failed: (yytypeid_), function as, file 
> /Users/wolfgang/Projects/Retro68-build/build-host/Rez/RezParser.generated.hh, 
> line 370.
> 
> The reason is that variant::destroy<int> gets called twice.
> When basic_symbol<by_type>::move is used, variant::move is invoked, which 
> destroys the source, and by_type::move is invoked, which resets the source’s 
> type to empty.
> When basic_symbol’s move constructor is used, variant::move is invoked as 
> well, but by_type’s constructor is used. Because by_type has no move 
> constructor, it never resets the source object’s type to empty, and so the 
> variant will be desrtroyed again when basic_symbol’s destructor is invoked.
> 
> Suggested fix: by_type needs a move constructor that acts like by_type::move.

Yes, I agree.  See the patch below.

> I also suggest that the fact that yy:: parser_class_name::symbol_type is no 
> longer copy-constructible in bison 3.2 should be listed as a breaking change 
> from bison 3.0.

The thing is that I don't know how people are using these types: they do things 
that are not documented, and as such, are not considered part of the contract.

Yet I'd be very happy to learn how people use them, and ensure that we preserve 
the expected API (if it's reasonable).  I guess you'd like symbol_type to be 
regular.

Why exactly do you need the copy constructor?  symbol_type was designed to be 
the return value of yylex, so it's not really needed.  I need to know how 
people use it :)

Another question: I hate breaking changes, especially when it means that you 
have to write workarounds depending on the versions of the tools.  If I release 
3.2.4 soon, does it save you from such hassle, or it makes no difference and it 
can wait 3.3?

I'm fine with releasing 3.2.4 soon with this fix, and the copy support for 
symbol_type if you think it makes sense.


commit 62e38c557efc67141fa533f1e159c18674b9095f
Author: Akim Demaille <akim.demai...@gmail.com>
Date:   Sun Dec 23 19:37:30 2018 +0100

    c++: fix double free when a symbol_type was moved
    
    Currently the following piece of code crashes (with parse.assert),
    because we don't record that s was moved-from, and we invoke its dtor.
    
        {
          auto s = parser::make_INT (42);
          auto s2 = std::move (s);
        }
    
    Reported by Wolfgang Thaller.
    http://lists.gnu.org/archive/html/bug-bison/2018-12/msg00077.html
    
    * data/c++.m4 (by_type): Provide a move-ctor.
    (basic_symbol): Be sure not to read a moved-from value.
    * tests/c++.at (C++ Variant-based Symbols Unit Tests): Check this case.

diff --git a/data/c++.m4 b/data/c++.m4
index fed06f6b..84586007 100644
--- a/data/c++.m4
+++ b/data/c++.m4
@@ -293,6 +293,11 @@ m4_define([b4_symbol_type_declare],
       /// Copy constructor.
       by_type (const by_type& that);
 
+#if 201103L <= YY_CPLUSPLUS
+      /// Move constructor.
+      by_type (by_type&& that);
+#endif
+
       /// The symbol type as needed by the constructor.
       typedef token_type kind_type;
 
@@ -345,7 +350,7 @@ m4_define([b4_public_types_define],
     , value (]b4_variant_if([], [YY_MOVE (that.value)]))b4_locations_if([
     , location (YY_MOVE (that.location))])[
   {]b4_variant_if([
-    b4_symbol_variant([that.type_get ()], [value], [YY_MOVE_OR_COPY],
+    b4_symbol_variant([this->type_get ()], [value], [YY_MOVE_OR_COPY],
                       [YY_MOVE (that.value)])])[
   }
 
@@ -427,6 +432,14 @@ m4_define([b4_public_types_define],
     : type (that.type)
   {}
 
+#if 201103L <= YY_CPLUSPLUS
+  ]b4_inline([$1])b4_parser_class_name[::by_type::by_type (by_type&& that)
+    : type (that.type)
+  {
+    that.clear ();
+  }
+#endif
+
   ]b4_inline([$1])b4_parser_class_name[::by_type::by_type (token_type t)
     : type (yytranslate_ (t))
   {}
diff --git a/tests/c++.at b/tests/c++.at
index 7867e0cb..73ac580c 100644
--- a/tests/c++.at
+++ b/tests/c++.at
@@ -160,6 +160,17 @@ int main()
     assert_eq (s.value.as<int> (), 12);
   }
 
+  // symbol_type: move constructor.
+#if 201103L <= YY_CPLUSPLUS
+  {
+    auto s = parser::make_INT (42);
+    auto s2 = std::move (s);
+    assert_eq (s2.value.as<int> (), 42);
+    // Used to crash here, because s was improperly cleared, and
+    // its destructor tried to delete its (moved) value.
+  }
+#endif
+
   // stack_symbol_type: construction, accessor.
   {
 #if 201103L <= YY_CPLUSPLUS