export stack walking API usable from C from XPCOM

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
XPCOM
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

Trunk
mozilla1.9alpha8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

11 years ago
I'd like to clean up the nsStackFrame* API (I never understood why they were split into separate files per platform) by having a single function that is exported from XPCOM.

I'd also like to clean up the mess of our compiling that code into three different places rather than using a single instance of the code.

(Actually, as a followup, I'd like to split it into a pair of functions so that trace-malloc can use it, and can keep up with platform-parity improvements in it; that will be a separate bug.)
(Assignee)

Comment 1

11 years ago
Created attachment 259161 [details] [diff] [review]
patch

So far I've only built and tested this on Linux.  I still need to test on Windows.  The Solaris stack walking code looks pretty broken to begin with (it depends on uninitialized stack memory being initialized to zero); I don't plan to test it.
(Assignee)

Updated

11 years ago
Whiteboard: [patch]
(Assignee)

Comment 2

11 years ago
Hrm, it's broken because of nsTraceRefcnt::LoadLibrarySymbols.  Boy, do I wish bug 189221 never landed.
(Assignee)

Updated

11 years ago
Blocks: 374829
(Assignee)

Comment 3

11 years ago
Created attachment 259278 [details] [diff] [review]
patch
Attachment #259161 - Attachment is obsolete: true
(Assignee)

Comment 4

11 years ago
Comment on attachment 259278 [details] [diff] [review]
patch

I'm not sure whether this is an appropriate way to export something of this type from XPCOM given where I want to use it.  But I do want to start cleaning up this mess.  Please let me know.  Note that this is used from toolkit/xre and xpfe/bootstrap (in nsSigHandlers) and I'd like to use it from within libtracemalloc.so.

Note that the API here isn't ready to be frozen, but after bug 374829 I'd probably be comfortable freezing it if that were helpful.  I'd like to take things one step at a time to keep it simple, though.

I'd also like to combine files back together; that is, replace the #includes that I currently have with actually putting the text back in the files.  (I wish it was never split up this way.)  When I do that I'd also want to move nsTraceRefcnt::LoadLibrarySymbols into that nsStackWalk.cpp.
Attachment #259278 - Flags: review?(benjamin)
dbaron, is this something that could be hooked up to the nsAutoLock/nsAutoMonitor potential deadlock detection code? plasticmillion did some work on trace-malloc and nsAutoLock in bug 322959, but it doesn't look like it ever got any traction.
(Assignee)

Comment 6

11 years ago
Did you intend that comment for bug 363334?  If so, the answer is no, since one of the locks isn't ours.  In general, if we switched nsTraceMalloc to being a C++ file we probably could, although the bugs it would show would be bugs only when trace-malloc is enabled.

Comment 7

11 years ago
Comment on attachment 259278 [details] [diff] [review]
patch

Instead of nsStackWalk.h, put the function header her:
http://lxr.mozilla.org/mozilla/source/xpcom/build/nsXPCOMPrivate.h#50

And use XPCOM_API(nsresult). This will ensure that the function is exported even  when --enable-libxul
Attachment #259278 - Flags: review?(benjamin) → review+
(Assignee)

Comment 8

11 years ago
That file doesn't look like it can be included from C -- it's got a bunch of (forward) class declarations in it.
(Assignee)

Comment 9

11 years ago
Created attachment 260487 [details] [diff] [review]
patch

This addresses Benjamin's comment about using XPCOM_API / EXPORT_XPCOM_API, but I'm still using nsStackWalk.h pending response to comment 8.  I also fixed some errors in the aSkipFrames handling based on testing on Linux and Windows (off by one in different directions on each platform)
Attachment #259278 - Attachment is obsolete: true
(Assignee)

Comment 10

11 years ago
Created attachment 275634 [details] [diff] [review]
patch

Merged to trunk again (which mostly consists of removing some no-longer-needed xpfe/bootstrap changes).
Attachment #260487 - Attachment is obsolete: true
(Assignee)

Comment 11

11 years ago
Comment on attachment 275634 [details] [diff] [review]
patch

This is mostly DEBUG-only, but not completely, so requesting approval.  It's part of the plan to consolidate stack-walking code and make trace-malloc usable on platforms other than Linux.
Attachment #275634 - Flags: approval1.9?
(Assignee)

Comment 12

11 years ago
And bsmedberg said yesterday on IRC that he was ok with keeping nsStackWalk.h.
Comment on attachment 275634 [details] [diff] [review]
patch

a=bzbarsky
Attachment #275634 - Flags: approval1.9? → approval1.9+

Comment 14

11 years ago
Please fix the build problem on Solaris

"/export/home/mozilla/uild/firefox-nightly/src/mozilla/xpcom/base/nsStackFrameUnix.cpp", line 281: Error: Variable args is not a structure.
"/export/home/mozilla/uild/firefox-nightly/src/mozilla/xpcom/base/nsStackFrameUnix.cpp", line 328: Error: Too few arguments in call to "int(*)(void*,void*,__FILE*)".
"/export/home/mozilla/uild/firefox-nightly/src/mozilla/xpcom/base/nsStackFrameUnix.cpp", line 359: Error: Could not find a match for cs_operate(int(void*,void*), my_user_args*) needed in NS_StackWalk(extern "C" void(*)(void*,void*), unsigned, void*).
"/export/home/mozilla/uild/firefox-nightly/src/mozilla/xpcom/base/nsStackFrameUnix.cpp", line 387: Error: func is not defined.
4 Error(s) detected.

Comment 15

11 years ago
Created attachment 276447 [details] [diff] [review]
patch for build issues
(committed)
Attachment #276447 - Flags: review?

Updated

11 years ago
Attachment #276447 - Flags: review? → review?(dbaron)
(Assignee)

Comment 16

11 years ago
Comment on attachment 276447 [details] [diff] [review]
patch for build issues
(committed)

r=dbaron
Attachment #276447 - Flags: review?(dbaron) → review+
(Assignee)

Updated

11 years ago
Attachment #276447 - Flags: approval1.9+

Updated

11 years ago
Attachment #276447 - Attachment description: patch for build issue → patch for build issues (committed)
(Assignee)

Comment 17

11 years ago
Fix checked in to trunk, 2007-08-10 14:28 -0700.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M8
You need to log in before you can comment on or make changes to this bug.