[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: assertion failure / double destruction triggered by parser::symbol_t
From: |
Akim Demaille |
Subject: |
Re: assertion failure / double destruction triggered by parser::symbol_type's move constructor |
Date: |
Sun, 23 Dec 2018 19:52:18 +0100 |
Hi Wolfgang,
Thanks a lot for this very precise (and up to date!) bug report!
> Le 23 déc. 2018 à 12:21, Wolfgang Thaller <address@hidden> 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 <address@hidden>
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