Closed Bug 281158 Opened 20 years ago Closed 19 years ago

Internal compiler error with MSVC 6 SP6

Categories

(Core :: Web Painting, defect, P4)

Product:
Core
Component:
Web Painting
Platform:
x86
Windows ME
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: piskozub, Assigned: dougt)

Details

(Whiteboard: patch)

Attachments

(5 files, 2 obsolete files)

Patch v. 1.1
682 bytes, patch
dougt
: superreview-
Details | Diff | Splinter Review
Workaround by changing code #1
1.55 KB, patch
Details | Diff | Splinter Review
Workaround by changing code #2, v1.1
2.53 KB, patch
dougt
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
Workaround by changing code #2, v1.2 (checked in)
2.60 KB, patch
Details | Diff | Splinter Review
Fix Doxygen output using @verbatim
721 bytes, patch
Biesinger
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.7.6) Gecko/20050204
Build Identifier: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.7.6) Gecko/20050204

More exactly it was the "2005-02-02 22:26 Fix win32 build bustage" checkin by
bzbarski after the fix for bug 280041.

What happens? I got an intyernal compiler (VS 6) error on Win ME:

nsViewManager.cpp
c:/mozilla_source/mozilla/view/src/nsViewManager.cpp(1697) : fatal error C1001:
INTERNAL COMPILER ERROR
                (compiler file 'F:\9782\vc98\p2\src\P2\main.c', line 494)
    Please choose the Technical Support command on the Visual C++
    Help menu, or open the Technical Support help file for more information
make[4]: *** [nsViewManager.obj] Error 2
make[4]: Leaving directory `/cygdrive/c/mozilla_source/mozilla/view/src'
make[3]: *** [libs] Error 2
make[3]: Leaving directory `/cygdrive/c/mozilla_source/mozilla/view'
make[2]: *** [tier_9] Error 2
make[2]: Leaving directory `/cygdrive/c/mozilla_source/mozilla'
MAKE[1]: *** [default] Error 2
MAKE[1]: Leaving directory `/cygdrive/c/mozilla_source/mozilla'
MAKE: *** [build] Error 2

nsViewManager.cpp(1697) is exactly the line bzbarski corrected for the recent
win32 bustage. It seems it did not work for Win9x.

Reproducible: Always

Steps to Reproduce:
1. Compile mozilla on WinME
2. Read the error
3. Cry out loudly

Actual Results:  
Compilation crashed

Expected Results:  
Build the suite, of course.
Reassigning to zbarski as the guily party. Sorry, mate.
Assignee: roc → bzbarsky
Correcting Boris's surname spelling from Polish to Russian. Sorry, again.
Summary: Internal compiler error caused by bzbarski bug 280041 fix → Internal compiler error caused by bzbarsky bug 280041 fix
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b) Gecko/20050205
Firefox/1.0+

http://lxr.mozilla.org/seamonkey/source/view/src/nsViewManager.cpp#1697

1693 void
1694 nsViewManager::WillBitBlit(nsView* aView, nsPoint aScrollAmount)
1695 {
1696   if (!IsRootVM()) {
1697     RootViewManager()->WillBitBlit(aView, aScrollAmount);
1698     return;
1699   }

I have just checked out and built Firefox on the Mac; all the Tinderboxes
look OK. Why do you think that an internal compiler error is attributable
to the code in nsViewManager::WillBitBlit(nsView* aView, nsPoint 
aScrollAmount) .

Did you repeat the build and find it stopped at the same place?

My congratulations/commiserations/best wishes for building on Win98
in the first place.
Yes, I did it twice with the same result.

Don't be ironic about building on Win9x. I have been doing it successfully since
early 2001. Before this one,  I have never seen a repeatable bustage that was
not visible on tinderbox.
> Why do you think that an internal compiler error is attributable
> to the code in nsViewManager::WillBitBlit(nsView* aView, nsPoint 
> aScrollAmount) .

Well. First of all this is what the compiles said in its famous last words )see
the bug report above).

Second. There was a Win32 bustage at that same position in the same file 3 days
ago. 
"2005-02-02 22:26	bzbarsky%mit.edu 	mozilla/view/src/nsViewManager.cpp  3.384 
2/1  Fix win32 build bustage"

It is my first build since then. The previous on on January 31 was successful.


The batch bzbarsky used fo fix the bustage changed these very lines of a new
code block that was added on the same day:

