GNOME Bugzilla – Bug 710034
parse: bison finds conflicts / ambiguities
Last modified: 2014-01-30 19:03:53 UTC
bison finds ambiguous grammar. reproduce message with: ~/gst/master/gstreamer$ make 2>&1 | grep -C5 conflic [..] make[4]: Entering directory `[..]/gst/master/gstreamer/gst/parse' GEN grammar.tab.h ./grammar.y: conflicts: 37 shift/reduce, 5 reduce/reduce Bison doc for this error: http://www.gnu.org/software/bison/manual/html_node/Shift_002fReduce.html Greetings Fabian
Did you check how many of these are real problems, and how many can be fixed without breaking backwards compat in the language? Should probably be fixed but I'm not aware of any problem this causes in practice for us.
what is the proper definition of the language? -gstreamer/tools/gst-launch.1.in -gstreamer/docs/manual/appendix-programs.xml -> "<title>Grammar Reference</title>" -or the current behaviour of gst-launch (which is hard to tell since grammar.y is inconsistent and bison just makes the best of it) For example: docs/* talks about "..property, optionally qualified with an element name. If the element is not specified, the previous element is assumed." man page talks only about properties set right after element creation gst-launch accepts: fakesrc name=foo name=bar fakesink name=go bar. ! go. gst-launch does not accept: fakesrc name=foo fakesink name=go foo.name=bar bar. ! go. having a look at parse.l and grammar.y i dont think there is a way to specify an element name for an assignment. i'd like to get grammar.y consistent and conforming to manual and man page. this should not break the language. but maybe some exotic lines out there make use of bugs/features that are created by bison trying to fix the grammar.
Specifying the element name for properties via element.property is not supported and can be dropped from the manpage. Unfortunately there is currently no proper definition of the language, but if you want to dig into this and collect all inconsistencies and get them fixed that would be great :)
Created attachment 259027 [details] [review] reorganized grammar tried to get grammar straight with minimal loss of backward compat. some structures could get their names re-factored, but wanted to keep diff small in first step. missing backward compat in following way: Bin's content can not start or end with LINK example #1) $ gst-launch ( fakesrc ! ) fakesink # does not work anymore $ gst-launch ( fakesrc ) ! fakesink # does work example #2) $ gst-launch fakesrc ( ! fakesink ) # does not work anymore $ gst-launch fakesrc ! ( fakesink ) # does work rationale) bins should behave similar to elements. imagine a element crazysrc which activly look for other elements and links to their pads. $ gst-launch crazysrc fakesink # this is not expected to work $ gst-launch fakesrc crazysink # this is also not expected to work
Created attachment 259028 [details] [review] reorganized grammar
Created attachment 259029 [details] grammar_overview.txt
Review of attachment 259028 [details] [review]: Looks good in general, but can you provide some additional unit tests that check if the parser does what it is supposed to do? Also did you check if all existing unit tests using the parser are still working fine? I think this single behaviour change is acceptable and a good idea btw.
(In reply to comment #6) > Created an attachment (id=259029) [details] > grammar_overview.txt 7 bin: '(' assignments chainlist ')' 8 | BINREF assignments chainlist ')' This should be | BINREF '(' assignments chainlist ')' right? Also I'm missing the "element.pad" and "element." syntax from that grammar, and maybe the "element::property" syntax but that is probably handled inside the code.
Created attachment 259802 [details] [review] make checks comform with proposed grammar change moves links out of bins. scary behavior: some errors are silently discarded with old grammar. please discuss if we need the parser guessing the meaning of bad input. the old parser has different kinds of error: a) some are just ignored ( e.g. "( filesrc blocksize=4 location=/dev/null @ )") b) some get flagged but a partial pipeline is constructed c) some get flagged; no pipeline is constructed IMHO only b) and c) are fine. the checks still go OK for old grammar after this patch, so this can be committed right now.
Comment on attachment 259028 [details] [review] reorganized grammar This causes crashes in the unit test here. Also include the unit test change in the main patch
Running suite(s): Parse Launch syntax [New Thread 0x2aaaacee9700 (LWP 7565)] [New Thread 0x2aaaad0ea700 (LWP 7566)] [New Thread 0x2aaaad2eb700 (LWP 7567)] [New Thread 0x2aaaad4ec700 (LWP 7568)] 0:00:00.021035047 7561 0x631350 ERROR GST_PIPELINE ./grammar.y:905:priv_gst_parse_launch: Unrecoverable syntax error while parsing pipeline ( filesrc blocksize=4 location=/dev/null @ ) Unexpected critical/warning: g_object_is_floating: assertion 'G_IS_OBJECT (object)' failed Unexpected critical/warning: gst_object_unref: assertion 'object != NULL' failed [Thread 0x2aaaad2eb700 (LWP 7567) exited] [Thread 0x2aaaad4ec700 (LWP 7568) exited] Program received signal SIGSEGV, Segmentation fault. priv_gst_parse_yyparse (scanner=0x63b3e0, graph=graph@entry=0x7fffffffb4c0) at ./grammar.y:785 785 $1->back->sink_pads = tmpLink->src_pads; (gdb) bt
+ Trace 232961
Also see my comment 8 about the grammar. Could you include the grammar inside the source code in a comment somewhere?
Created attachment 265970 [details] [review] grammar fix, including small changes to unittest passes "make check" on my machine. valgrind finds the same 3 leaks as in the current git master. grammer is described in code comments (search for "explanation:" it is next to the bison grammar).
commit 156d925cff38d6e99a90c5ecc9cb50aeabcb978a Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Jan 14 13:45:34 2014 +0100 parse: Remove some C99-style comments commit dd086a4e6e5e8d63fa57310129007600de2610d4 Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Jan 14 13:44:11 2014 +0100 parse: Use GSlice for allocating and freeing links and chains commit 748690aba17a1163eee4ffbe24072babe353c156 Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Jan 14 13:42:20 2014 +0100 parse: Add comment about why we disable the "tracing" It did not print anything useful before anyway, everything was commented out. Also remove some unneeded struct members. commit c47f0f2ec528b4fdb9f593dab28b8fd217821be6 Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Jan 14 13:36:24 2014 +0100 parse-launch: Add some more failing pipelines Also convert some comments about valgrind warnings to FIXME comments. These were leaking since some time already. commit 2b33d3318519fd613dd5a4ebbd7c308609904e68 Author: Fabian Kirsch <derFakir@web.de> Date: Fri Jan 10 21:10:17 2014 +0100 parse: Refactor grammar, make it more consistent and fix conflicts https://bugzilla.gnome.org/show_bug.cgi?id=710034
Created attachment 266470 [details] [review] more unittests for the parser
Created attachment 266472 [details] [review] fix segfault; fix bin-properties; dont tolerate grammar ambiguity +Fixing a segfault triggered by some syntax errors +expect 0 grammar-ambiguities (ERROR instead of WARN in case of ambiguities) +fix order of bin-properties assignment (left to right)
Comment on attachment 266472 [details] [review] fix segfault; fix bin-properties; dont tolerate grammar ambiguity commit ffe072f694266f6226821c4760adb4f7a95d02f6 Author: Fabian Kirsch <derFakir@web.de> Date: Thu Jan 16 12:28:47 2014 +0100 parse: Some minor fixes Fix destructor segfaulting Expect 0 grammar-ambiguities Fix order of bin-properties assignment https://bugzilla.gnome.org/show_bug.cgi?id=710034
Review of attachment 266470 [details] [review]: ::: tests/check/pipelines/parse-launch.c @@ +106,3 @@ "fakesrc name=100 fakesink name=101 silent=true 100. ! 101.", "fakesrc ! 1dentity ! fakesink silent=true", + "fakesrc ! tee name=t t.src_0 ! queue ! fakesink t.src_1 ! queue ! fakesink", This is supposed to work after my tee changes, the same as the other way around ... @@ +109,3 @@ + "fakesrc name=foo name=fin fin. ! fakesink", + "( fakesrc ) ! fakesink", + "( fakesrc name=dasrc ) dasrc. ! fakesink", This does not look like it is supposed to work. You're linking an element inside the bin to something outside the bin. It also fails in 1.2 @@ +110,3 @@ + "( fakesrc ) ! fakesink", + "( fakesrc name=dasrc ) dasrc. ! fakesink", +/* "(name=mabin fakesrc) mabin. ! fakesink", FIXME: linking to named bin does not work yet */ This however should work but doesn't... but also doesn't work in 1.2 @@ +367,3 @@ + /* wrong order of linking, asks for non-existing pads from tee */ + /* FIXME: does not fail at the moment. is this a problem? */ + /* "fakesrc ! tee name=t t.src_1 ! queue ! fakesink t.src_0 ! queue ! fakesink", */ ... (cont the tee case) which is this case here. That's supposed to work because tee gives us the pads with the names we were requesting.
"( fakesrc name=dasrc ) dasrc. ! fakesink": works because gst_element_link_pads_full(..) in gst/gstutils.c calls pad_link_maybe_ghosting(..) which creates necessary ghostpads on the fly. I did not change that. "(name=mabin fakesrc) mabin. ! fakesink": Whats missing is a syntax to explicitly create ghostpads. AFAIK that was never in the syntax. The problem arises when gst_element_get_compatible_pad(..) gets called on the bin. GstBin has no pointer like frontElement or backElement that could be examined recursivley. Peeking at the first and last element in GstBin.elements would not be ok.
I think that's all correct then, sorry. Can you update the patch to consider the tee changes?
Created attachment 266746 [details] [review] added some more testcases added some more testcases. in addition refactored "1dentity" to "1__dentity". So it does not look like typo. About linkorder: I did not construct a pipeline that depends on left-to-right order of linking. IMHO that would be a strange pipeline behaviour anyway. A robust(tm) pipeline string should not depend on order of linkage. Padtemplates or Capsfilters may be used. BTW linkorder already was discussed in: https://bugzilla.gnome.org/show_bug.cgi?id=563728 Discussion of linkoder maybe should go there.
Yes, should be handled there. And the order should not matter indeed. commit 702e5d11c3308d6e4f143aa37dfc76a5dabece1a Author: Fabian Kirsch <derFakir@web.de> Date: Mon Jan 20 15:26:54 2014 +0100 parse: Additional tests for parser https://bugzilla.gnome.org/show_bug.cgi?id=710034
This breaks the build for osx (commit 2b33d3318519fd613dd5a4ebbd7c308609904e68) OSX (even latest) ships with bison 2.3 Bison 2.3 doesn't support the '<>' notation. Here's a thread of another project that ended up with the same issue: http://lists.osgeo.org/pipermail/qgis-developer/2011-August/015757.html
bison 2.3 is from 2006. Do we really care, we could just ship newer bison with cerbero. Or could try to find a workaround that works with ancient bison too. Or we could go back to ship the generated files with the tarballs like with did in 0.10 because of exactly the same problem.
Maybe also add detection of bison > 2.3 somewhere then. I'm fine with using bison from cerbero (which we already do for windows afaik).
Created attachment 267484 [details] [review] gst-parser: Bump bison requirement to 2.4 Needed for '<>' syntax