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

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Gavin, Assigned: Mook)

Tracking

({regression})

Trunk
x86
Windows XP
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
Created attachment 248825 [details]
screenshot
It looks like installing Visual Studio .NET 2003 SP1 fixes this bug. I guess that might make this WONTFIX.
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...
(Assignee)

Comment 4

11 years ago
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 on attachment 248965 [details] [diff] [review]
Add configure check for MSVC 7.1 without service pack

Oooh.  This, I like!
Attachment #248965 - Flags: superreview+
Attachment #248965 - Flags: review?(benjamin)
(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?

(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 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.
Attachment #248965 - Flags: review?(benjamin) → review+
Assignee: dbaron → mook.moz+sites.org.mozilla.bugzilla
Whiteboard: [checkin needed]
mozilla/configure.in 	1.1757
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]

Comment 10

11 years ago
(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.
Yeah, that const doesn't make sense, and it should just be removed.
I removed the const, with r=bzbarsky over IRC.
... and reverted the configure check, likewise r=bzbarsky over IRC.
You need to log in before you can comment on or make changes to this bug.