Le 7 mars 2011 à 01:45, Joel E. Denny a écrit : > Hi Akim, Tys, > > On Wed, 2 Mar 2011, Akim Demaille wrote: > >> Thanks for the bug report. I have pushed in candidates/named-ref-free a >> fix for branch-2.5, attached below. Unless someone raises the hand, >> I'll push it in 2.5 and master in the near future. > > Thanks to both of you. > > The final value of current_lhs_named_ref doesn't get freed, so the new > test group doesn't pass maintainer-check-valgrind. With a fix for that, > it seems fine to push.
Yes, I thought about it once the mail sent, and the candidate I had pushed had it right. I can't use valgrind currently on my Mac, there's a bazillions of false alarms due to the OS allocations :( For the records, these are the patches as installed on 2.5 (including the change of address Tys asked me).
From 686e83e396696572171fad32f440cc279ab82a75 Mon Sep 17 00:00:00 2001 From: Akim Demaille <[email protected]> Date: Wed, 2 Mar 2011 17:03:37 +0100 Subject: [PATCH 1/2] tests: style changes. * tests/named-refs.at (Redundant words in LHS brackets) (Unresolved references): here. --- ChangeLog | 6 ++ tests/named-refs.at | 130 ++++++++++++++++++++------------------------------- 2 files changed, 57 insertions(+), 79 deletions(-) diff --git a/ChangeLog b/ChangeLog index f5eee1e..4bb16a8 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2011-03-09 Akim Demaille <[email protected]> + + tests: style changes. + * tests/named-refs.at (Redundant words in LHS brackets) + (Unresolved references): here. + 2011-03-06 Joel E. Denny <[email protected]> java: fix parser tracing bug. diff --git a/tests/named-refs.at b/tests/named-refs.at index 2147314..98f45a5 100644 --- a/tests/named-refs.at +++ b/tests/named-refs.at @@ -463,7 +463,7 @@ AT_SETUP([Redundant words in LHS brackets]) AT_DATA_GRAMMAR([test.y], [[ %% -start[a s]: foo +start[a s]: foo; ]]) AT_BISON_CHECK([-o test.c test.y], 1, [], [[test.y:11.9: unexpected identifier in bracketed name: `s' @@ -477,89 +477,61 @@ AT_DATA_GRAMMAR([test.y], [[ %% stat: -sym_a sym_b -{ func($sym.field); } -| -sym_a sym_b -{ func($<aa>sym.field); } -| -sym_a sym_b -{ func($[sym.field]); } -| -sym_a sym_b -{ func($<aa>[sym.field]); } -| -sym_a sym_b -{ func($sym); } -| -sym_a sym_b -{ func($<aa>sym); } -| -sym_a sym_b -{ func($[sym]); } -sym_a sym_b -{ func($<aa>[sym]); } + sym_a sym_b { func($sym.field); } +| sym_a sym_b { func($<aa>sym.field); } +| sym_a sym_b { func($[sym.field]); } +| sym_a sym_b { func($<aa>[sym.field]); } +| sym_a sym_b { func($sym); } +| sym_a sym_b { func($<aa>sym); } +| sym_a sym_b { func($[sym]); } sym_a sym_b { func($<aa>[sym]); } ; + stat1: -sym_a sym_b -{ func($sym-field); } -| -sym_a sym_b -{ func($<aa>sym-field); } -| -sym_a sym_b -{ func($[sym-field]); } -| -sym_a sym_b -{ func($<aa>[sym-field]); } -| -sym_a sym_b -{ func($sym); } -| -sym_a sym_b -{ func($<aa>sym); } -| -sym_a sym_b -{ func($[sym]); } -sym_a sym_b -{ func($<aa>[sym]); } + sym_a sym_b { func($sym-field); } +| sym_a sym_b { func($<aa>sym-field); } +| sym_a sym_b { func($[sym-field]); } +| sym_a sym_b { func($<aa>[sym-field]); } +| sym_a sym_b { func($sym); } +| sym_a sym_b { func($<aa>sym); } +| sym_a sym_b { func($[sym]); } sym_a sym_b { func($<aa>[sym]); } ; -sym_a : 'a'; -sym_b : 'b'; + +sym_a: 'a'; +sym_b: 'b'; ]]) AT_BISON_CHECK([-o test.c test.y], 1, [], -[[test.y:13.8-17: invalid reference: `$sym.field' -test.y:12.1-13.21: symbol not found in production: sym -test.y:16.8-21: invalid reference: `$<aa>sym.field' -test.y:15.1-16.25: symbol not found in production: sym -test.y:19.8-19: invalid reference: `$[sym.field]' -test.y:18.1-19.23: symbol not found in production: sym.field -test.y:22.8-23: invalid reference: `$<aa>[sym.field]' -test.y:21.1-22.27: symbol not found in production: sym.field -test.y:25.8-11: invalid reference: `$sym' -test.y:24.1-25.15: symbol not found in production: sym -test.y:28.8-15: invalid reference: `$<aa>sym' -test.y:27.1-28.19: symbol not found in production: sym -test.y:31.8-13: invalid reference: `$[sym]' -test.y:30.1-33.21: symbol not found in production before $3: sym -test.y:33.8-17: invalid reference: `$<aa>[sym]' -test.y:30.1-33.21: symbol not found in production: sym -test.y:37.8-17: invalid reference: `$sym-field' -test.y:36.1-37.21: symbol not found in production: sym -test.y:40.8-21: invalid reference: `$<aa>sym-field' -test.y:39.1-40.25: symbol not found in production: sym -test.y:43.8-19: invalid reference: `$[sym-field]' -test.y:42.1-43.23: symbol not found in production: sym-field -test.y:46.8-23: invalid reference: `$<aa>[sym-field]' -test.y:45.1-46.27: symbol not found in production: sym-field -test.y:49.8-11: invalid reference: `$sym' -test.y:48.1-49.15: symbol not found in production: sym -test.y:52.8-15: invalid reference: `$<aa>sym' -test.y:51.1-52.19: symbol not found in production: sym -test.y:55.8-13: invalid reference: `$[sym]' -test.y:54.1-57.21: symbol not found in production before $3: sym -test.y:57.8-17: invalid reference: `$<aa>[sym]' -test.y:54.1-57.21: symbol not found in production: sym +[[test.y:12.22-31: invalid reference: `$sym.field' +test.y:12.3-35: symbol not found in production: sym +test.y:13.22-35: invalid reference: `$<aa>sym.field' +test.y:13.3-39: symbol not found in production: sym +test.y:14.22-33: invalid reference: `$[sym.field]' +test.y:14.3-37: symbol not found in production: sym.field +test.y:15.22-37: invalid reference: `$<aa>[sym.field]' +test.y:15.3-41: symbol not found in production: sym.field +test.y:16.22-25: invalid reference: `$sym' +test.y:16.3-29: symbol not found in production: sym +test.y:17.22-29: invalid reference: `$<aa>sym' +test.y:17.3-33: symbol not found in production: sym +test.y:18.22-27: invalid reference: `$[sym]' +test.y:18.3-65: symbol not found in production before $3: sym +test.y:18.52-61: invalid reference: `$<aa>[sym]' +test.y:18.3-65: symbol not found in production: sym +test.y:22.22-31: invalid reference: `$sym-field' +test.y:22.3-35: symbol not found in production: sym +test.y:23.22-35: invalid reference: `$<aa>sym-field' +test.y:23.3-39: symbol not found in production: sym +test.y:24.22-33: invalid reference: `$[sym-field]' +test.y:24.3-37: symbol not found in production: sym-field +test.y:25.22-37: invalid reference: `$<aa>[sym-field]' +test.y:25.3-41: symbol not found in production: sym-field +test.y:26.22-25: invalid reference: `$sym' +test.y:26.3-29: symbol not found in production: sym +test.y:27.22-29: invalid reference: `$<aa>sym' +test.y:27.3-33: symbol not found in production: sym +test.y:28.22-27: invalid reference: `$[sym]' +test.y:28.3-65: symbol not found in production before $3: sym +test.y:28.52-61: invalid reference: `$<aa>[sym]' +test.y:28.3-65: symbol not found in production: sym ]]) AT_CLEANUP -- 1.7.4.1
From 8f462efe923947cc4e72deea5b0fa93a5f88000d Mon Sep 17 00:00:00 2001 From: Akim Demaille <[email protected]> Date: Wed, 2 Mar 2011 17:06:58 +0100 Subject: [PATCH 2/2] named references: fix double free. In `rhs[name]: "a" | "b"', do not free "name" twice. Reported by Tys Lefering. <http://lists.gnu.org/archive/html/bug-bison/2010-06/msg00002.html> * src/named-ref.h, src/named-ref.c (named_ref_copy): New. * src/parse-gram.y (current_lhs): Rename as... (current_lhs_symbol): this. (current_lhs): New function. Use it to free the current lhs named reference. * src/reader.c: Bind lhs to a copy of the current named reference. * src/symlist.c: Rely on free (0) being valid. * tests/named-refs.at: Test this. --- ChangeLog | 15 +++++++++++++++ THANKS | 2 +- src/named-ref.c | 6 ++++++ src/named-ref.h | 3 +++ src/parse-gram.y | 30 ++++++++++++++++++++++++++---- src/reader.c | 2 +- src/symlist.c | 3 +-- tests/named-refs.at | 13 +++++++++++++ 8 files changed, 66 insertions(+), 8 deletions(-) diff --git a/ChangeLog b/ChangeLog index 4bb16a8..448e54a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,20 @@ 2011-03-09 Akim Demaille <[email protected]> + named references: fix double free. + In `rhs[name]: "a" | "b"', do not free "name" twice. + Reported by Tys Lefering. + <http://lists.gnu.org/archive/html/bug-bison/2010-06/msg00002.html> + * src/named-ref.h, src/named-ref.c (named_ref_copy): New. + * src/parse-gram.y (current_lhs): Rename as... + (current_lhs_symbol): this. + (current_lhs): New function. Use it to free the current lhs + named reference. + * src/reader.c: Bind lhs to a copy of the current named reference. + * src/symlist.c: Rely on free (0) being valid. + * tests/named-refs.at: Test this. + +2011-03-09 Akim Demaille <[email protected]> + tests: style changes. * tests/named-refs.at (Redundant words in LHS brackets) (Unresolved references): here. diff --git a/THANKS b/THANKS index ac6047a..ad6f379 100644 --- a/THANKS +++ b/THANKS @@ -102,7 +102,7 @@ Tom Lane [email protected] Tom Tromey [email protected] Tommy Nordgren [email protected] Troy A. Johnson [email protected] -Tys Lefering [email protected] +Tys Lefering [email protected] Vin Shelton [email protected] Wayne Green [email protected] Wolfram Wagner [email protected] diff --git a/src/named-ref.c b/src/named-ref.c index 4bf3da1..132c741 100644 --- a/src/named-ref.c +++ b/src/named-ref.c @@ -33,6 +33,12 @@ named_ref_new (uniqstr id, location loc) return res; } +named_ref * +named_ref_copy (const named_ref *r) +{ + return named_ref_new (r->id, r->loc); +} + void named_ref_free (named_ref *r) { diff --git a/src/named-ref.h b/src/named-ref.h index 0f96e46..46d9d8d 100644 --- a/src/named-ref.h +++ b/src/named-ref.h @@ -37,6 +37,9 @@ typedef struct named_ref /* Allocate a named reference object. */ named_ref *named_ref_new (uniqstr id, location loc); +/* Allocate and return a copy. */ +named_ref *named_ref_copy (const named_ref *r); + /* Free a named reference object. */ void named_ref_free (named_ref *r); diff --git a/src/parse-gram.y b/src/parse-gram.y index 8871f67..8c2e18d 100644 --- a/src/parse-gram.y +++ b/src/parse-gram.y @@ -61,11 +61,30 @@ static void add_param (char const *type, char *decl, location loc); static symbol_class current_class = unknown_sym; static uniqstr current_type = NULL; -static symbol *current_lhs; +static symbol *current_lhs_symbol; static location current_lhs_location; static named_ref *current_lhs_named_ref; static int current_prec = 0; +/** Set the new current left-hand side symbol, possibly common + * to several right-hand side parts of rule. + */ +static +void +current_lhs(symbol *sym, location loc, named_ref *ref) +{ + current_lhs_symbol = sym; + current_lhs_location = loc; + /* In order to simplify memory management, named references for lhs + are always assigned by deep copy into the current symbol_list + node. This is because a single named-ref in the grammar may + result in several uses when the user factors lhs between several + rules using "|". Therefore free the parser's original copy. */ + free (current_lhs_named_ref); + current_lhs_named_ref = ref; +} + + #define YYTYPE_INT16 int_fast16_t #define YYTYPE_INT8 int_fast8_t #define YYTYPE_UINT16 uint_fast16_t @@ -526,8 +545,11 @@ rules_or_grammar_declaration: ; rules: - id_colon named_ref.opt { current_lhs = $1; current_lhs_location = @1; - current_lhs_named_ref = $2; } rhses.1 + id_colon named_ref.opt { current_lhs ($1, @1, $2); } rhses.1 + { + /* Free the current lhs. */ + current_lhs (0, @1, 0); + } ; rhses.1: @@ -538,7 +560,7 @@ rhses.1: rhs: /* Nothing. */ - { grammar_current_rule_begin (current_lhs, current_lhs_location, + { grammar_current_rule_begin (current_lhs_symbol, current_lhs_location, current_lhs_named_ref); } | rhs symbol named_ref.opt { grammar_current_rule_symbol_append ($2, @2, $3); } diff --git a/src/reader.c b/src/reader.c index 9153f21..86cc82c 100644 --- a/src/reader.c +++ b/src/reader.c @@ -231,7 +231,7 @@ grammar_current_rule_begin (symbol *lhs, location loc, p = grammar_symbol_append (lhs, loc); if (lhs_name) - assign_named_ref(p, lhs_name); + assign_named_ref (p, named_ref_copy (lhs_name)); current_rule = grammar_end; diff --git a/src/symlist.c b/src/symlist.c index e717c3e..190d007 100644 --- a/src/symlist.c +++ b/src/symlist.c @@ -151,8 +151,7 @@ symbol_list_free (symbol_list *list) for (node = list; node; node = next) { next = node->next; - if (node->named_ref) - named_ref_free (node->named_ref); + named_ref_free (node->named_ref); free (node); } } diff --git a/tests/named-refs.at b/tests/named-refs.at index 98f45a5..ff414a9 100644 --- a/tests/named-refs.at +++ b/tests/named-refs.at @@ -472,6 +472,19 @@ AT_CLEANUP ####################################################################### +# Bison used to free twice the named ref for "a", since a single copy +# was used in two rules. +AT_SETUP([Factored LHS]) +AT_DATA_GRAMMAR([test.y], +[[ +%% +start[a]: "foo" | "bar"; +]]) +AT_BISON_CHECK([-o test.c test.y]) +AT_CLEANUP + +####################################################################### + AT_SETUP([Unresolved references]) AT_DATA_GRAMMAR([test.y], [[ -- 1.7.4.1
