Closed Bug 374689 Opened 17 years ago Closed 17 years ago

export stack walking API usable from C from XPCOM

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Whiteboard: [patch])

Attachments

(2 files, 3 obsolete files)

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.)
Attached patch patch (obsolete) — Splinter Review
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.
Hrm, it's broken because of nsTraceRefcnt::LoadLibrarySymbols.  Boy, do I wish bug 189221 never landed.
Attached patch patch (obsolete) — Splinter Review
Attachment #259161 - Attachment is obsolete: true
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.
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 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+
That file doesn't look like it can be included from C -- it's got a bunch of (forward) class declarations in it.
Attached patch patch (obsolete) — Splinter Review
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
Attached patch patchSplinter Review
Merged to trunk again (which mostly consists of removing some no-longer-needed xpfe/bootstrap changes).
Attachment #260487 - Attachment is obsolete: true
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?
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+
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.
Attachment #276447 - Flags: review? → review?(dbaron)
Comment on attachment 276447 [details] [diff] [review]
patch for build issues
(committed)

r=dbaron
Attachment #276447 - Flags: review?(dbaron) → review+
Attachment #276447 - Attachment description: patch for build issue → patch for build issues (committed)
Fix checked in to trunk, 2007-08-10 14:28 -0700.
Status: NEW → RESOLVED
Closed: 17 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.

Attachment

General

Created:
Updated:
Size: