153462 2016-01-25 16:13:15 -0800 CSSGrammar.y:1742.31-34: warning: unused value: $3 2016-01-27 19:20:48 -0800 1 1 1 Unclassified WebKit CSS Other PC Linux RESOLVED FIXED P2 Minor --- 1 mcatanzaro mcatanzaro achristensen commit-queue hyatt mcatanzaro ossy oldest_to_newest 1158584 0 mcatanzaro 2016-01-25 16:13:15 -0800 I've been ignoring this bison warning for a while now: [620/5167] Generating ../../DerivedSou.../DerivedSources/WebCore/CSSGrammar.cpp /home/mcatanzaro/src/WebKit/WebKitBuild/GNOME/DerivedSources/WebCore/CSSGrammar.y:1742.31-34: warning: unused value: $3 [-Wother] | VARFUNCTION maybe_space expr closing_parenthesis { ^^^^ It means we are leaking the CSSParserValueList associated with that expr. 1158651 1 269835 mcatanzaro 2016-01-25 18:36:37 -0800 Created attachment 269835 Patch 1158654 2 achristensen 2016-01-25 18:38:31 -0800 When did this start? 1158661 3 mcatanzaro 2016-01-25 18:55:02 -0800 (In reply to comment #2) > When did this start? Several months ago. It's not the only build warning we have, and the last time I looked I wasn't sure what to do with it. I think the warning is only available in newer versions of bison than you'd have easy access to. 1158664 4 mcatanzaro 2016-01-25 18:59:21 -0800 It was introduced in r191128, so I'll CC David for review. P.S. My rationale for CCing you, Alex, was that I once heard you say "bison." 1158701 5 achristensen 2016-01-25 21:43:38 -0800 http://trac.webkit.org/changeset/191128/trunk/Source/WebCore/css/CSSGrammar.y.in definitely added some new's without a corresponding delete. I'd prefer to have someone double check this, though. Hyatt? 1158723 6 ossy 2016-01-25 23:13:35 -0800 *** Bug 150325 has been marked as a duplicate of this bug. *** 1158931 7 achristensen 2016-01-26 12:35:46 -0800 This code is definitely run by fast/css/variables/test-suite/036.html and fast/css/variables/test-suite/068.html 1158937 8 269835 achristensen 2016-01-26 12:42:13 -0800 Comment on attachment 269835 Patch Running those tests with guardmalloc passes, so I'm going to say r=me 1158942 9 achristensen 2016-01-26 12:50:49 -0800 (In reply to comment #5) > http://trac.webkit.org/changeset/191128/trunk/Source/WebCore/css/CSSGrammar. > y.in definitely added some new's without a corresponding delete. And this delete does not correspond to the new's added in that patch. 1158967 10 269835 commit-queue 2016-01-26 13:12:40 -0800 Comment on attachment 269835 Patch Clearing flags on attachment: 269835 Committed r195612: <http://trac.webkit.org/changeset/195612> 1158968 11 commit-queue 2016-01-26 13:12:44 -0800 All reviewed patches have been landed. Closing bug. 1159226 12 achristensen 2016-01-26 23:39:57 -0800 (In reply to comment #9) > (In reply to comment #5) > > http://trac.webkit.org/changeset/191128/trunk/Source/WebCore/css/CSSGrammar. > > y.in definitely added some new's without a corresponding delete. > And this delete does not correspond to the new's added in that patch. This delete corresponds with the following new in line 1668 in the definition of valid_expr: $$ = new CSSParserValueList; Correct me if I'm wrong. 1159313 13 269835 darin 2016-01-27 10:19:44 -0800 Comment on attachment 269835 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269835&action=review > Source/WebCore/ChangeLog:12 > + "Right-hand side symbols of a rule that explicitly triggers a syntax error via YYERROR are > + not discarded automatically. As a rule of thumb, destructors are invoked only when user > + actions cannot manage the memory." Sounds like we might have leaks in other rules that invoke YYERROR; did you check those over? 1159328 14 achristensen 2016-01-27 10:43:59 -0800 (In reply to comment #13) > Sounds like we might have leaks in other rules that invoke YYERROR; did you > check those over? I looked them over, and I'm pretty sure there isn't one we're missing a delete before. Most of them would have a similar warning, and the ones that wouldn't (because $1, $2, etc. are used somewhere else) have entries like IDENT (which I think is a string) or a nested_selector_list that is verified to be nullptr (and it's a unique_ptr anyways). Maybe we should use unique_ptrs everywhere in the parser instead of new to avoid future problems like this. 1159376 15 mcatanzaro 2016-01-27 12:43:42 -0800 (In reply to comment #13) > Sounds like we might have leaks in other rules that invoke YYERROR; did you > check those over? I think the other YYERROR cases are fine; there are not that many. (In reply to comment #12) > This delete corresponds with the following new in line 1668 in the > definition of valid_expr: > $$ = new CSSParserValueList; > > Correct me if I'm wrong. I am not sure whether it's guaranteed to come from there; grammars are hard and I don't really want to try to understand this one. What's important is that expr has a semantic value of type CSSParserValueList*, so each rule that uses expr needs to either delete it or percolate it up by assigning it to $$, else it will be leaked. (In reply to comment #14) > I looked them over, and I'm pretty sure there isn't one we're missing a > delete before. Most of them would have a similar warning, and the ones that > wouldn't (because $1, $2, etc. are used somewhere else) have entries like > IDENT (which I think is a string) or a nested_selector_list that is verified > to be nullptr (and it's a unique_ptr anyways). Maybe we should use > unique_ptrs everywhere in the parser instead of new to avoid future problems > like this. This would make the bison parser more robust, to be sure. (I am worried that the current grammar might be leaking values in cases where they are actually used, which would suppress the warning. But maybe it is fine; I don't plan to investigate further.) There might be an easy way to use std::unique_ptr. I have heard that it might be possible in C++ 11 to use class instances in unions (and have them be properly destructed), but I am having trouble finding reliable info with a quick Internet search. If that's true, and it works in GCC/Clang/MSVCC, then we could use std::unique_ptr in the %union declaration at the top of the file, which would allow us to get some safety without switching to a different skeleton. I am not sure if it is possible to safely use class types in unions, though; if not, we are stuck with error-prone new and delete. There is an alternative, which is to switch to the experimental C++ skeleton. The new skeleton allows storing C++ objects as semantic values, rather than relying on a union. Unfortunately, it is only available in modern versions of bison. I believe you would have internal political difficulties to do this, due to the GPLv3+ license, although the same non-infectious license exception applies to the generated code as always (so we can safely use it in WebKit without infecting our license). I hesitate to recommend this, though, as the C++ skeleton is still experimental; some serious bugs in it that I discovered were fixed not so long ago, and my grammar was for a school project, much simpler than WebKit's. 1159379 16 achristensen 2016-01-27 12:47:45 -0800 I think the right solution is to not use bison. 1159575 17 mcatanzaro 2016-01-27 19:20:48 -0800 I'm a fan of bison. *Shrug* I looked up unions in "The C++ Programming Language" and the answer is: yes you can use class types in unions now, but it's not going to work for std::unique_ptr because it requires no custom contructor, destructor, or copy/move assignment operators. 269835 2016-01-25 18:36:37 -0800 2016-01-26 13:12:40 -0800 Patch bug-153462-20160125203610.patch text/plain 1506 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMTk1NTU5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOTA3YmU0ZDkyMmFkM2Jj ZTc4NzVmNTkzODhiMzkwM2QyN2JhYjY2Ni4uNTkzOTg5YWUxMTBjNDZjNjBmM2EyZmI4Y2UxZTkx YWRiOWRkZDYxYyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDE2LTAxLTI1ICBNaWNo YWVsIENhdGFuemFybyAgPG1jYXRhbnphcm9AaWdhbGlhLmNvbT4KKworICAgICAgICBDU1NHcmFt bWFyLnk6MTc0Mi4zMS0zNDogd2FybmluZzogdW51c2VkIHZhbHVlOiAkMworICAgICAgICBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTUzNDYyCisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhpcyB3YXJuaW5nIGluZGljYXRl cyB0aGF0IHdlIGhhdmUgYSBtZW1vcnkgbGVhay4gRnJvbSB0aGUgYmlzb24gbWFudWFsOgorCisg ICAgICAgICJSaWdodC1oYW5kIHNpZGUgc3ltYm9scyBvZiBhIHJ1bGUgdGhhdCBleHBsaWNpdGx5 IHRyaWdnZXJzIGEgc3ludGF4IGVycm9yIHZpYSBZWUVSUk9SIGFyZQorICAgICAgICBub3QgZGlz Y2FyZGVkIGF1dG9tYXRpY2FsbHkuIEFzIGEgcnVsZSBvZiB0aHVtYiwgZGVzdHJ1Y3RvcnMgYXJl IGludm9rZWQgb25seSB3aGVuIHVzZXIKKyAgICAgICAgYWN0aW9ucyBjYW5ub3QgbWFuYWdlIHRo ZSBtZW1vcnkuIgorCisgICAgICAgIEFyZ3VhYmx5IGEgZGVzaWduIGVycm9yLCBidXQgdGhhdCdz IGhvdyBpdCBpcy4KKworICAgICAgICAqIGNzcy9DU1NHcmFtbWFyLnkuaW46CisKIDIwMTYtMDEt MjUgIFNhbSBXZWluaWcgIDxzYW1Ad2Via2l0Lm9yZz4KIAogICAgICAgICBGaXggdGhlIEFTQU4g YnVpbGQuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9jc3MvQ1NTR3JhbW1hci55LmluIGIv U291cmNlL1dlYkNvcmUvY3NzL0NTU0dyYW1tYXIueS5pbgppbmRleCBmYjUzZjc1MDhhZWU1MTg1 YjExYmFiMjMwYWE3YWU2MzliZWFhZTYwLi44YmJiNDIwMGI3NzFkYTJkMGZhMjc3NWIzYzQ5OTNl ZGQ2M2YyY2VjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9jc3MvQ1NTR3JhbW1hci55Lmlu CisrKyBiL1NvdXJjZS9XZWJDb3JlL2Nzcy9DU1NHcmFtbWFyLnkuaW4KQEAgLTE4MjQsNiArMTgy NCw3IEBAIHZhcmlhYmxlX2Z1bmN0aW9uOgogICAgIHwgVkFSRlVOQ1RJT04gbWF5YmVfc3BhY2Ug ZXhwciBjbG9zaW5nX3BhcmVudGhlc2lzIHsKICAgICAgICAgJCQuaWQgPSBDU1NWYWx1ZUludmFs aWQ7CiAgICAgICAgICQkLnVuaXQgPSAwOworICAgICAgICBkZWxldGUgJDM7CiAgICAgICAgIFlZ RVJST1I7CiAgICAgfQogICAgIHwgVkFSRlVOQ1RJT04gbWF5YmVfc3BhY2UgZXhwcl9yZWNvdmVy eSBjbG9zaW5nX3BhcmVudGhlc2lzIHsK