170618 - linker map file diffing tool

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Cathleen]

for tracking code size increase/decrease

Comment 1

[Garrett Arch Blythe]

I checked in some work into mozilla/tools/codesighs

For those interested, apply an upcoming patch to your build system, build, and
then run 

./mozilla/tools/codesighs/autosummary.win.bash

from the parent directory of your windows mozilla source tree to get a summary
report which I hope to hook up to tinderbox.

McAfee, I'll need help to hook up into tinderbox to watch code size changes from
build to build.

Alec, the tool mozilla/tools/codesighs/msmap2tsv.c digests a linker mapfile and
outputs in tab delimited format.  You can then reverse sort the output to see
what symbols are taking up the most space.  Hopefully this will aid you
targeting larger code size wins, we'll see....  There are problems, still, like
the classic static symbols are not visible so I am assigning their space to
something else, but this is a step forward.

Comment 2

[Garrett Arch Blythe]

Attachment: Better enable MOZ_MAPINFO

Request for r=,sr=
This needs to go in before turning on the tinderbox support.

Comment 3

[Garrett Arch Blythe]

Attachment: Sample summary file from autosummary.win.bash

Sample summary file showing code size changes detected from yesterday morning
to this morning.
As a tinderbox would be pulling every few hours, the summary would hopefully be
even more concise.

Comment 4

[Brendan Eich [:brendan]]

Looks like you just need an r= if the patch touches only build files.

/be

Comment 5

[hacker formerly known as [e-mail]]

Comment on attachment 101647 [details] [diff] [review]
Better enable MOZ_MAPINFO

I thought that new modules needed a sr before they could be enabled?  Or maybe
that's just enabled by default?  Anyhoo, r=cls on the build changes.

Comment 6

[Chris McAfee]

garrett: ok on tbox help; I will need win32 help from you :-)

Comment 7

[Garrett Arch Blythe]

checked in build file changes.

will work with McAfee in near future for tbox stuff before closing this bug.

Comment 8

[Garrett Arch Blythe]

Attachment: mozilla/tools/codesighs/Makefile no longer missing

I missed one file that was not in my prior patch, as I had created the Makefile
manually....  My mistake.
Request for r=

Comment 9

[hacker formerly known as [e-mail]]

Comment on attachment 102059 [details] [diff] [review]
mozilla/tools/codesighs/Makefile no longer missing

r=cls

Comment 10

[Garrett Arch Blythe]

Attachment: Pull code when MOZ_MAPINFO defined

seawood pointed out that I should pull the necessary directory automatically
when appropriate so the build will just work and not break.
Request for r=

Comment 11

[Garrett Arch Blythe]

Checked in ability to use tools on linux.
When run under linux, unmangled symbols result and there is a guarantee on the
sizes of the symbols being reported (well, as good as "nm" does its job).

Alecs, this might be a better way to do the size information chart you were
talking about.

Chris, we should hook this up to a linux tinderbox somewhere.

Patch coming for ability to turn on in linux build.

Comment 12

[Garrett Arch Blythe]

Attachment: Build changes to support linux

Request for r=,a=

Build changes to enable building codesighs under linux.

Comment 13

[hacker formerly known as [e-mail]]

Comment on attachment 102531 [details] [diff] [review]
Build changes to support linux

Why are you checking for MOZ_MAPINFO != "" instead of just using the ifdef?  

The configure option should check for the existence of the codesighs directory
since we don't pull it by default.

Comment 14

[Garrett Arch Blythe]

re: comment #13

I guess I was confused re: wether or not MOZ_MAPINFO would be defined as I saw
it in autoconf.mk as "MOZ_MAPINFO =" when not enabled.  I will revert to the
ifdef instead of the string compare and make sure everything works.

I will also check for the existance of said directory, as I failed to realize
the client.mk checkout build step doesn't run/respect the variable set by configure.

Thanks for the feedback.

Comment 15

[Garrett Arch Blythe]

Attachment: Better patch for linux

Request for r=,a=

New build changes to support the linux build.

Comment 16

[Garrett Arch Blythe]

Attachment: Even better patch for linux.

Request for r=, a=

Error in case directory does not exist instead of warn as suggested by cls.

Comment 17

[hacker formerly known as [e-mail]]

Comment on attachment 102640 [details] [diff] [review]
Even better patch for linux.

r=cls

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 102640 [details] [diff] [review]
Even better patch for linux.

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

Comment 19

[Garrett Arch Blythe]

checked in for linux, closing bug