127985 - Build Mozilla with Microsoft Visual Studio.NET C++ 7.0

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[David Gardiner]

This is a placeholder bug to track all the things that need to be fixed to get 
Mozilla compiling with Microsoft's C++ 7.0 (The C++ compiler that comes with 
Visual Studio.NET)

-dave

Comment 1

[hacker formerly known as [e-mail]]

Didn't Hyatt & Ben already get this working as a precursor to mantic0re?

Comment 2

[Bernard Alleysson]

I used /GL (optimize across obj boundaries)compiler switch along with /LTCG
(link time code generation) linker flag to get a static build
It's working but code generation took more than 1 hour (PIII 400), and it's only
link time !
It took 650M of memory so be sure you have at least 512MB and enough paging space...

problems found:

1) work around xpidl crash by commenting out some code
2) replace #include "ver.h" by #include "winver.h" (ver.h is gone)
3) remove precompiled header option (/YX) in xpconnect makefile.win
4) replace /O1 with its equivalent from MS doc and use /Gf instead of /GF
the problem here is that /O1 (and even /O2) now defaults to /GF instead of /Gf
It means that string constants are stored in read-only memory, and I know by
experience that this causes mozilla to crash on startup... sometimes (several
profiles ?) :-(. I got this crash in nsClipboard.cpp and I could have a stack
trace if someone is interested. I think that this needs more investigation.

Comment 3

[Bernard Alleysson]

for those who want to check this out, here is the program:

#include <stdio.h>
#include <tchar.h>

int main(int argc, char* argv[])
{
	const char *test = "a";
	char *p = (char *)test;
	*p = 'b';
	// strlen() here to examine the code produced by the compiler for inline functions
	int l = strlen(p);
	// strdup here so that the code above is not optimized away
	return (int)strdup(p) + l;
}

the same source compiled with the same option (/O1 or /O2) will produce
different results with VC70 and VC60: this will crash with VC7.0 and succeed
with VC60, just because /GF is now the default instead of /Gf.

remark:

the code produced by the compiler for inline functions like strlen, memcpy,
strcpy, ... has been improved, they are 2 times faster now because rep string
instructions aren't used anymore ('repne scasb' for instance is replaced by a
simple loop)

VC60 /O1 is /Og /Os /Oy /Ob1 /Gs /Gf /Gy
VC70 /O1 is /Og /Os /Oy /Ob2 /Gs /GF /Gy

like I already said, I experience random crashes with /GF so I would recommend to 
use /O1 - /GF + /Gf when compiling with VC70. So for VC70 I'd use 

/Og /Os /Oy /Ob2 /Gs /Gf /Gy

explicitly.

personally I would add /Oi too :-), even if the code will be (little) bigger

please note that /Ob1 (ony expand functions marked 'inline') is now /Ob2 (inline
any suitable functions) so this could have a footprint impact

Comment 4

[Christian Mattar]

I'd be really interested in some startup/page-load-time comparisons between VC6 
and VC7. Is anyone capable and willing to do these?

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Bernard, can you attach your patches to make the build go with vc7?  It sounds
like you've done some good work there, and it'd be a shame to have to redo it.

Comment 6

[Bernard Alleysson]

bug 123743:
crash in xpidl.exe(xpidl_write_comment()) when building with VC++7.0

Comment 7

[Bernard Alleysson]

Attachment: patch to build mozilla with VC++7.0

mozilla/config/WIN32 :
  the code is generated at link time to allow better optimization between all
objs linked together
  /O1 is expanded and /GF is replaced with /Gf to match VC60 behaviour
  /Oi is added because VC70 now generate good code for compiler intrinsic
functions like strlen, strcpy, memcpy, ...

mozilla/js/src/xpconnect/src/makefile.win:
  pch conflicts with new linker options

mozilla/xpcom/typelib/xpidl/xpidl_util.c:
  work around the crash reported in bug 123743 by commenting out the faulty
code

Comments:

1) you can also build static with this patch
2) once this patch is applied I don't know if you can use VC60 again later
(this will depend on how VC60 treat unknown compiler and linker option)
3) this is not appropriate if you want to redistribute the generated library
files (*.lib) because only VC70 will understand them
4) 2) and 3) can be solved by removing /GL and /LTCG: options but my
understanding is that it will not produce the best code

Comment 8

[Bernard Alleysson]

oops, and this patch is just for nmake builds

Comment 9

[Bernard Alleysson]

preliminary tests at:
http://bugzilla.mozilla.org/show_bug.cgi?id=136999#c11
shows that VC7 O1 is a small footprint win over VC6 O1 (current default settings)

Comment 10

[Shankar Unni]

Isn't it a *good* thing that VC++.Net puts strings into readonly space?
Modifying literal string constants is (almost) as evil as old Fortran programs
that could modify literal number constants so that, for instance, PI == E after
an ill-behaved function call.

Shouldn't these evildoers be hunted down and "fixed"?

Comment 11

[Bernard Alleysson]

I believe they are fixed in XP code, the problem is for Windows only code
for instance, I had to fix this:

