331743 - trace-malloc is not 64-bit clean

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

trace-malloc isn't 64-bit clean -- it has various places that pass pointers through 32-bit values and then back to pointers.  I have a patch that should fix the issues other than those in the log format (which probably aren't that serious, since they'll only cause problems if there are collisions of the low 32 bits of different pointers).

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Here's the patch I have so far; I snuck in one x86_64 thing and one warning fix as well.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This fixes bug 331744 as well.

I haven't fully tested it because it crashes pretty quickly for me, since some code doesn't use frame pointer linkage.  I think that's more an issue of gcc 4.1 than x86_64, but the calling conventions on x86_64 make it more of a problem.  We probably need to move to more advanced stack walking code at some point.

I should test this on i386 sometime...

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Well, it doesn't really work for me on i386 without the patch, and this leaves it in its almost-working state.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 216377 [details] [diff] [review]
patch

Sure.

/be

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: fix regression in dump format

This fixes a regression in the dump format from the previous patch.  Using %p means we print "(nil)" instead of "0x00000000".  So instead of printing the data a pointer-at-a-time, I'm printing it a long-at-a-time, which avoids portability problems and still should do pointers almost everywhere.

And then, where I read the data, I also read it long-at-a-time, for symmetry.

Padding to only 8 zeros might not be ideal for 64-bit, but this should at least work.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 252868 [details] [diff] [review]
fix regression in dump format

Just landed this patch on the trunk.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3: Improve trace-malloc memory dumps and their handling on 64-bit.

There are three categories of improvements:

 (1) using size_t* rather than unsigned long* (and "%zX" rather than
     "%lX"), to better support platforms where sizeof(long) !=
     sizeof(void*), such as Win64 (untested, though).  This is a
     non-issue for 64-bit Linux (where I tested) and Mac.

 (2) Using the correct amount of 0-padding when printing addresses to
     show how much memory space is being printed.  In other words, using
     "%016zX" on 64-bit platforms instead of "%08zX".  This change is
     cosmetic-only, though it makes the logs much more understandable.

 (3) [in leaksoup.cpp only] Fixing an occurrence of assuming that
     sizeof(int) == sizeof(void*).  This occurrence led to printing only
     the lower half of each word in the output, after doing a correct
     analysis of the memory graph.

This patch is patching three files:

 (A) nsTraceMalloc.cpp, which is the in-process Gecko trace-malloc code
     that generates the memory dumps.

 (B) adreader.cpp, which is shared utility code for reading such a
     memory dump (currently used only by leaksoup.cpp)

 (C) leaksoup.cpp, which reads in such a memory dump, performs a
     strongly connected components analysis of the memory graph, and
     writes it back out, HTML-ized, with the roots listed at the top.

A fourth file appears to need no modification since it only looks at the
stack part of the dump and not the contents of the memory:

 (D) diffbloatdump.pl, which diffs two bloat dumps and produces a stack
     tree showing the change in allocations between them

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 4: Fix -Wshadow warning in leaksoup.cpp.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 5: Fix -Wformat warnings in leaksoup.cpp related to 32-bit vs. 64-bit sizes.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0662a9525b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/343851681bc0
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb1eb32b89d8

https://tbpl.mozilla.org/?tree=Try&rev=49d369b164bb

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0662a9525b4
https://hg.mozilla.org/mozilla-central/rev/343851681bc0
https://hg.mozilla.org/mozilla-central/rev/cb1eb32b89d8