154535 2016-02-22 08:06:48 -0800 Workaround for ICE in GCC 4.8 appeared in r196846. 2016-02-22 12:04:21 -0800 1 1 1 Unclassified WebKit WebCore Misc. WebKit Local Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 annulen webkit-unassigned cdumez commit-queue mcatanzaro ossy oldest_to_newest 1167017 0 annulen 2016-02-22 08:06:48 -0800 This is an exact error message: ../../../Source/WebCore/html/HTMLFormElement.cpp:876:1: internal compiler error: in output_index_string, at dwarf2out.c:21825 } // namespace ^ 1167022 1 271923 annulen 2016-02-22 08:36:39 -0800 Created attachment 271923 Patch 1167039 2 271923 mcatanzaro 2016-02-22 09:07:16 -0800 Comment on attachment 271923 Patch Add a FIXME please! 1167041 3 annulen 2016-02-22 09:13:56 -0800 What FIXME do you mean? equalLettersIgnoringASCIICase(..., "off") where "off" is literal is used in many other places of this file, and I guess this form is a bit faster because literal needs not need is8Bit() check. 1167043 4 271923 cdumez 2016-02-22 09:33:44 -0800 Comment on attachment 271923 Patch No this is not right. It was meant to be off (without the quotes), referring to the local off variable. 1167048 5 mcatanzaro 2016-02-22 09:37:02 -0800 (In reply to comment #4) > No this is not right. It was meant to be off (without the quotes), referring > to the local off variable. Surely equalLettersIgnoringASCIICase does not have different behavior depending on whether I pass a wtf::String or a literal? 1167049 6 271923 cdumez 2016-02-22 09:37:04 -0800 Comment on attachment 271923 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271923&action=review > Source/WebCore/ChangeLog:7 > + The change log does not explain why this should be "off" instead of off. It seems valid to use off without quotes to me. It should end up calling: inline bool equalIgnoringASCIICase(const AtomicString& a, const String& b); 1167051 7 cdumez 2016-02-22 09:40:01 -0800 (In reply to comment #6) > Comment on attachment 271923 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271923&action=review > > > Source/WebCore/ChangeLog:7 > > + > > The change log does not explain why this should be "off" instead of off. It > seems valid to use off without quotes to me. It should end up calling: > inline bool equalIgnoringASCIICase(const AtomicString& a, const String& b); I don't think we do have a equalIgnoringASCIICase() overload that takes a literal. Therefore, if you pass "off" it likely calls: inline bool equalIgnoringASCIICase(const String& a, const char* b); Which means we don't know the length of b in advance. If we pass a String in, the size of b is known in advance. 1167056 8 annulen 2016-02-22 09:44:59 -0800 (In reply to comment #7) > I don't think we do have a equalIgnoringASCIICase() overload that takes a > literal. Therefore, if you pass "off" it likely calls: > inline bool equalIgnoringASCIICase(const String& a, const char* b); > > Which means we don't know the length of b in advance. If we pass a String > in, the size of b is known in advance. I believe it calls template<unsigned length> bool equalLettersIgnoringASCIICase(const StringImpl&, const char (&lowercaseLetters)[length]); 1167065 9 cdumez 2016-02-22 10:09:22 -0800 (In reply to comment #8) > (In reply to comment #7) > > > I don't think we do have a equalIgnoringASCIICase() overload that takes a > > literal. Therefore, if you pass "off" it likely calls: > > inline bool equalIgnoringASCIICase(const String& a, const char* b); > > > > Which means we don't know the length of b in advance. If we pass a String > > in, the size of b is known in advance. > > I believe it calls > > template<unsigned length> bool equalLettersIgnoringASCIICase(const > StringImpl&, const char (&lowercaseLetters)[length]); oh, I did indeed miss the one taking a literal: template<unsigned length> inline bool equalLettersIgnoringASCIICase(const AtomicString& string, const char (&lowercaseLetters)[length]); That said, it does not explain why the previous code does not work in GCC. It seems perfectly legal to me. 1167074 10 annulen 2016-02-22 10:28:29 -0800 (In reply to comment #9) > That said, it does not explain why the previous code does not work in GCC. > It seems perfectly legal to me. That's hard to explain internal compiler errors. If you are interested I'll prepare reduced test case which should reveal broken C++ construction. 1167079 11 271923 cdumez 2016-02-22 10:38:01 -0800 Comment on attachment 271923 Patch Ok to unbreak you and because equalIgnoringASCIICase() has a literal taking an override. However, I wish we understood the problem a bit better. 1167102 12 271923 commit-queue 2016-02-22 11:26:45 -0800 Comment on attachment 271923 Patch Clearing flags on attachment: 271923 Committed r196946: <http://trac.webkit.org/changeset/196946> 1167103 13 commit-queue 2016-02-22 11:26:50 -0800 All reviewed patches have been landed. Closing bug. 1167126 14 mcatanzaro 2016-02-22 12:04:21 -0800 (In reply to comment #11) > Comment on attachment 271923 [details] > Patch > > Ok to unbreak you and because equalIgnoringASCIICase() has a literal taking > an override. However, I wish we understood the problem a bit better. To be clear, it's a GCC bug causing some obsolete version of GCC to crash (ICE = internal compiler error), and this modification is a workaround for that crash. It'd be reasonable to accept the patch (it's a one-liner that adds support for a new compiler) or reject it (it's a workaround for a compiler we technically don't support anymore), I'd err on the side of accept... which you did. (In reply to comment #8) > I believe it calls > > template<unsigned length> bool equalLettersIgnoringASCIICase(const > StringImpl&, const char (&lowercaseLetters)[length]); Wow! This is quite esoteric C++, the only time I ever saw this syntax was in a trivia question... array parameters are normally just pointers, but for templates it's not necessarily true! 271923 2016-02-22 08:36:39 -0800 2016-02-22 11:26:45 -0800 Patch bug-154535-20160222193620.patch text/plain 1448 annulen U3VidmVyc2lvbiBSZXZpc2lvbjogMTk2OTM0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOGRlMTg4ODgzZWE0MTRj ZjA3MmZjZDA4ODk0MTU1MTRkZTcxMTJjMS4uMDlkNzlmYjAxYmUyNGMwMWFiOTM1NDUzZDExYzhh MmM1ZjQ0YjE4OSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDE2LTAyLTIyICBLb25z dGFudGluIFRva2FyZXYgIDxhbm51bGVuQHlhbmRleC5ydT4KKworICAgICAgICBXb3JrYXJvdW5k IGZvciBJQ0UgaW4gR0NDIDQuOCBhcHBlYXJlZCBpbiByMTk2ODQ2LgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTU0NTM1CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzIG5lZWRlZC4KKwor ICAgICAgICAqIGh0bWwvSFRNTEZvcm1FbGVtZW50LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkhU TUxGb3JtRWxlbWVudDo6YXV0b2NvbXBsZXRlKToKKwogMjAxNi0wMi0yMiAgTWFudWVsIFJlZ28g Q2FzYXNub3ZhcyAgPHJlZ29AaWdhbGlhLmNvbT4KIAogICAgICAgICBbY3NzLWdyaWRdIFN3YXAg Y29sdW1ucyBhbmQgcm93cyBpbiBncmlkLXRlbXBsYXRlIHNob3J0aGFuZApkaWZmIC0tZ2l0IGEv U291cmNlL1dlYkNvcmUvaHRtbC9IVE1MRm9ybUVsZW1lbnQuY3BwIGIvU291cmNlL1dlYkNvcmUv aHRtbC9IVE1MRm9ybUVsZW1lbnQuY3BwCmluZGV4IGIxYjQzOGZhMzVhOGQxODkwZjcwOTY1ODYy YjA5ODhiMzM5MmMzZGUuLjZhZmEwNDMzM2M2NmRkYzY1NmUxMWE4MzdiMGE3NzQ5MjZlNTMwNWEg MTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2h0bWwvSFRNTEZvcm1FbGVtZW50LmNwcAorKysg Yi9Tb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxGb3JtRWxlbWVudC5jcHAKQEAgLTg3MCw3ICs4NzAs NyBAQCBjb25zdCBTdHJpbmcmIEhUTUxGb3JtRWxlbWVudDo6YXV0b2NvbXBsZXRlKCkgY29uc3QK ICAgICBzdGF0aWMgTmV2ZXJEZXN0cm95ZWQ8Y29uc3QgU3RyaW5nPiBvbigib24iLCBTdHJpbmc6 OkNvbnN0cnVjdEZyb21MaXRlcmFsKTsKICAgICBzdGF0aWMgTmV2ZXJEZXN0cm95ZWQ8Y29uc3Qg U3RyaW5nPiBvZmYoIm9mZiIsIFN0cmluZzo6Q29uc3RydWN0RnJvbUxpdGVyYWwpOwogCi0gICAg cmV0dXJuIGVxdWFsSWdub3JpbmdBU0NJSUNhc2UoZmFzdEdldEF0dHJpYnV0ZShhdXRvY29tcGxl dGVBdHRyKSwgb2ZmKSA/IG9mZiA6IG9uOworICAgIHJldHVybiBlcXVhbElnbm9yaW5nQVNDSUlD YXNlKGZhc3RHZXRBdHRyaWJ1dGUoYXV0b2NvbXBsZXRlQXR0ciksICJvZmYiKSA/IG9mZiA6IG9u OwogfQogCiB9IC8vIG5hbWVzcGFjZQo=