Index: nsClipboard.cpp
===================================================================
RCS file: /cvsroot/mozilla/widget/src/windows/nsClipboard.cpp,v
retrieving revision 1.77
diff -u -r1.77 nsClipboard.cpp
--- nsClipboard.cpp     6 Aug 2002 00:53:00 -0000       1.77
+++ nsClipboard.cpp     9 Aug 2002 14:59:30 -0000
@@ -93,7 +93,8 @@
 //-------------------------------------------------------------------------
 UINT nsClipboard::GetFormat(const char* aMimeStr)
 {
-  nsCAutoString mimeStr ( CBufDescriptor(NS_CONST_CAST(char*,aMimeStr), PR_TRUE
, PL_strlen(aMimeStr)+1) );
+  nsDependentCString mimeStr(aMimeStr);
+
   UINT format = 0;

   if (mimeStr.Equals(kTextMime))

maybe CBufDescriptor should be removed, see bug 98195, which has the patch

Comment 12

[Daniel Veditz [:dveditz]]

This can't be nobody, we've got to get VC7 working. It looks like the work's
basically done, what's the holdup?

Comment 13

[hacker formerly known as [e-mail]]

Having someone who actually uses MSVC 7 to verify the patch would help.  As
would a  more recent patch against the gmake build system.  Not to mention
having someone who is actually familiar with the MSVC compiler could verify that
all of those funky optimization flags are what we want/need for the entire tree.
 Did I mention how the random commenting out of a troublesome IDL_tree_to_IDL()
call in xpidl_util.c just looks suspicious?

Over to dveditz since he actually has MSVC7.

Comment 14

[David Bradley]

I think this was commented out due to bug 123743. There's lib's in that bug that
can be used, so I suspect with these two bugs, the patch could be updated to
remove the commenting of the "comment" code.

Comment 15

[jag (Peter Annema)]

I think we should try and get Windows building with /GF (I thought we were
already doing this on VC++6). We should be able to store string literals in
read-only memory, anything that tries to modify them is evil (even though it
might happen to work). In this case, someone is casting the const-ness away
before constructing the CBufDescriptor. We store the const-ness to determine
whether or not to add a null-terminator in nsCAutoString's constructor[0], the
writing of this null-terminator causes us to crash.

The smallest fix to nsClipboard.cpp is to remove the cast. The better change is
the one you have, r=/sr=jag on that. Have you run into any other crashes because
of /CF?

0: http://lxr.mozilla.org/seamonkey/source/string/obsolete/nsString.cpp#1266

Comment 16

[Bernard Alleysson]

Comment on attachment 80061 [details] [diff] [review]
patch to build mozilla with VC++7.0

sorry my patch is a mess, it tries to address, or is related to several
problems:
1) Build mozilla with /GF
2) Remove precompiled header build option for xpconnect because it blocks
experiment with /GL
3) Build mozilla with /Oi (inline functions like memcpy, memset, strcpy ,...)
4) Build mozilla with /Ob2 (inline any suitable  function not only those marked
"inline")

all that is *really* needed to build with VC7.0 is VC 7.0 version of libidl and
glib (this is bug 123743). Commenting out the code in xpidl_util.c is a
workaround until we have the proper libidl and glib version. Strictly speaking,
mozilla will build with /O1 and VC7.0 but if you want to avoid potential
problems with /GF and use the equivalent to VC6.0 current default settings, you
can set --enable-optimize=-Og -Os -Oy -Ob1 -Gs -Gf -Gy
in your mozconfig

Comment 17

[Bernard Alleysson]

"Have you run into any other crashes because of /GF?"

No, only nsClipboard (and this should be another bug, I think)
I use Mozilla built with VC7.0 as my main browser since VC7.0 has been released
I also use Mail/News
I don't use Composer, Venkman, etc ...

Comment 18

[jag (Peter Annema)]

I like your fix to nsClipboard.cpp though. Anyone here wanna r= that so it can
be checked in?

Comment 19

[hacker formerly known as [e-mail]]

Attachment: Add GLIB_PREFIX & LIBIDL_PREFIX support to win32

I decided to keep the standard names for the VC7 built libs and add support for
searching for the libs in an alternate path.  GLIB_PREFIX/bin &
LIBIDL_PREFIX/bin must be in your path before MOZ_TOOLS/bin or xpidl will still
segfault when it uses the dlls from MOZ_TOOLS/bin.

Comment 20

[hacker formerly known as [e-mail]]

Btw, if you try to build xpidl using VC6 and the VC7 libs, it will link but it
won't run.  It complains about a missing mspdb70.dll.

Comment 21

[Brian Ryner (not reading)]

Comment on attachment 103134 [details] [diff] [review]
Add GLIB_PREFIX  & LIBIDL_PREFIX support to win32

r=bryner

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 103134 [details] [diff] [review]
Add GLIB_PREFIX  & LIBIDL_PREFIX support to win32

a=asa for checkin to 1.2 (on behalf of drivers).

Comment 23

[hacker formerly known as [e-mail]]

Patch has been checked in.  The updated build instructions are at
http://www.mozilla.org/build/win32.html .