1695    RootViewManager()->WillBitBlit(aView, aScrollAmount);
1696    return;

before that the new code that crerated the original 20050202 bustage was:

1695    return RootViewManager()->WillBitBlit(aView, aScrollAmount);                
1696
	
Both now and before the following line 1697 is the block end. I do not believe a
block end created a compiler internal error. 

To sum it up, this code fragment _did_ create a win32 bustage. It was corrected
but I still got a bustage in the same very place on the first (and second)
compilation after the change. Ergo, I believe the cause is still the same. 

Does that convince you?
                      


I clobbered my tree and tried again. No luck. That makes three identical build
bustages:

c:/mozilla_source/mozilla/view/src/nsViewManager.cpp(1697) : fatal error C1001:
INTERNAL COMPILER ERROR
                (compiler file 'F:\9782\vc98\p2\src\P2\main.c', line 494)

The error number, Mozilla file and line and compiler line are all identical
every time. BTW, the compiler file path is evidently from a Microsoft build
machine, not my PC - so I have no idea what's line 494 of \vc98\p2\src\P2\main.c).
if all fails may be you should the manual, aka msdn ;-), 

http://search.microsoft.com/search/results.aspx?qu=main.c'%2c+line+494&View=msdn&st=b&c=0&s=1&swc=0


Thanks. That resolves to three possible problems:

http://support.microsoft.com/default.aspx?scid=kb;en-us;216718
http://support.microsoft.com/default.aspx?scid=kb;en-us;259244
http://support.microsoft.com/default.aspx?scid=kb;en-us;217164

The first was allegedly resolved by SP3. I have the latest (SP6, I believe) so
let's forget about this.

The second assumes we use _assume() function [sorry for the pun]. We don't.

The third says "Bad code is generated when you pass the return value of an
intrinsic function as an argument to a function that takes a reference to an
integer while using Global Optimizations (/Og) and Enable Intrinsic Functions
(/Oi)."
The problem is we do not use any intristic function (as defined by Microsoft) here.

Did we find a fourth case?
It seems the bustage is optimizer caused. Changing -O1 to -Od (no optimization)
seemed to solve the problem. 

However, -O1 is the default for Win32 builds so it a workaround, not a real fix
for the problem.
Jacek, I'm going to need some help here, since there's no way I can possibly
reproduce your issue, and because that code pattern is quite commonly used in
that file (so I'm not even sure what VC6 doesn't like about this particular
instance).

So for a start:

1) Does commenting out that WillBitBlit() call make the compiler not die?
2) Does using a nsViewManager* temporary for the return value of
   RootViewManager() help?
Priority: -- → P4
Target Milestone: --- → mozilla1.8beta
OK. I'll work on this, starting with commenting the line out.
> 1) Does commenting out that WillBitBlit() call make the compiler not die?

WFM with this line commented out:
//    RootViewManager()->WillBitBlit(aView, aScrollAmount);

Starting to test 2)...
Per 2):

Alas! The following code:
    nsViewManager* dupa = RootViewManager();
    dupa->WillBitBlit(aView, aScrollAmount);
crashes the compiler in an identical way (and the actual the line number it
gives is the second line).

Isn't the recursive way WillBitBlit() is defined the reason, then?


> Isn't the recursive way WillBitBlit() is defined the reason, then?

Why should that cause a compiler error, though?  I'm just getting another object
and calling a method on it....

Note, again, that the same code pattern is used all over this file (search for
"!IsRootVM()").

