698580 - Stack unwinding API that weaves together native and JS frames

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

For part of bug 642054, I'm working on a reusable CaptureStack that grabs not just the stack of return addresses, but also enough information to identify and interleave JS frames. I also have an iterator that allows traversing it. I figure it might be useful for other profilers (eg SPS and jprof) as well as anyone who just wants to see the additional detail.

It relies on libunwind, and it's looking like it may require changes to its JIT code registration API, so I'll need to pull it into the tree somewhere.

Comment 1

[Steve Fink [:sfink] [:s:]]

Note that I have a patch queue containing this at http://hg.mozilla.org/users/sfink_mozilla.com/patches-jsprofiler

At the moment, it's partially functional. (It does properly interleave the stack, but there are many caveats including a build issue or three. And Linux only.)

Comment 2

[Steve Fink [:sfink] [:s:]]

Still Linux only. Full functionality depends on having libunwind installed and configuring with --enable-libunwind, but will fall back to a much degraded mode using glibc's backtrace() and gcc's builtin stuff if libunwind is not used.

Probably does not yet compile on non-Linux platforms.

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Patch 1: Use reader/write reentrancy checks in HashTable.h to allow signal handlers to do concurrent reads

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Patch 2: Implement some algorithms that operate on collections

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: Patch 3: implement an --enable-libunwind configure option

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Patch 4: devtools::profiler::CaptureBacktrace() and friends for grabbing C++ and JS stacktraces, interleaving them, and resolving the symbols

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Patch 5: Implement a backtrace() function in the shell that calls devtool::profiler::Backtrace() and friends

Comment 8

[Steve Fink [:sfink] [:s:]]

Attachment: Patch 6: Test for backtrace(). Test will pass even if no support for interweaving stack frames is available.

Comment 9

[Steve Fink [:sfink] [:s:]]

Comment on attachment 589717 [details] [diff] [review]
Patch 1: Use reader/write reentrancy checks in HashTable.h to allow signal handlers to do concurrent reads

I need this because otherwise in a debug build I can get a reentrancy guard failure when a function called from a signal handler does a read operation on a hashtable that is currently being read in the interrupted thread, even though that is safe.

Comment 10

[Steve Fink [:sfink] [:s:]]

Comment on attachment 589719 [details] [diff] [review]
Patch 2: Implement some algorithms that operate on collections

Sorry, this is ugly and almost certainly in the wrong place. I see similar stuff in jsutil.h, and the names are capitalized there. So this r? is really a "please tell me how to do this properly."

Comment 11

[Steve Fink [:sfink] [:s:]]

Comment on attachment 589725 [details] [diff] [review]
Patch 4: devtools::profiler::CaptureBacktrace() and friends for grabbing C++ and JS stacktraces, interleaving them, and resolving the symbols

This is the implementation itself.

Note that this is still failing in a couple of situations on the tryserver (only currently known failure: win64), but it'll take me some time to track those down and the fixes will be minor judging from experience, so I'd like to finally put this up for review now.

Also note that the real Windows functionality isn't implemented in this patch, but it'll behave the same as other OSes when --enable-libunwind is not given: you'll still get both C++ and JS stacks, but the C++ stacks will only be return addresses, and the JS stack might miss some JITted frames.

I'll fill in the rest in a followup.

Comment 12

[Steve Fink [:sfink] [:s:]]

Comment on attachment 589728 [details] [diff] [review]
Patch 5: Implement a backtrace() function in the shell that calls devtool::profiler::Backtrace() and friends

Sorry, I know this overlaps with dumpStack().

Comment 13

[Steve Fink [:sfink] [:s:]]

Attachment: Patch 4: devtools::profiler::CaptureBacktrace() and friends for grabbing C++ and JS stacktraces, interleaving them, and resolving the symbols

Comment 14

[Steve Fink [:sfink] [:s:]]

Comment on attachment 590266 [details] [diff] [review]
Patch 4: devtools::profiler::CaptureBacktrace() and friends for grabbing C++ and JS stacktraces, interleaving them, and resolving the symbols

Fixed some problems uncovered by running this from a signal handler and with the full browser, which I haven't done in a while.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 589723 [details] [diff] [review]
Patch 3: implement an --enable-libunwind configure option

