Last Comment Bug 364049 - patch for bug 347705 caused bustage for optimized msvc7.1 builds
: patch for bug 347705 caused bustage for optimized msvc7.1 builds
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: :Mook
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-12-16 05:11 PST by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2007-01-25 22:48 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
screenshot (117.17 KB, image/png)
2006-12-16 05:12 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details
Add configure check for MSVC 7.1 without service pack (1.38 KB, patch)
2006-12-17 21:36 PST, :Mook
benjamin: review+
bzbarsky: superreview+
Details | Diff | Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2006-12-16 05:11:51 PST
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: gavin@gavinsharp.com] 2006-12-16 05:12:25 PST
Created attachment 248825 [details]
screenshot
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-12-16 07:41:20 PST
It looks like installing Visual Studio .NET 2003 SP1 fixes this bug. I guess that might make this WONTFIX.
Comment 3 Boris Zbarsky [:bz] 2006-12-17 19:48:18 PST
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 2006-12-17 21:36:16 PST
Created attachment 248965 [details] [diff] [review]
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 [:bz] 2006-12-17 21:40:31 PST
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] 2006-12-18 04:26:58 PST
(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: gavin@gavinsharp.com] 2006-12-18 09:14:50 PST
(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 [:bsmedberg] 2006-12-20 07:57:01 PST
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: gavin@gavinsharp.com] 2007-01-04 11:59:38 PST
mozilla/configure.in 	1.1757
Comment 10 neil@parkwaycc.co.uk 2007-01-05 00:07:10 PST
(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-7 (review requests must explain patch) 2007-01-25 22:35:11 PST
Yeah, that const doesn't make sense, and it should just be removed.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-01-25 22:39:12 PST
I removed the const, with r=bzbarsky over IRC.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-01-25 22:48:16 PST
... and reverted the configure check, likewise r=bzbarsky over IRC.

Note You need to log in before you can comment on or make changes to this bug.