364049 - patch for bug 347705 caused bustage for optimized msvc7.1 builds

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

For some reason the msvc7.1 optimizer doesn't like the patch for bug 347705. In my build, CSSParserImpl::TranslateDimension always returns false at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/style/nsCSSParser.cpp&rev=3.333&mark=3622#3609  when it falls into that branch, because the UnitData struct's name fields are seemingly all empty strings. This results in a completely horked browser UI (see screenshot). This bug doesn't appear in mozilla.org builds (which use msvc8) and also doesn't appear if I disable optimization. Mook on IRC was also seeing this problem.

Removing the "const" from http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/style/nsCSSParser.cpp&rev=3.333&mark=3570#3569 fixes it, though I have no idea why. I couldn't find any related reports of compiler bugs, but I didn't look very hard.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: screenshot

Comment 2

[:Gavin Sharp [email: [e-mail]]]

It looks like installing Visual Studio .NET 2003 SP1 fixes this bug. I guess that might make this WONTFIX.

Comment 3

[Boris Zbarsky [:bzbarsky]]

We could remove the const, I guess.  Is the MSVC 7.1 version in question supported?  I really do prefer having the const there... but now I seem to recall some issues with MSVC and const string members in structs...

Comment 4

[:Mook]

Attachment: Add configure check for MSVC 7.1 without service pack

This configure check errors out on MSVC 7.1 when the compiler revision is 3077 (without any service pack).

The logic being that the service pack is a free download from Microsoft, and not having it (therefore having a compiler known to silently generate bad code) doesn't make much sense :)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 248965 [details] [diff] [review]
Add configure check for MSVC 7.1 without service pack

Oooh.  This, I like!

Comment 6

[Bill Gianopoulos [:WG9s]]

(In reply to comment #4)

> The logic being that the service pack is a free download from Microsoft, and
> not having it (therefore having a compiler known to silently generate bad code)
> doesn't make much sense :)
> 
Well. using the same argument, Visual C++ 2005 Express Edition with the Microsoft Platform SDK is a free download from Microsoft, and not having it (therefore having a compiler known to silently generate bad code) doesn't make much sense :)

So why don't we just require VC8 and be done with it?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #6)
> So why don't we just require VC8 and be done with it?

Because installing an express edition of VC8 is a much bigger hassle than installing a service pack to an existing version of VC8, and there's no reason to require VC8 when VC7.1 with the service pack is known to work just fine.

Comment 8

[Benjamin Smedberg]

Comment on attachment 248965 [details] [diff] [review]
Add configure check for MSVC 7.1 without service pack

I'd prefer an actual compile check, but I understand that's almost impossible in autoconf, so this will do.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

mozilla/configure.in 	1.1757

Comment 10

[[e-mail]]

(In reply to comment #3)
>We could remove the const, I guess.  Is the MSVC 7.1 version in question
>supported?  I really do prefer having the const there... but now I seem to
>recall some issues with MSVC and const string members in structs...
That was VC6 which refuses to allow const members in POD structs point blank.
(For example, how are you supposed to assign a struct with a const member?)
Since UnitData is const it's unnecessary to make UnitInfo::name const.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yeah, that const doesn't make sense, and it should just be removed.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I removed the const, with r=bzbarsky over IRC.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... and reverted the configure check, likewise r=bzbarsky over IRC.