Generation of MAP and COD files

VERIFIED FIXED in mozilla0.9.7

Status

SeaMonkey
Build Config
--
blocker
VERIFIED FIXED
16 years ago
7 years ago

People

(Reporter: Shiva Thirumazhusai, Assigned: Shiva Thirumazhusai)

Tracking

({topembed})

Trunk
mozilla0.9.7
x86
Windows NT
topembed

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: Fixed checked into Trunk and 0.9.4 edt Branch)

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

16 years ago
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 ?
(Assignee)

Comment 1

16 years ago
Created attachment 58635 [details] [diff] [review]
To generate MAP and COD files in the release mode

Comment 2

16 years ago
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 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 ?
(Assignee)

Comment 4

16 years ago
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.


 
(Assignee)

Comment 5

16 years ago
Created attachment 58742 [details] [diff] [review]
Here is the new patch to generate cod and map files.
Attachment #58635 - Attachment is obsolete: true
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
(Assignee)

Updated

16 years ago
Keywords: edt0.9.4, topembed
Target Milestone: --- → mozilla0.9.4

Comment 7

16 years ago
marking it edt0.9.4+ (we need also review)
Keywords: edt0.9.4 → edt0.9.4+
(Assignee)

Comment 8

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

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

r=cls
Attachment #58742 - Flags: review+
(Assignee)

Comment 10

16 years ago
Created attachment 59864 [details] [diff] [review]
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.
Attachment #58742 - Attachment is obsolete: true
(Assignee)

Comment 11

16 years ago
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 on attachment 59864 [details] [diff] [review]
Removed warning messages in linking and added path for cod file generation.

r=cls
Attachment #59864 - Flags: review+
(Assignee)

Updated

16 years ago
Whiteboard: Fix check into Trunk. needs to checked into 0.9.4 edt branch

Comment 13

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

Comment 14

16 years ago
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

16 years ago
Jonathan, this type of verification will be fine.
(Assignee)

Comment 16

16 years ago
Updated patch is checked into Trunk as well as 0.9.4(edt) Branch. 
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Whiteboard: Fix check into Trunk. needs to checked into 0.9.4 edt branch → Fix checked into Trunk. Fix checked into 0.9.4 branch

Comment 17

16 years ago
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
(Assignee)

Comment 18

16 years ago
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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 19

16 years ago
FYI: The Linker does not generate COD files, just the compiler.
(Assignee)

Comment 20

16 years ago
Created attachment 60679 [details] [diff] [review]
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.
(Assignee)

Comment 21

16 years ago
All the warnings are gone. Marking it fixed.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED

Comment 22

16 years ago
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(Assignee)

Comment 24

16 years ago
I am working on the fix.
Status: REOPENED → ASSIGNED
(Assignee)

Comment 25

16 years ago
I have created the fix and I am testing in my local tree. 
Whiteboard: Fix checked into Trunk. Fix checked into 0.9.4 branch → Fix in Hand
(Assignee)

Comment 26

16 years ago
Created attachment 60934 [details] [diff] [review]
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 ?
(Assignee)

Updated

16 years ago
Whiteboard: Fix in Hand → [patch attached][needs review][needs super-review]
Comment on attachment 60934 [details] [diff] [review]
Disabled generating map/cod files when MOZ_MAPINFO/MOZ_CODINFO is NOTset.

r=cls
Attachment #60934 - Flags: review+
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.
Attachment #60934 - Flags: needs-work+

Comment 29

16 years ago
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!
Target Milestone: mozilla0.9.4 → mozilla0.9.7

Comment 30

16 years ago
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.
(Assignee)

Comment 31

16 years ago
Created attachment 61187 [details] [diff] [review]
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.
Attachment #59864 - Attachment is obsolete: true
Attachment #60679 - Attachment is obsolete: true
Attachment #60934 - Attachment is obsolete: true
Comment on attachment 61187 [details] [diff] [review]
Patch to fix the linker crash problem.

sr=dveditz
Attachment #61187 - Flags: superreview+

Comment 33

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

r=alecf
Attachment #61187 - Flags: review+
(Assignee)

Updated

16 years ago
Whiteboard: [patch attached][needs review][needs super-review] → Fixed checked into Trunk and 0.9.4 edt Branch

Comment 34

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

Comment 35

16 years ago
A final clobber_all seems to have fixed things.
(Assignee)

Comment 36

16 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED

Comment 37

16 years ago
adding fixed0.9.4 keyword
Keywords: fixed0.9.4

Updated

16 years ago
No longer blocks: 114455
verifying per comments 35 and 36: building fine and no crashes reported
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.