111093 - Generation of MAP and COD files

Status: VERIFIED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Shiva Thirumazhusai]

This patch will modify the mozilla release build to generate MAP and COD files
using MOZ_MAPINFO and MOZ_CODINFO tags. This affects only the windows make and
configuration files.

 leaf :- Can I get review ?
 dveditz :- Can I get Super Review ?

Comment 1

[Shiva Thirumazhusai]

Attachment: To generate MAP and COD files in the release mode

Comment 2

[Daniel (Leaf) Nunes]

tentative r=leaf, but i'm concerned that MOZ_COVERAGE defines get blown away if
MOZ_CODINFO or MOZ_MAPINFO is set.

It would probably be better to add to the existing OSFLAGS, rather than just
overwriting whatever they're set to when we get to the ifdefs

Comment 3

[Daniel Veditz [:dveditz]]

Comment on attachment 58635 [details] [diff] [review]
To generate MAP and COD files in the release mode

>+# if MOZ_DEBUG is not set and MOZ_MAPINFO
>+!if defined (MOZ_MAPINFO)

I guess I have to agree with leaf here, especially since this conflicts with my
not-yet-checked-in MOZ_COVERAGE changes :-)  but also because having to
triplicate the code means something's wrong. better might be:

  !ifdef MOZ_CODINFO
  OS_LFLAGS = $(OS_LFLAGS) /COD:$(CODFILE)
  !endif

  !ifdef MOZ_MAPINFO
  OS_LFLAGS = $(OS_LFLAGS) /MAP:$(MAPFILE) /MAPINFO:LINES
  !endif

>RCS file: /cvsroot/mozilla/config/obj.inc,v
>retrieving revision 3.11
>diff -u -r3.11 obj.inc
>--- obj.inc	1999/11/06 03:16:58	3.11
>+++ obj.inc	2001/11/21 03:10:37
>@@ -55,6 +55,8 @@
>         $(LINCS_1) 
>         $(INCS)
>         -Fd$(PDBFILE)
>+        -Fm$(MAPFILE)
>+		-FAsc
>         $(CURDIR)$(*B).c

Not enough context to see what rules this applies to, but I can't
go for this anyway unless it's conditional on your MOZ_CODINFO,
whatever you need that for--this bug doesn't explain why we need or
want these changes.

- it ***TRIPLED*** the size of my output. In a small test using only
  the xpinstall dir I went from 4.5Mb of output to 13Mb.
- slowed compilation by 15%


>@@ -151,6 +163,7 @@
>         $(LINCS_1)
>         $(INCS)
>         -Fd$(PDBFILE)
>+        -Fm$(MAPFILE)
>         -Fo.\$(OBJDIR)\
>         $(CURDIR)$(*B).c

Why does this one rule not have a -FAsc line?

>RCS file: /cvsroot/mozilla/config/rules.mak,v

> # Replace optimizer and pdb related flags to use our own conventions
> !ifdef DLLNAME
> PDBFILE=$(DLLNAME)
>+!else
>+PDBFILE=GIVENAME
>+!endif

What's GIVENAME ?

Comment 4

[Shiva Thirumazhusai]

All the points raised are valid.

  - Fixed the OS_LFLAGS just to add the needed things.
  - Removed duplicate lines.
  - Removed "GIVENAME" debug messages.
  - Included conditional compilation for map and cod file generation.

Size and Compile Time

  Size of the release binary should not change at all. I am just generating more 
files to get stack frame, souce file name and line number information for 
release builds. And it is generated only when MOZ_MAPINFO and MOZ_CODINFO flags 
are set. 

  For the builds which don't have talkback facility this is the best solution.
We need the facility to generate map and cod files for some of our products 
otherwise it will be impossible to debug the crashing problems.

According to the documentation and our makefile/config files it should not 
increase the size of the binary. But, the output directory may be have more 
files and the compile time may increase due to more files generated.

Here is the new diff.


 

Comment 5

[Shiva Thirumazhusai]

Attachment: Here is the new patch to generate cod and map files.

Comment 6

[Daniel Veditz [:dveditz]]

