Bug 154535 – Workaround for ICE in GCC 4.8 appeared in r196846.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Konstantin Tokarev]

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
 ^

[Konstantin Tokarev]

Created attachment 271923 [details]
Patch

[Michael Catanzaro]

Comment on attachment 271923 [details]
Patch

Add a FIXME please!

[Konstantin Tokarev]

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.

[Chris Dumez]

Comment on attachment 271923 [details]
Patch

No this is not right. It was meant to be off (without the quotes), referring to the local off variable.

[Michael Catanzaro]

(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?

[Chris Dumez]

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);

[Chris Dumez]

(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.

[Konstantin Tokarev]

(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]);

[Chris Dumez]

(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.

[Konstantin Tokarev]

(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.

[Chris Dumez]

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.

[WebKit Commit Bot]

Comment on attachment 271923 [details]
Patch

Clearing flags on attachment: 271923

Committed r196946: <http://trac.webkit.org/changeset/196946>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

(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!