Review of attachment 589723 [details] [diff] [review]:
-----------------------------------------------------------------

Why do we need this?

Comment 16

[Steve Fink [:sfink] [:s:]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #15)
> Comment on attachment 589723 [details] [diff] [review]
> Patch 3: implement an --enable-libunwind configure option
> 
> Review of attachment 589723 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why do we need this?

libunwind is needed to support the interleaving of native frames and JS frames on non-Windows platforms. Specifically what is required is the address of the stack pointer just before invoking the native call at depth d in the stack, for all d. (Even more specific: we need the CFA address so we can add an offset and pull out local variables and parameters, but the CFA is pretty much always equal to the pre-call stack pointer or a small constant offset from it.)

And since you asked, this is used for both js::Interpret frames and mjit frames to pull out local variables giving the starting and final Javascript stack frame pointers that represent the subsequence of stack frames in the JS stack that are "handled" by that native frame.

glibc's backtrace() is insufficient, since it just gives the return addresses, not the stack pointers. gcc supports it natively, sort of, using the __builtin_frame_address(d) keyword, but that requires a compile-time constant d, and the documentation says scary things about it.

In the absence of this support, I can still pull out all of the native frames and all of the JS frames, but I can't interleave them.

A followup to this bug will be to install libunwind on all the non-Windows build boxes. (I have not yet implemented the equivalent support on Windows, but the functionality appears to be available by default.)

I hope I answered your question somewhere in there.

Comment 17

[Steve Fink [:sfink] [:s:]]

Comment on attachment 590266 [details] [diff] [review]
Patch 4: devtools::profiler::CaptureBacktrace() and friends for grabbing C++ and JS stacktraces, interleaving them, and resolving the symbols

Canceling review for now because the latest rebase looks like it may not be straightforward. Plus I think I can implement some over luke's verbal comments.

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 589717 [details] [diff] [review]
Patch 1: Use reader/write reentrancy checks in HashTable.h to allow signal handlers to do concurrent reads

Review of attachment 589717 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/Utility.h
@@ +857,5 @@
> +class ReentrancyGuardRW
> +{
> +    /* ReentrancyGuardRW is not copyable. */
> +    ReentrancyGuardRW(const ReentrancyGuardRW &);
> +    void operator=(const ReentrancyGuardRW &);

Use MOZ_DELETE from "mozilla/Attributes.h" here, for explicitness and because it makes misuse a guaranteed compile-time error (versus potentially link time if the user is a friend of the class).

Comment 19

[Luke Wagner [:luke]]

Comment on attachment 589717 [details] [diff] [review]
Patch 1: Use reader/write reentrancy checks in HashTable.h to allow signal handlers to do concurrent reads

Nice solution.

>-        ReentrancyGuard g(*this);
>+        REENTRANCY_GUARD(ReentrancyGuardRW::OP_READ);

Could you use some macro symbol concating in REENTRANCY_GUARD so that these can all read:

  REENTRANCY_GUARD(READ);

?  That avoids the unsightly mixing of ALL_CAPS(AndCamelCase).

>+#ifdef DEBUG
>+    ReentrancyGuardRW(T &obj, Operation op)
>+      : enteredForRead(obj.enteredForRead),
>+        enteredForWrite(obj.enteredForWrite),
>+        initiallyReading(obj.enteredForRead)
>+#else
>+    ReentrancyGuardRW(T &/*obj*/)
>+#endif
>+    {

IIUC, ReentrencyGuardRW is only constructed in debug builds, so could you just put the whole class in one #ifdef DEBUG and avoid the inter-line mangling?  I see that ReentrencyGuard does the same mangling so (if it works), could you wrap that in a single #ifdef DEBUG as well?

Comment 20

[Steve Fink [:sfink] [:s:]]

Attachment: Use reader/write reentrancy checks in HashTable.h to allow signal handlers to do concurrent reads

This has Waldo's and Luke's updates, but one followup question -- should the ReentrancyGuard/ReentrancyGuardRW classes be defined at all in non-DEBUG builds? It looks like I'd just need to modify REENTRANCY_GUARD_ET_AL to make that work.

Comment 21

[Luke Wagner [:luke]]

Comment on attachment 593229 [details] [diff] [review]
Use reader/write reentrancy checks in HashTable.h to allow signal handlers to do concurrent reads

Ah, you're right.  What you've done looks good.

>+class ReentrancyGuardRW
>+{
>+public:

Two space indent here and with class ReentrancyeGuard above.

Comment 22

[Luke Wagner [:luke]]

Comment on attachment 589719 [details] [diff] [review]
Patch 2: Implement some algorithms that operate on collections

Review of attachment 589719 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/Algorithm.h
@@ +20,5 @@
> + * Name intentionally chosen to be revolting enough to :luke that he'll tell me
> + * a better name. Or a better way.
> + */
> +template <class Iterator, class Predicate>
> +Iterator halfstable_partition(Iterator begin, Iterator end, Predicate predicate)

This is basically std::remove_if (http://www.sgi.com/tech/stl/remove_if.html).  So, nice design choice :)  Since remove_if doesn't throw (if the underlying element 
operations don't throw), I see no problem in straight-up using std::remove_if.  It will call std::swap() so no need to define swap() above.

Lastly, as you pointed out, there is an unpleasant dependency on the container type essentially being vector.  The STL solves this by just having callers write:

  v.erase(remove_if(v.begin(), v.end(), p), v.end())

or making their own trivial wrapper for this.  I suggest you do the same (not in Algorithm.h, but in whatever local file uses it.  If it is widespread, seems like we could add to jsutility.h.

As for Invert, I don't think that's a great idea.  Instead you can reuse std::unary_negate (with helper std::not1: http://www.cplusplus.com/reference/std/functional/not1/)

With those in place, I don't think this header is needed.

Comment 23

[Steve Fink [:sfink] [:s:]]

(In reply to Luke Wagner [:luke] from comment #22)
> Comment on attachment 589719 [details] [diff] [review]
> Patch 2: Implement some algorithms that operate on collections
> 
> Review of attachment 589719 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/public/Algorithm.h
> @@ +20,5 @@
> > + * Name intentionally chosen to be revolting enough to :luke that he'll tell me
> > + * a better name. Or a better way.
> > + */
> > +template <class Iterator, class Predicate>
> > +Iterator halfstable_partition(Iterator begin, Iterator end, Predicate predicate)
> 
> This is basically std::remove_if
> (http://www.sgi.com/tech/stl/remove_if.html).  So, nice design choice :) 
> Since remove_if doesn't throw (if the underlying element 
> operations don't throw), I see no problem in straight-up using
> std::remove_if.  It will call std::swap() so no need to define swap() above.

Oh. Sure enough, remove_if *is* very similar.

It's not quite the same, though -- remove_if isn't a partitioning, in that it doesn't promise anything about the values of the remaining elements. In the example implementation at http://www.cplusplus.com/reference/algorithm/remove_if/ , the remaining elements may contain duplicates of the matching elements, and some of the non-matching elements may be clobbered entirely. I was using halfstable_partition like this:

  Iterator deadStuff = halfstable_partition(...);
  for (Iterator p = deadStuff; p != container.end(); ++p) {
    ...record information about the dead stuff...
  }
  container.resize(deadStuff - container.begin());

so I actually need to know what I'm murdering (but the order that I process the soon-to-be-dead elements doesn't matter.)

But now that I'm writing this, it is obvious that I could just do two passes over the array instead -- one to find the non-matching (in proper order, even!), and then a remove_if pass. So never mind.

> As for Invert, I don't think that's a great idea.  Instead you can reuse
> std::unary_negate (with helper std::not1:
> http://www.cplusplus.com/reference/std/functional/not1/)

Ah! I've never used any of the functional things. Or if I did, I used the boost versions. Thanks.

> With those in place, I don't think this header is needed.

Trust me, I have no regrets about shooting this file in the head.

Thanks!

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 589723 [details] [diff] [review]
Patch 3: implement an --enable-libunwind configure option

Review of attachment 589723 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the understanding that this is a temporary configure option until libunwind is installed on the build machines.

Comment 25

[Steve Fink [:sfink] [:s:]]

Done elsewhere. I think? Mostly?