My size concern was not about the size of the resulting binary but rather the
huge bloat to a development tree that could suddenly cause builds to fail due to
lack of space. This might be an issue for tinderbox also.

Thank you for making these changes. sr=dveditz

Comment 7

[Marek Z. Jeziorek]

marking it edt0.9.4+ (we need also review)

Comment 8

[Shiva Thirumazhusai]

cls: Can you do review of the updated patch ? Leaf is not available.

Comment 9

[hacker formerly known as [e-mail]]

Comment on attachment 58742 [details] [diff] [review]
Here is the new patch to generate cod and map files.

r=cls

Comment 10

[Shiva Thirumazhusai]

Attachment: Removed warning messages in linking and added path for cod file generation.

Need for the updated patch :-
   1.  There link warning I noticed second time clobber_all and build_all. That
I removed now.
   2.  In the Earlier patch I added code to generate cod files but they were
generating in the src directory. I added extract compiler option to give the
path for the dist directory. And it works fine.

Comment 11

[Shiva Thirumazhusai]

When I applied the patch and re-compiled with clobber_all in the Trunk builds I
noticed linker warning messages. I removed the linker options in exe.inc and
dll.inc which I introduced for cod files.  This removed the linker warnings.

Shiva

Comment 12

[hacker formerly known as [e-mail]]

Comment on attachment 59864 [details] [diff] [review]
Removed warning messages in linking and added path for cod file generation.

r=cls

Comment 13

[Marek Z. Jeziorek]

who will do the verification? Johnathan G., is this you?

Comment 14

[Jon Granrose]

All I can do is verify the code landed.  If someone wants better verification
than that they'll have to do it themselves.

Comment 15

[Marek Z. Jeziorek]

Jonathan, this type of verification will be fine.

Comment 16

[Shiva Thirumazhusai]

Updated patch is checked into Trunk as well as 0.9.4(edt) Branch. 

Comment 17

[Ken]

Just tried this on the branch. I set MOZ_CODINFO and MOZ_MAPINFO and kicked off 
a release build. Received something like this on each module link:

LINK : warning LNK4044: unrecognized option "COD:.\.\WIN32_O.OBJ\mozilla.cod"; 
ignored

Comment 18

[Shiva Thirumazhusai]

I am currently reproducing the linker warnings in my system. I suspect the
following option is giving those linker warnings. I will update once I the full
build done.

+# if MOZ_DEBUG is not set and MOZ_CODINFO
+!if defined (MOZ_CODINFO)
+OS_LFLAGS= $(OS_LFLAGS) /COD:$(CODFILE) 
+!endif
+#MOZ_CODINFO

Comment 19

[Gustavo Verdun]

FYI: The Linker does not generate COD files, just the compiler.

Comment 20

[Shiva Thirumazhusai]

Attachment: Removed COD linker flags to avoid linker warnings

I missed out this when I created patch to remove all the linker options
relating to COD file generation. With this new patch it should all be complete.

Comment 21

[Shiva Thirumazhusai]

All the warnings are gone. Marking it fixed.

Comment 22

[Alec Flett]

I'm reopening this. my linker is crashing all over the place with these
changes.. and I've talked to others who have the same problems.. I'm using VC++
6 SP 5 on Win2k

we've got to either back these changes out, or only turn the on for release
builds (i.e. not even for opt builds, I need to be able to build opt too)

Comment 23

[Daniel Veditz [:dveditz]]

I thought all this code was supposed to be conditional on setting MOZ_MAPINFO
and MOZ_CODINFO. There should be no effect if those aren't set, and if there is
it's got to get fixed.

It looks like all the changes in obj.inc in particular are based on plain
MAPFILE and CODFILE getting set, and it looks like rules.mak sets those
regardless of the MOZ_CODFILE setting.

Comment 24

[Shiva Thirumazhusai]

I am working on the fix.

Comment 25

[Shiva Thirumazhusai]

I have created the fix and I am testing in my local tree. 

Comment 26

[Shiva Thirumazhusai]

Attachment: Disabled generating map/cod files when MOZ_MAPINFO/MOZ_CODINFO is NOTset.

With this new patch I completely removed generating map/cod files if the
MOZ_MAPINFO/MOZ_CODINFO is not set. Whether you are compiling as debug or
release mode or optimized mode you need to set MOZ_MAPINFO or MOZ_CODINFO to
gener map or cod files. Compiler version or service pack doesn't matter. 

I need review and super review for this patch. 

review - cls, leaf ?
super review - dveditz, alecf ?

Comment 27

[hacker formerly known as [e-mail]]

Comment on attachment 60934 [details] [diff] [review]
Disabled generating map/cod files when MOZ_MAPINFO/MOZ_CODINFO is NOTset.

r=cls

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 60934 [details] [diff] [review]
Disabled generating map/cod files when MOZ_MAPINFO/MOZ_CODINFO is NOTset.

>Index: dll.inc
>         echo $(OBJDIR)\$(*B).dll,            >> $(CFGFILE)
>-!ifdef MAPFILE
>+!if   defined(MOZ_MAPINFO)
>         echo $(MAPFILE),                     >> $(CFGFILE)
> !endif
>         echo $(LLIBS) $(WIN_LIBS) $(OS_LIBS) >> $(CFGFILE)
>@@ -79,7 +79,7 @@
> !ifdef DEFFILE
>                 /DEF:$(DEFFILE)
> !endif
>-!ifdef MAPFILE
>+!if   defined(MOZ_MAPINFO)
>                 /MAP:$(MAPFILE) /MAPINFO:LINES
> !endif

The ifdef MAPFILE was already there -- do you know who was using it and who
might
need to be warned about this new behavior?

You did add the /MAPINFO:LINES setting so I'd prefer

  !ifdef MAPFILE
	/MAP:$(MAPFILE)
  !ifdef MOZ_MAPINFO
	/MAPINFO:LINES
  !endif
  !endif


>Index: exe.inc
>-!ifdef MAPFILE
>+!if  defined(MOZ_MAPINFO)
>                 /MAP:$(MAPFILE)

same thing in this file, revert back to ifdef MAPFILE

>Index: rules.mak
>==================================== 
>+!if defined(MOZ_MAPINFO)
> !ifdef MAPFILE
> MAPFILE=.\$(OBJDIR)\$(MAPFILE).map
> !else
> MAPFILE=.\$*.map  # used for executables
> !endif
>+!endif
> 
>+!if defined(MOZ_CODINFO)
> !ifdef CODFILE
> CODFILE=.\$(OBJDIR)\$(CODFILE).cod
> !else
> CODFILE=.\$*.cod  # used for executables
>+!endif
> !endif

I don't think you should bracket these, at least not the MAPFILE rule
which was there before. If MOZ_CODINFO isn't set then CODFILE probably
isn't set, so it's safe to leave off the ifdefs you add here.

Comment 29

[Alec Flett]

are we going to see another patch? this absolutely must be fixed for 0.9.7 - we
can't release a milestone that doesn't build for everyone!

Comment 30

[Stephane Saux]

Is there a way developers can work around this problem?  Without a workaround a
developer has zero productivity.  I have this problem, and I can't post a patch
because I can't do the minimum qa required.

Comment 31

[Shiva Thirumazhusai]

Attachment: Patch to fix the linker crash problem.

With this patch we are good to use. I am currently testing with 0.9.4 branch.
Otherwise this patch all code to fix linker crash and things which dveditz
wanted.

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 61187 [details] [diff] [review]
Patch to fix the linker crash problem.

sr=dveditz

Comment 33

[Alec Flett]

Comment on attachment 61187 [details] [diff] [review]
Patch to fix the linker crash problem.

r=alecf

Comment 34

[jag (Peter Annema)]

I've updated my tree this morning and I'm still seeing this problem.

Comment 35

[jag (Peter Annema)]

A final clobber_all seems to have fixed things.

Comment 36

[Shiva Thirumazhusai]

Otherthan the requirement of clobber_all after the source update, I don't see
any problem report. I am marking this bug as fixed. If you anybody else sees other
problem related to this configuration change please feel free to open it.

Comment 37

[Judson Valeski]

adding fixed0.9.4 keyword

Comment 38

[Tracy Walker [:tracy]]

verifying per comments 35 and 36: building fine and no crashes reported