trace-malloc is not 64-bit clean

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
14 years ago
6 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

Trunk
mozilla30
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(5 attachments, 2 obsolete attachments)

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).
Posted patch patch (obsolete) — Splinter Review
Here's the patch I have so far; I snuck in one x86_64 thing and one warning fix as well.
Status: NEW → ASSIGNED
Whiteboard: [patch]
Posted patch patch (obsolete) — Splinter Review
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...
Attachment #216287 - Attachment is obsolete: true
Posted patch patchSplinter Review
Well, it doesn't really work for me on i386 without the patch, and this leaves it in its almost-working state.
Attachment #216325 - Attachment is obsolete: true
Attachment #216377 - Flags: review?(brendan)
Comment on attachment 216377 [details] [diff] [review]
patch

Sure.

/be
Attachment #216377 - Flags: review?(brendan) → review+
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.
Attachment #252868 - Flags: review?(brendan)
Comment on attachment 252868 [details] [diff] [review]
fix regression in dump format

Just landed this patch on the trunk.
Attachment #252868 - Flags: review?(brendan)
Component: XP Miscellany → General
QA Contact: brendan → general
Component: General → XPCOM
QA Contact: general → xpcom
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
Attachment #8377984 - Flags: review?(khuey)
You need to log in before you can comment on or make changes to this bug.