I'm open to suggestions on how I can call the WillBitBlit method of
mRootViewManager here without MSVC barfing....
What about the optimization pragmas (see comment #9) around the function?

#pragma optimize( "", off )
.
.
.
#pragma optimize( "", on ) 

Of course we need a #ifdef for vc6 (whatever it is) around the pragmas.
The following code still gives the internal compiler error. Did I use the right
constant name?

#ifdef _WIN32_MSVC
#pragma optimize( "", off )
#endif
void
nsViewManager::WillBitBlit(nsView* aView, nsPoint aScrollAmount)
{
[...]
}
#ifdef _WIN32_MSVC
#pragma optimize( "", on )
#endif

BTW, the use of these pragmas is described here:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vccore98/html/_predir_optimize.asp

"Using the optimize pragma with the empty string (" ") is a special form of the
directive. It either turns off all optimizations or restores them to their
original (or default) settings."

Maybe I should de-optimize the whole file? I can try that in the (European)
afternoon.
It seems the pragma solution does not work - even when I mark all functions in
nsViewManager.cpp as not optimizable.
> Did I use the right constant name?

Not sure.  Does it work if you use _MSC_VER instead?
Attached patch This patch fixes the problem (obsolete) — — Splinter Review 
It worked. Can we check this before the tree freeze?
Attachment #173641 - Flags: superreview?
Attachment #173641 - Flags: review?
This disables optimization for all MSVC versions.  Shouldn't you only disable it
for the buggy version in question?

Also, you want to request review from someone specific (and it needs to be
someone familiar with msvc, looks like).
Assuming it fails in all MSVC 6 versions, I guess

#if (_MSC_VER < 1300)

would be adequate check. I'd also tell in the comment that it fixes an internal
compiler error to point out there's nothing wrong with the code itself.
Attached patch Patch v. 1.1 — — Splinter Review 
This patch is only for version 1200 (which is what I have).

No idea yet whom I need for reviewers (however the win32 build documentation
was done by Benjamin Smedberg and Jeff Walden. On the otherhand, dougt used
_MSC_VER so he should know...
Attachment #173641 - Attachment is obsolete: true
Attachment #173644 - Flags: superreview?(dougt)
Attachment #173644 - Flags: review?(jwalden+bmo)
Comment on attachment 173641 [details] [diff] [review]
This patch fixes the problem

can you limit this to MSVC 6?
(#if _MSC_VER == 1200 should work, I think)
1200 should stay as it is in attachment 173644 [details] [diff] [review]. I do not even know if this
pragma was used in MSVC 5 (1100).

The comment could read 

// Pragmas around the function are needed to avoid MSVC 6 internal compiler error

as it fits into one line. But I do not want to create a new patch just for this.
Until you think the patch will be reviewed sooner if it is subm,itted by someone
more known for submitting fixes. In this case, please feel free to re-submit the
patch and/or change the s and sr requests I improvised. 
Disabling optimization for the entire file seems like an awful solution.  Worst
case figure out how to rewrite the function so that it doesn't break.

Also, I'm pretty sure all the tinderboxes build with VC6 and they aren't seeing
this bustage.  Do you have all the latest VC6 service packs?
I disable the optimization only for one function (14 lines including two empty
ones and two comment lines).

I'm not sure why tinderboxes don't see this bustage. Possibly they use MSVC 7
(and certainly on a different Windows version). I see this bustage on _two_
different machines, both with MSVC 6 SP6 (the latest one there is). The patch
disables optimization for a few lines and one (old) compiler version. The cost
and risk seem to me minimal (especially as presently it is not possible to
compile the suite with that version).
release builds as well as tinderboxes are using vc6, btw. although I bet they
use SP5, since SP6 is incompatible with the processor pack, which is needed for NSS.
Comment on attachment 173644 [details] [diff] [review]
Patch v. 1.1

Me, being asked to review a change to accommodate a compiler I probably
couldn't afford?  This is almost funny...

Oh, ahem.  You'll need to get someone else to review this, because I'm nowhere
near qualified to do so.  I just wrote up the steps for getting a build going;
that doesn't mean I know a thing about how the build process works.
Attachment #173644 - Flags: review?(jwalden+bmo)
ccing some people who may be able to help...
I am inclined to WONTFIX this, since it doesn't appear using MSVC SP5, which is
the only supported configuration.
I just started downloading SP5, just in case I need to make the SP6->SP5
downgrade (meaning de- and re-installing the whole MSVC). 

However, it would seem a strange decision to break Mozilla compilation (more or
less on purpose as we have a working patch) for the most recent service pack of
the most widely used compiler on the most used platform.

Well, it's your choice.
Hmmm, weren't there other reasons that SP6 didn't work already?
I build with VC6.0SP5 since ages and I dont see this issue. One problem I have
seen however, and it increases bloodpressure, if people require the processor
pack ( like the jpg did for a time). If you have the VC6.0 100USD standard
edition you cannot apply the processor pack, you need the much more expensive
professional edition. 
Changing the summary to avoid dupes.
Summary: Internal compiler error caused by bzbarsky bug 280041 fix → Internal compiler error with MSVC 6 SP6
WFM with MSVC 6 SP5, after downgrading from SP6. However, take note that this is
not the point of the bug which is about MSVC 6 SP6.

I decided against downgrading on my other machine (at least for now) to enable
verifying this bug in case the patch is actually applied.
One more comment: Shoudn't the win32 build page
(http://www.mozilla.org/build/win32.html) be more explicit about SP5 being the
only supported version of Visual C++ 6? I mean, the present text technically
correct ("Version 6 requires installation of the Visual C++ Service Pack 5  and
Visual C++ Processor Pack." - as there is no Processor Pack for SP6).

However the way an experienced developer understands "requires installation of
the Visual C++ Service Pack 5" on a Web page is as equal to "requires
installation of the Visual C++ Service Pack 5 _or later_" because many Web pages
tend to be out of date.

I believe a sentence about SP6 being not supported (maybe with a link to this
bug) would be helpful.
Comment on attachment 173644 [details] [diff] [review]
Patch v. 1.1

SP6 isn't supported yet.

I also see a similar failure when building this file using the MS Embedded C
4.0sp4 compiler when using -Ox when compiling for x86 WinCE emulator.


As for the patch, we should add some notes to why we are using this #pragma to
the patch.  Cite the bug and specifically comment #16 in the code.
Attachment #173644 - Flags: superreview?(dougt) → superreview-
I'm clearly not working on this (esp. since I have no way to possibly reproduce
or test patches).  If we support this compiler version, then we probably need to
check in some variant of that workaround.  Otherwise, we should wontfix the bug.
 In either case, let's not have this just sit here?
Assignee: bzbarsky → nobody
Target Milestone: mozilla1.8beta1 → ---
Doug, could you patch or WONTFIX this?
Assignee: nobody → dougt
Note, this patch is applied for WIN_CE but not for (_MSC_VER == 1200).
Please also add this patch for (_MSC_VER == 1200)...
I get the same error when I compile the source in Windows XP, using Microsoft
Visual C++ 6.0
Harish, which service pack have installed for MSVC? I stopped seeing this
problem after downgrading from SP6 to SP5. This is a little painfull as the only
way to do it is to uninstall MSVC6, install it again and apply the Mozilla
supported (5) version of the service pack. 
In rev. 3.401 of nsViewManager.cpp, Doug checked in the
workaround but it was only used for WINCE.

I am running into this internal compiler error under
MSVC 6.0 SP5 (not SP6), with Processor Pack and with
Microsoft Platform SDK February 2003.
If I make the RootViewManager()->WillBitBlit() call in
another function, the compilation works.  That other
function can even be inline.
Attached patch Workaround by changing code #2 (obsolete) — — Splinter Review 
If I make the WillBitBlit method return nsresult instead
of void, the compilation works.  Note that either
    return RootViewManager()->WillBitBlit(aView, aScrollAmount);
or
    RootViewManager()->WillBitBlit(aView, aScrollAmount);
    return NS_OK;
works.
So this now hits also the supported MSVC6 SP5 version. 

Well, we have a patch which could be easily updated and checked in - if only
there is "political" will to do it.
Whiteboard: patch
Does assigning the result of RootViewManager() to a temporary variable fix it? 
That seems less intrusive.
David: It didn'tfor me. See comment #13.
David: assigning the result of RootViewManager() to a temporary
variable doesn't fix it.  I tried all the simpler code changes
(ones that are contained in that method) first but none of them
worked.
Doug,

Could you please review this patch and test it
under Microsoft eMbedded Visual C++ 4.0 SP4?

Your former manager

Does Doxygen have a keyword for notes or remarks?
Attachment #197644 - Flags: review?(dougt)
Attachment #197468 - Attachment is obsolete: true
Again, my compiler is MSVC 6.0 SP5 (verified by examining this
registry key:
HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\VisualStudio\6.0\ServicePacks).
I believe I installed the Processor Pack, but I don't know how to
verify that.  I'm using the compiler with Microsoft Platform SDK
February 2003.  The compiler reports its version as follows:

Microsoft (R) 32-bit C/C++ Optimizing Compiler Version 12.00.8804 for 80x86
Copyright (C) Microsoft Corp 1984-1998. All rights reserved.
Comment on attachment 197644 [details] [diff] [review]
Workaround by changing code #2, v1.1

Yes, this worked fine under evc4 sp4.

Your former favorite employee.
Attachment #197644 - Flags: review?(dougt) → review+
Comment on attachment 197644 [details] [diff] [review]
Workaround by changing code #2, v1.1

David, could you review this workaround?

I have confirmed that my machine doesn't have Processor
Pack.  It only has MSVC 6.0 SP5.  (Recall that the only
reason we need Processor Pack is to get MASM ml.exe,
which I already have.)	I found a machine with
MSVC 6.0 SP5 + Processor Pack installed and its registry key
HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\VisualStudio\6.0\ServicePacks
has an entry with name "ProcessorPack", type REG_SZ, and empty
data.  My machine doesn't have that registry key entry.

So this internal compiler error hits MSVC 6.0 SP5 (without
Processor Pack) and SP6 (which Processor Pack can't be used
with).
Attachment #197644 - Flags: superreview?(dbaron)
(In reply to comment #50)
> Does Doxygen have a keyword for notes or remarks?

@note (http://www.stack.nl/~dimitri/doxygen/commands.html#cmdnote)
Attachment #197644 - Flags: superreview?(dbaron) → superreview+
Christian: thanks for the link to Doxygen keywords.
Does this mean the @returns keywords in
mozilla/view/src/nsViewManager.h are wrong and should
be changed to @return ?
This is what I checked in on the trunk.  I only changed
the comment.  I use @note to write the note, and use
@return instead of @returns.  I also note that SP5 (without
the Processor Pack) also has the internal compiler error.

Checking in nsViewManager.cpp;
/cvsroot/mozilla/view/src/nsViewManager.cpp,v  <--  nsViewManager.cpp
new revision: 3.416; previous revision: 3.415
done
Checking in nsViewManager.h;
/cvsroot/mozilla/view/src/nsViewManager.h,v  <--  nsViewManager.h
new revision: 3.160; previous revision: 3.159
done
If you are affected by this bug (e.g., if you use MSVC 6.0 SP6),
please verify my fix by checking out and building the tip of the
source tree.  Thanks.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
I tested and it looks like doxygen treats @return and @returns identically.
Christian: thanks for testing.  Does my comment achieve
the desired effect (a note section of three paragraphs)?
Does the second paragraph (INTERNAL COMPILER ERROR) look
right?  (Should be two lines, ideally with the same
indentation as the original comment.)
it has three paragraphs; but the error line has no indentation, and the path
components \foo got apparently treated as doxygen commands and ignored,
resulting in:

    fatal error C1001: INTERNAL COMPILER ERROR (compiler file 'E:.c', line 494)
Christian: could you please try adding @verbatim @endverbatim to
that paragraph?

    * @par
+   * @verbatim
    *   fatal error C1001: INTERNAL COMPILER ERROR
    *               (compiler file 'E:\8966\vc98\p2\src\P2\main.c', line 494)
+   * @endverbatim
that works, if the asterisks are removed as well (including on the @endverbatim
line); otherwise, they show up as part of the error message.
The comment looks a little ugly without the asterisks,
but this should work.  Christian, could you review and
test this patch?  Thanks.
Attachment #198359 - Flags: review?(cbiesinger)
Comment on attachment 198359 [details] [diff] [review]
Fix Doxygen output using @verbatim

Two alternatives are 1) (ab)use the @code @endcode commands
(they seem to ignore the asterisks), and 2) simply delete the
internal compiler error message from the comment.
Comment on attachment 198359 [details] [diff] [review]
Fix Doxygen output using @verbatim

works fine
Attachment #198359 - Flags: review?(cbiesinger) → review+
Comment on attachment 198359 [details] [diff] [review]
Fix Doxygen output using @verbatim

Boris, could you super-review this patch?

I first had to change the return type of WillBitBlit
from void to nsresult to work around a MSVC internal
compiler error.  I tried to document the workaround in
the comment, but the backslashes in the compiler file's
pathname were interpreted as Doxygen commands.	This
patch uses Doxygen's @verbatim and @endverbatim commands
around the problematic comment.  Unfortunately I can't
put the leading asterisks in the @verbatim block, so
the comment looks a little ugly.
Attachment #198359 - Flags: superreview?(bzbarsky)
Comment on attachment 198359 [details] [diff] [review]
Fix Doxygen output using @verbatim

Here's hoping the other compilers optimize away this return value... :(
Attachment #198359 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 198359 [details] [diff] [review]
Fix Doxygen output using @verbatim

I checked in this patch on the trunk (1.9a1).

Checking in nsViewManager.h;
/cvsroot/mozilla/view/src/nsViewManager.h,v  <--  nsViewManager.h
new revision: 3.161; previous revision: 3.160
done
Attachment #173641 - Flags: superreview?
Attachment #173641 - Flags: review?
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: