Link stack frames of JNI stubs to JNI callbacks.

Link stack frames of JNI stubs to JNI callbacks.

Chrome's native heap profiler relies on stack frame pointers to unwind. That
is fast, but requires that all code in a call chain is built with frame pointer
support. Which is not the case for Java / JNI code on Android, so unwinding
stops prematurely once it crosses JNI boundary.

This CL exploits the fact that both JNI stubs (which call into Java) and JNI
callbacks (which are called by Java) are generated. It changes JNI generator
to save stack frame pointer in a JNI stub, and later link saved frame pointer
to a frame pointer of a JNI callback. That repairs broken stack frame chain
and allows unwinder to yield complete trace.

Changes in this CL are active only for profiling builds (ones that build with
enable_profiling=true). Otherwise JNI generator produces exactly the same code
as before (modulo removed empty lines).

Example of a complete trace made possible by this CL:

<.../base.odex>
Java_org_chromium_base_SystemMessageHandler_nativeDoRunLoopOnce
base::MessageLoop::DoWork
base::MessageLoop::DeferOrRunPendingTask
base::MessageLoop::RunTask
base::debug::TaskAnnotator::RunTask
cc::SingleThreadProxy::BeginMainFrame
cc::SingleThreadProxy::DoBeginMainFrame
content::CompositorImpl::UpdateLayerTreeHost..............................#4
<.../base.odex>
Java_org_chromium_chrome_..._ToolbarSceneLayer_nativeUpdateToolbarLayer...#3
android::ToolbarSceneLayer::UpdateToolbarLayer
android::ToolbarLayer::PushResource
ui::ResourceManagerImpl::GetResource......................................#2
<.../base.odex>
Java_org_chromium_ui_resources_ResourceManager_nativeOnResourceReady......#1
ui::ResourceManagerImpl::OnResourceReady
gfx::CreateSkBitmapFromJavaBitmap
...more frames from Chrome...

Previously unwinding would've stopped at #1. Code added by JNI generator links
frames #1->#2 and #3->#4.

BUG=624362
Committed: https://crrev.com/a8c951effbf899ae4951c67d606a634905f88ab4
Cr-Commit-Position: refs/heads/master@{#427534}

[Dmitry Skiba, 2016-09-23 05:48:41]

Description was changed from

==========
Link stack frames of JNI stubs to JNI callbacks.

Chrome's native heap profiler relies on stack frame pointers to unwind. That
is fast, but requires that all code in a call chain is built with frame pointer
support. Which is not the case for Java / JNI code on Android, so unwinding
stops prematurely once it crosses JNI boundary.

This CL exploits the fact that both JNI stubs (which call into Java) and JNI
callbacks (which are called by Java) are generated. It changes JNI generator
to save stack frame pointer in a JNI stub, and later link saved frame pointer
to a frame pointer of a JNI callback. That repairs broken stack frame chain
and allows unwinding to yield complete trace. In addition JNI generator adds
NOINLINE annotations to stubs to make sure they appear in traces.

The change in this CL is activated only for profiling builds (ones that build
with enable_profiling=true). Otherwise JNI generator produces exactly the same
code as before.

Example of a complete trace made possible by this CL:

<.../base.odex>
Java_org_chromium_base_SystemMessageHandler_nativeDoRunLoopOnce
base::MessageLoop
base::debug::TaskAnnotator::RunTask
cc::SingleThreadProxy::BeginMainFrame
cc::SingleThreadProxy::DoBeginMainFrame
cc::LayerTreeHost::UpdateLayers
cc::Scheduler::NotifyReadyToCommit
content::CompositorImpl::UpdateLayerTreeHost
chrome::android::CompositorView::UpdateLayerTreeHost
Java_CompositorView_onCompositorLayout....................................#4
<.../base.odex>
Java_org_chromium_chrome_..._ToolbarSceneLayer_nativeUpdateToolbarLayer...#3
chrome::android::ToolbarSceneLayer::UpdateToolbarLayer
chrome::android::ToolbarLayer::Create
chrome::android::ToolbarLayer::PushResource
ui::ResourceManagerImpl::GetResource
ui::ResourceManagerImpl::RequestResourceFromJava
Java_ResourceManager_resourceRequested....................................#2
<.../base.odex>
Java_org_chromium_ui_resources_ResourceManager_nativeOnResourceReady......#1
ui::ResourceManagerImpl::OnResourceReady
gfx::CreateSkBitmapFromJavaBitmap
...more frames from Chrome...
malloc

Previously unwinding would've stopped at #1. Code added by JNI generator links
frames #1->#2 and #3->#4. Frames #4 and #2 are visible because of NOINLINE
annotation.

BUG=624362
==========

to

==========
Link stack frames of JNI stubs to JNI callbacks.

Chrome's native heap profiler relies on stack frame pointers to unwind. That
is fast, but requires that all code in a call chain is built with frame pointer
support. Which is not the case for Java / JNI code on Android, so unwinding
stops prematurely once it crosses JNI boundary.

This CL exploits the fact that both JNI stubs (which call into Java) and JNI
callbacks (which are called by Java) are generated. It changes JNI generator
to save stack frame pointer in a JNI stub, and later link saved frame pointer
to a frame pointer of a JNI callback. That repairs broken stack frame chain
and allows unwinding to yield complete trace. In addition JNI generator adds
NOINLINE annotations to stubs to make sure they appear in traces.

Changes in this CL are active only for profiling builds (ones that build with
enable_profiling=true). Otherwise JNI generator produces exactly the same code
as before.

Example of a complete trace made possible by this CL:

<.../base.odex>
Java_org_chromium_base_SystemMessageHandler_nativeDoRunLoopOnce
base::MessageLoop
base::debug::TaskAnnotator::RunTask
cc::SingleThreadProxy::BeginMainFrame
cc::SingleThreadProxy::DoBeginMainFrame
cc::LayerTreeHost::UpdateLayers
cc::Scheduler::NotifyReadyToCommit
content::CompositorImpl::UpdateLayerTreeHost
chrome::android::CompositorView::UpdateLayerTreeHost
Java_CompositorView_onCompositorLayout....................................#4
<.../base.odex>
Java_org_chromium_chrome_..._ToolbarSceneLayer_nativeUpdateToolbarLayer...#3
chrome::android::ToolbarSceneLayer::UpdateToolbarLayer
chrome::android::ToolbarLayer::Create
chrome::android::ToolbarLayer::PushResource
ui::ResourceManagerImpl::GetResource
ui::ResourceManagerImpl::RequestResourceFromJava
Java_ResourceManager_resourceRequested....................................#2
<.../base.odex>
Java_org_chromium_ui_resources_ResourceManager_nativeOnResourceReady......#1
ui::ResourceManagerImpl::OnResourceReady
gfx::CreateSkBitmapFromJavaBitmap
...more frames from Chrome...
malloc

Previously unwinding would've stopped at #1. Code added by JNI generator links
frames #1->#2 and #3->#4. Frames #4 and #2 are visible because of NOINLINE
annotation.

BUG=624362
==========

[Dmitry Skiba, 2016-09-23 06:04:52]

dskiba@google.com changed reviewers:
+ primiano@chromium.org, thakis@chromium.org

[Dmitry Skiba, 2016-09-26 19:14:06]

Sad, but LinkStackFrames overwriting stack is too clever, because per ARM ABI
r11 (fp) should be preserved, and apparently stack frame is used as storage for
parent's fp. So when Java calls
Java_org_chromium_ui_resources_ResourceManager_nativeOnResourceReady() with a
value of fp, it expects us to preserve it. But LinkStackFrames() overwrites
saved fp's value on the stack, which is then used to restore fp's original
value:

00419e98 <Java_org_chromium_ui_resources_ResourceManager_nativeOnResourceReady>:
  419e98:	e92d4870 	push	{r4, r5, r6, fp, lr}
  419e9c:	e1a05002 	mov	r5, r2
  419ea0:	e1a04000 	mov	r4, r0
  419ea4:	e1a06001 	mov	r6, r1
  419ea8:	e28db010 	add	fp, sp, #16
  419eac:	e24dd034 	sub	sp, sp, #52	; 0x34
  419eb0:	e1a0000b 	mov	r0, fp
  419eb4:	eb1a6eee 	bl	ab5a74 <LinkSavedJNIStackFrame>
  *** [fp] is rewritten by LinkStackFrames() ***
  ...
  419f28:	e24bd010 	sub	sp, fp, #16
  419f2c:	e8bd8870 	pop	{r4, r5, r6, fp, pc}
  *** fp is restored from location we overwrote ***


I have two other solutions to try, stay tuned.

[Primiano Tucci (use gerrit), 2016-09-26 19:33:08]

yeah that's precisely what I meant in our offline chat when I asked how
weren't you clobber in the fp register.
try something less clever. I love boring and predictable code :)
P. S. thanks for looking into this.

On Mon, Sep 26, 2016, 20:14 <dskiba@google.com> wrote:

> Sad, but LinkStackFrames overwriting stack is too clever, because per ARM
> ABI
> r11 (fp) should be preserved, and apparently stack frame is used as
> storage for
> parent's fp. So when Java calls
> Java_org_chromium_ui_resources_ResourceManager_nativeOnResourceReady()
> with a
> value of fp, it expects us to preserve it. But LinkStackFrames() overwrites
> saved fp's value on the stack, which is then used to restore fp's original
> value:
>
> 00419e98
> <Java_org_chromium_ui_resources_ResourceManager_nativeOnResourceReady>:
> 419e98: e92d4870 push {r4, r5, r6, fp, lr}
> 419e9c: e1a05002 mov r5, r2
> 419ea0: e1a04000 mov r4, r0
> 419ea4: e1a06001 mov r6, r1
> 419ea8: e28db010 add fp, sp, #16
> 419eac: e24dd034 sub sp, sp, #52 ; 0x34
> 419eb0: e1a0000b mov r0, fp
> 419eb4: eb1a6eee bl ab5a74 <LinkSavedJNIStackFrame>
> *** [fp] is rewritten by LinkStackFrames() ***
> ...
> 419f28: e24bd010 sub sp, fp, #16
> 419f2c: e8bd8870 pop {r4, r5, r6, fp, pc}
> *** fp is restored from location we overwrote ***
>
>
> I have two other solutions to try, stay tuned.
>
> https://codereview.chromium.org/2361353002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dmitry Skiba, 2016-10-03 21:34:37]

Fixed the issue with corrupting fp register by restoring its value before
returning. I.e. instead of LinkStackFrames() function we now have
ScopedStackFrameLinker, which saves original value of stack frame's fp and
restores it before returning from the enclosing function.  With that disassembly
now looks like this:

004a10a0 <Java_org_chromium_ui_resources_ResourceManager_nativeOnResourceReady>:
  4a10a0:	e92d48f0 	push	{r4, r5, r6, r7, fp, lr}
  4a10a4:	e1a06002 	mov	r6, r2
  4a10a8:	e1a05000 	mov	r5, r0
  4a10ac:	e1a07001 	mov	r7, r1
  4a10b0:	e28db014 	add	fp, sp, #20
  4a10b4:	e24b401c 	sub	r4, fp, #28
  4a10b8:	e24dd038 	sub	sp, sp, #56	; 0x38
  4a10bc:	e1a0100b 	mov	r1, fp
  4a10c0:	e1a00004 	mov	r0, r4
  4a10c4:	eb1e177b 	bl	c26eb8 <JNIStackFrameLinker::JNIStackFrameLinker>
  ...
  4a1138:	e1a00004 	mov	r0, r4
  4a113c:	eb1e2a11 	bl	c2b988 <ScopedStackFrameLinker::~ScopedStackFrameLinker>
  4a1140:	e24bd014 	sub	sp, fp, #20
  4a1144:	e8bd88f0 	pop	{r4, r5, r6, r7, fp, pc}

[Dmitry Skiba, 2016-10-06 15:49:16]

Ping.

BTW, one advantage of doing it this way is that other tools that unwind using
frame pointers (like asan) will also be able to get complete trace.

[Dmitry Skiba, 2016-10-12 05:22:33]

Primiano raised concern that this change has too many layers and is hard to
understand. So I removed FakeStackFrame, JNI_PROFILING_SAVE_STACK_FP and
NOINLINE (from JNI generator). That simplified things, but also caused marked
frames to disappear from the trace:

<.../base.odex>
Java_org_chromium_base_SystemMessageHandler_nativeDoRunLoopOnce
base::MessageLoop::DoWork
base::MessageLoop::DeferOrRunPendingTask
base::MessageLoop::RunTask
base::debug::TaskAnnotator::RunTask
cc::SingleThreadProxy::BeginMainFrame
cc::SingleThreadProxy::DoBeginMainFrame
content::CompositorImpl::UpdateLayerTreeHost
android::CompositorView::UpdateLayerTreeHost <----------------------
Java_CompositorView_onCompositorLayout <----------------------------
<.../base.odex>
Java_org_chromium_..._ToolbarSceneLayer_nativeUpdateToolbarLayer
android::ToolbarSceneLayer::UpdateToolbarLayer
android::ToolbarLayer::PushResource
ui::ResourceManagerImpl::GetResource
ui::ResourceManagerImpl::RequestResourceFromJava <------------------
Java_ResourceManager_resourceRequested <----------------------------
<.../base.odex>
Java_org_chromium_ui_resources_ResourceManager_nativeOnResourceReady
ui::ResourceManagerImpl::OnResourceReady
gfx::CreateSkBitmapFromJavaBitmap

[Dmitry Skiba, 2016-10-12 05:24:55]

Description was changed from

==========
Link stack frames of JNI stubs to JNI callbacks.

Chrome's native heap profiler relies on stack frame pointers to unwind. That
is fast, but requires that all code in a call chain is built with frame pointer
support. Which is not the case for Java / JNI code on Android, so unwinding
stops prematurely once it crosses JNI boundary.

This CL exploits the fact that both JNI stubs (which call into Java) and JNI
callbacks (which are called by Java) are generated. It changes JNI generator
to save stack frame pointer in a JNI stub, and later link saved frame pointer
to a frame pointer of a JNI callback. That repairs broken stack frame chain
and allows unwinding to yield complete trace. In addition JNI generator adds
NOINLINE annotations to stubs to make sure they appear in traces.

Changes in this CL are active only for profiling builds (ones that build with
enable_profiling=true). Otherwise JNI generator produces exactly the same code
as before.

Example of a complete trace made possible by this CL:

<.../base.odex>
Java_org_chromium_base_SystemMessageHandler_nativeDoRunLoopOnce
base::MessageLoop
base::debug::TaskAnnotator::RunTask
cc::SingleThreadProxy::BeginMainFrame
cc::SingleThreadProxy::DoBeginMainFrame
cc::LayerTreeHost::UpdateLayers
cc::Scheduler::NotifyReadyToCommit
content::CompositorImpl::UpdateLayerTreeHost
chrome::android::CompositorView::UpdateLayerTreeHost
Java_CompositorView_onCompositorLayout....................................#4
<.../base.odex>
Java_org_chromium_chrome_..._ToolbarSceneLayer_nativeUpdateToolbarLayer...#3
chrome::android::ToolbarSceneLayer::UpdateToolbarLayer
chrome::android::ToolbarLayer::Create
chrome::android::ToolbarLayer::PushResource
ui::ResourceManagerImpl::GetResource
ui::ResourceManagerImpl::RequestResourceFromJava
Java_ResourceManager_resourceRequested....................................#2
<.../base.odex>
Java_org_chromium_ui_resources_ResourceManager_nativeOnResourceReady......#1
ui::ResourceManagerImpl::OnResourceReady
gfx::CreateSkBitmapFromJavaBitmap
...more frames from Chrome...
malloc

Previously unwinding would've stopped at #1. Code added by JNI generator links
frames #1->#2 and #3->#4. Frames #4 and #2 are visible because of NOINLINE
annotation.

BUG=624362
==========

to

==========
Link stack frames of JNI stubs to JNI callbacks.

Chrome's native heap profiler relies on stack frame pointers to unwind. That
is fast, but requires that all code in a call chain is built with frame pointer
support. Which is not the case for Java / JNI code on Android, so unwinding
stops prematurely once it crosses JNI boundary.

This CL exploits the fact that both JNI stubs (which call into Java) and JNI
callbacks (which are called by Java) are generated. It changes JNI generator
to save stack frame pointer in a JNI stub, and later link saved frame pointer
to a frame pointer of a JNI callback. That repairs broken stack frame chain
and allows unwinding to yield complete trace.

Changes in this CL are active only for profiling builds (ones that build with
enable_profiling=true). Otherwise JNI generator produces exactly the same code
as before.

Example of a complete trace made possible by this CL:

<.../base.odex>
Java_org_chromium_base_SystemMessageHandler_nativeDoRunLoopOnce
base::MessageLoop::DoWork
base::MessageLoop::DeferOrRunPendingTask
base::MessageLoop::RunTask
base::debug::TaskAnnotator::RunTask
cc::SingleThreadProxy::BeginMainFrame
cc::SingleThreadProxy::DoBeginMainFrame
content::CompositorImpl::UpdateLayerTreeHost..............................#4
<.../base.odex>
Java_org_chromium_chrome_..._ToolbarSceneLayer_nativeUpdateToolbarLayer...#3
android::ToolbarSceneLayer::UpdateToolbarLayer
android::ToolbarLayer::PushResource
ui::ResourceManagerImpl::GetResource......................................#2
<.../base.odex>
Java_org_chromium_ui_resources_ResourceManager_nativeOnResourceReady......#1
ui::ResourceManagerImpl::OnResourceReady
gfx::CreateSkBitmapFromJavaBitmap
...more frames from Chrome...

Previously unwinding would've stopped at #1. Code added by JNI generator links
frames #1->#2 and #3->#4.

BUG=624362
==========

[Primiano Tucci (use gerrit), 2016-10-13 20:03:22]

Ok, this is extremely clever yet extremely frightening.
The thing that makes me feel good is that all this happens only when
enable_profiling=1.

P.S. I assume you only tested on arm (which is fine). Should we just make all
this disabled on other archs? or is the HAVE_TRACE_STACK_FRAME_POINTERS already
taking care of this?

LGTM, Now I will run to a pub and drink until I will have forgotten to have
reviewed this.
Joking aside, technically speaking this is really clever... probably even a tid
too much.

Welcome in the stack unwinder club. The first rule of the stack unwinder club
is: you never talk about the stack unwinder club (or people will cc you to all
sort of crash reports, trust me) :P

https://codereview.chromium.org/2361353002/diff/60001/base/android/jni_android.h
File base/android/jni_android.h (right):

https://codereview.chromium.org/2361353002/diff/60001/base/android/jni_androi...
base/android/jni_android.h:28: #define JNI_PROFILING_LEAVING_NATIVE \
I'd just call these JNI_SAVE_FRAME_POINTER and JNI_LINK_SAVED_FRAME_POINTER to
remove this naming-layer-hopping you force readers to go to.
Concretely, right now when I look at the JNI generator and I see LEAVING_NATIVE
I have to remember that this is  the place where you save and the other is where
you restore.
In few words: don't generalize concepts unless you really need to. Direct names
are way easier to follow.

https://codereview.chromium.org/2361353002/diff/60001/base/android/jni_androi...
base/android/jni_android.h:168: static void* SavedFrame();
s/SavedFrame/GetLastFrameSavedForCurrentThread()/

https://codereview.chromium.org/2361353002/diff/60001/base/android/jni_androi...
base/android/jni_android.h:169: private:
micro-nit: add \n after private:

https://codereview.chromium.org/2361353002/diff/60001/base/android/jni_androi...
base/android/jni_android.h:171: DISALLOW_COPY_AND_ASSIGN(JNIStackFrameSaver);
micro-nit: add extra \n before DISALLOW

https://codereview.chromium.org/2361353002/diff/60001/base/debug/stack_trace.cc
File base/debug/stack_trace.cc (right):

https://codereview.chromium.org/2361353002/diff/60001/base/debug/stack_trace....
base/debug/stack_trace.cc:179: // Links stack frame |fp| to |parent_fp|, so that
during stack unwinding
I think you commented this already in the .h. Usually that is fine.

https://codereview.chromium.org/2361353002/diff/60001/base/debug/stack_trace....
base/debug/stack_trace.cc:263: LinkStackFrames(fp_, original_parent_fp_);
could you add a CHECK here that verifies that the content of the fp saved on the
stack still matches what you did set in the ctor, before resetting?
If something goes unexpectedly wrong at least we'd fail very fast showing where
the problem is.
I'd make this a CHECK and not a DCHECK beause almost nobody uses debug on
android builds.

https://codereview.chromium.org/2361353002/diff/60001/base/debug/stack_trace.h
File base/debug/stack_trace.h (right):

https://codereview.chromium.org/2361353002/diff/60001/base/debug/stack_trace....
base/debug/stack_trace.h:120: // Both frame pointers must come from
__builtin_frame_address().
+0 (i.e. __builtin_frame_address(0))

https://codereview.chromium.org/2361353002/diff/60001/base/debug/stack_trace....
base/debug/stack_trace.h:148: // JVM doesn't use frame pointers, so when
JavaCallback() is called back by
Honestly I think that these lines of comment (148-154) belong to jni_android

https://codereview.chromium.org/2361353002/diff/60001/base/debug/stack_trace....
base/debug/stack_trace.h:159: private:
nit \n here and below (before disallow)

[Dmitry Skiba, 2016-10-17 22:09:12]

> P.S. I assume you only tested on arm (which is fine). Should we just make all
> this disabled on other archs? or is the HAVE_TRACE_STACK_FRAME_POINTERS
already
> taking care of this?

Yes, I developed this on ARM, but there is no reason for this not to work on x86
(or on any architecture that HAVE_TRACE_STACK_FRAME_POINTERS supports), in fact
I've just tested it on x86 Android and it worked fine.

https://codereview.chromium.org/2361353002/diff/60001/base/android/jni_android.h
File base/android/jni_android.h (right):

https://codereview.chromium.org/2361353002/diff/60001/base/android/jni_androi...
base/android/jni_android.h:28: #define JNI_PROFILING_LEAVING_NATIVE \
On 2016/10/13 20:03:21, Primiano Tucci wrote:
> I'd just call these JNI_SAVE_FRAME_POINTER and JNI_LINK_SAVED_FRAME_POINTER to
> remove this naming-layer-hopping you force readers to go to.
> Concretely, right now when I look at the JNI generator and I see
LEAVING_NATIVE
> I have to remember that this is  the place where you save and the other is
where
> you restore.
> In few words: don't generalize concepts unless you really need to. Direct
names
> are way easier to follow.

Done.

https://codereview.chromium.org/2361353002/diff/60001/base/android/jni_androi...
base/android/jni_android.h:168: static void* SavedFrame();
On 2016/10/13 20:03:21, Primiano Tucci wrote:
> s/SavedFrame/GetLastFrameSavedForCurrentThread()/

This feels unnecessarily long. I think we should either rename the class to
something like ScopedJNIPerThreadStackFrameSaver and leave SavedFrame() as is,
or just leave all those details to the class comment, as they are now.

https://codereview.chromium.org/2361353002/diff/60001/base/android/jni_androi...
base/android/jni_android.h:169: private:
On 2016/10/13 20:03:21, Primiano Tucci wrote:
> micro-nit: add \n after private:

Done.

https://codereview.chromium.org/2361353002/diff/60001/base/android/jni_androi...
base/android/jni_android.h:171: DISALLOW_COPY_AND_ASSIGN(JNIStackFrameSaver);
On 2016/10/13 20:03:21, Primiano Tucci wrote:
> micro-nit: add extra \n before DISALLOW

Done.

https://codereview.chromium.org/2361353002/diff/60001/base/debug/stack_trace.cc
File base/debug/stack_trace.cc (right):

https://codereview.chromium.org/2361353002/diff/60001/base/debug/stack_trace....
base/debug/stack_trace.cc:179: // Links stack frame |fp| to |parent_fp|, so that
during stack unwinding
On 2016/10/13 20:03:21, Primiano Tucci wrote:
> I think you commented this already in the .h. Usually that is fine.

That was for ScopedStackFrameLinker thing, and I remember you asked earlier what
fp and parent_fp arguments are, so I decided to put comment here too.

https://codereview.chromium.org/2361353002/diff/60001/base/debug/stack_trace....
base/debug/stack_trace.cc:263: LinkStackFrames(fp_, original_parent_fp_);
On 2016/10/13 20:03:21, Primiano Tucci wrote:
> could you add a CHECK here that verifies that the content of the fp saved on
the
> stack still matches what you did set in the ctor, before resetting?
> If something goes unexpectedly wrong at least we'd fail very fast showing
where
> the problem is.
> I'd make this a CHECK and not a DCHECK beause almost nobody uses debug on
> android builds.

Done.

https://codereview.chromium.org/2361353002/diff/60001/base/debug/stack_trace.h
File base/debug/stack_trace.h (right):

https://codereview.chromium.org/2361353002/diff/60001/base/debug/stack_trace....
base/debug/stack_trace.h:120: // Both frame pointers must come from
__builtin_frame_address().
On 2016/10/13 20:03:21, Primiano Tucci wrote:
> +0 (i.e. __builtin_frame_address(0))

Not necessarily, if __builtin_frame_address(7) works on your platform, you can
as well use it.

https://codereview.chromium.org/2361353002/diff/60001/base/debug/stack_trace....
base/debug/stack_trace.h:148: // JVM doesn't use frame pointers, so when
JavaCallback() is called back by
On 2016/10/13 20:03:21, Primiano Tucci wrote:
> Honestly I think that these lines of comment (148-154) belong to jni_android

The fact that JNI is used in explanation how ScopedStackFrameLinker works is
just a coincidence, I'll reword it to use 'Some Library' instead.

https://codereview.chromium.org/2361353002/diff/60001/base/debug/stack_trace....
base/debug/stack_trace.h:159: private:
On 2016/10/13 20:03:21, Primiano Tucci wrote:
> nit \n here and below (before disallow)

Done.

[Dmitry Skiba, 2016-10-17 22:22:35]

Description was changed from

==========
Link stack frames of JNI stubs to JNI callbacks.

Chrome's native heap profiler relies on stack frame pointers to unwind. That
is fast, but requires that all code in a call chain is built with frame pointer
support. Which is not the case for Java / JNI code on Android, so unwinding
stops prematurely once it crosses JNI boundary.

This CL exploits the fact that both JNI stubs (which call into Java) and JNI
callbacks (which are called by Java) are generated. It changes JNI generator
to save stack frame pointer in a JNI stub, and later link saved frame pointer
to a frame pointer of a JNI callback. That repairs broken stack frame chain
and allows unwinding to yield complete trace.

Changes in this CL are active only for profiling builds (ones that build with
enable_profiling=true). Otherwise JNI generator produces exactly the same code
as before.

Example of a complete trace made possible by this CL:

<.../base.odex>
Java_org_chromium_base_SystemMessageHandler_nativeDoRunLoopOnce
base::MessageLoop::DoWork
base::MessageLoop::DeferOrRunPendingTask
base::MessageLoop::RunTask
base::debug::TaskAnnotator::RunTask
cc::SingleThreadProxy::BeginMainFrame
cc::SingleThreadProxy::DoBeginMainFrame
content::CompositorImpl::UpdateLayerTreeHost..............................#4
<.../base.odex>
Java_org_chromium_chrome_..._ToolbarSceneLayer_nativeUpdateToolbarLayer...#3
android::ToolbarSceneLayer::UpdateToolbarLayer
android::ToolbarLayer::PushResource
ui::ResourceManagerImpl::GetResource......................................#2
<.../base.odex>
Java_org_chromium_ui_resources_ResourceManager_nativeOnResourceReady......#1
ui::ResourceManagerImpl::OnResourceReady
gfx::CreateSkBitmapFromJavaBitmap
...more frames from Chrome...

Previously unwinding would've stopped at #1. Code added by JNI generator links
frames #1->#2 and #3->#4.

BUG=624362
==========

to

==========
Link stack frames of JNI stubs to JNI callbacks.

Chrome's native heap profiler relies on stack frame pointers to unwind. That
is fast, but requires that all code in a call chain is built with frame pointer
support. Which is not the case for Java / JNI code on Android, so unwinding
stops prematurely once it crosses JNI boundary.

This CL exploits the fact that both JNI stubs (which call into Java) and JNI
callbacks (which are called by Java) are generated. It changes JNI generator
to save stack frame pointer in a JNI stub, and later link saved frame pointer
to a frame pointer of a JNI callback. That repairs broken stack frame chain
and allows unwinder to yield complete trace.

Changes in this CL are active only for profiling builds (ones that build with
enable_profiling=true). Otherwise JNI generator produces exactly the same code
as before (modulo removed empty lines).

Example of a complete trace made possible by this CL:

<.../base.odex>
Java_org_chromium_base_SystemMessageHandler_nativeDoRunLoopOnce
base::MessageLoop::DoWork
base::MessageLoop::DeferOrRunPendingTask
base::MessageLoop::RunTask
base::debug::TaskAnnotator::RunTask
cc::SingleThreadProxy::BeginMainFrame
cc::SingleThreadProxy::DoBeginMainFrame
content::CompositorImpl::UpdateLayerTreeHost..............................#4
<.../base.odex>
Java_org_chromium_chrome_..._ToolbarSceneLayer_nativeUpdateToolbarLayer...#3
android::ToolbarSceneLayer::UpdateToolbarLayer
android::ToolbarLayer::PushResource
ui::ResourceManagerImpl::GetResource......................................#2
<.../base.odex>
Java_org_chromium_ui_resources_ResourceManager_nativeOnResourceReady......#1
ui::ResourceManagerImpl::OnResourceReady
gfx::CreateSkBitmapFromJavaBitmap
...more frames from Chrome...

Previously unwinding would've stopped at #1. Code added by JNI generator links
frames #1->#2 and #3->#4.

BUG=624362
==========

[Dmitry Skiba, 2016-10-17 22:29:26]

The CQ bit was checked by dskiba@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 22:30:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 01:43:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 01:43:07]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Nico, 2016-10-25 19:36:33]

lgtm, thanks for the good CL description.

https://codereview.chromium.org/2361353002/diff/120001/base/debug/stack_trace.cc
File base/debug/stack_trace.cc (right):

https://codereview.chromium.org/2361353002/diff/120001/base/debug/stack_trace...
base/debug/stack_trace.cc:262: , original_parent_fp_(LinkStackFrames(fp,
parent_fp)) {}
please run `git cl format`

[Dmitry Skiba, 2016-10-25 21:29:16]

The CQ bit was checked by dskiba@google.com

[Dmitry Skiba, 2016-10-25 21:29:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2361353002/#ps160001
(title: "git cl format")

[commit-bot: I haz the power, 2016-10-25 21:30:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 23:39:39]

Description was changed from

==========
Link stack frames of JNI stubs to JNI callbacks.

Chrome's native heap profiler relies on stack frame pointers to unwind. That
is fast, but requires that all code in a call chain is built with frame pointer
support. Which is not the case for Java / JNI code on Android, so unwinding
stops prematurely once it crosses JNI boundary.

This CL exploits the fact that both JNI stubs (which call into Java) and JNI
callbacks (which are called by Java) are generated. It changes JNI generator
to save stack frame pointer in a JNI stub, and later link saved frame pointer
to a frame pointer of a JNI callback. That repairs broken stack frame chain
and allows unwinder to yield complete trace.

Changes in this CL are active only for profiling builds (ones that build with
enable_profiling=true). Otherwise JNI generator produces exactly the same code
as before (modulo removed empty lines).

Example of a complete trace made possible by this CL:

<.../base.odex>
Java_org_chromium_base_SystemMessageHandler_nativeDoRunLoopOnce
base::MessageLoop::DoWork
base::MessageLoop::DeferOrRunPendingTask
base::MessageLoop::RunTask
base::debug::TaskAnnotator::RunTask
cc::SingleThreadProxy::BeginMainFrame
cc::SingleThreadProxy::DoBeginMainFrame
content::CompositorImpl::UpdateLayerTreeHost..............................#4
<.../base.odex>
Java_org_chromium_chrome_..._ToolbarSceneLayer_nativeUpdateToolbarLayer...#3
android::ToolbarSceneLayer::UpdateToolbarLayer
android::ToolbarLayer::PushResource
ui::ResourceManagerImpl::GetResource......................................#2
<.../base.odex>
Java_org_chromium_ui_resources_ResourceManager_nativeOnResourceReady......#1
ui::ResourceManagerImpl::OnResourceReady
gfx::CreateSkBitmapFromJavaBitmap
...more frames from Chrome...

Previously unwinding would've stopped at #1. Code added by JNI generator links
frames #1->#2 and #3->#4.

BUG=624362
==========

to

==========
Link stack frames of JNI stubs to JNI callbacks.

Chrome's native heap profiler relies on stack frame pointers to unwind. That
is fast, but requires that all code in a call chain is built with frame pointer
support. Which is not the case for Java / JNI code on Android, so unwinding
stops prematurely once it crosses JNI boundary.

This CL exploits the fact that both JNI stubs (which call into Java) and JNI
callbacks (which are called by Java) are generated. It changes JNI generator
to save stack frame pointer in a JNI stub, and later link saved frame pointer
to a frame pointer of a JNI callback. That repairs broken stack frame chain
and allows unwinder to yield complete trace.

Changes in this CL are active only for profiling builds (ones that build with
enable_profiling=true). Otherwise JNI generator produces exactly the same code
as before (modulo removed empty lines).

Example of a complete trace made possible by this CL:

<.../base.odex>
Java_org_chromium_base_SystemMessageHandler_nativeDoRunLoopOnce
base::MessageLoop::DoWork
base::MessageLoop::DeferOrRunPendingTask
base::MessageLoop::RunTask
base::debug::TaskAnnotator::RunTask
cc::SingleThreadProxy::BeginMainFrame
cc::SingleThreadProxy::DoBeginMainFrame
content::CompositorImpl::UpdateLayerTreeHost..............................#4
<.../base.odex>
Java_org_chromium_chrome_..._ToolbarSceneLayer_nativeUpdateToolbarLayer...#3
android::ToolbarSceneLayer::UpdateToolbarLayer
android::ToolbarLayer::PushResource
ui::ResourceManagerImpl::GetResource......................................#2
<.../base.odex>
Java_org_chromium_ui_resources_ResourceManager_nativeOnResourceReady......#1
ui::ResourceManagerImpl::OnResourceReady
gfx::CreateSkBitmapFromJavaBitmap
...more frames from Chrome...

Previously unwinding would've stopped at #1. Code added by JNI generator links
frames #1->#2 and #3->#4.

BUG=624362
==========

[commit-bot: I haz the power, 2016-10-25 23:39:40]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-10-25 23:41:16]

Description was changed from

==========
Link stack frames of JNI stubs to JNI callbacks.

Chrome's native heap profiler relies on stack frame pointers to unwind. That
is fast, but requires that all code in a call chain is built with frame pointer
support. Which is not the case for Java / JNI code on Android, so unwinding
stops prematurely once it crosses JNI boundary.

This CL exploits the fact that both JNI stubs (which call into Java) and JNI
callbacks (which are called by Java) are generated. It changes JNI generator
to save stack frame pointer in a JNI stub, and later link saved frame pointer
to a frame pointer of a JNI callback. That repairs broken stack frame chain
and allows unwinder to yield complete trace.

Changes in this CL are active only for profiling builds (ones that build with
enable_profiling=true). Otherwise JNI generator produces exactly the same code
as before (modulo removed empty lines).

Example of a complete trace made possible by this CL:

<.../base.odex>
Java_org_chromium_base_SystemMessageHandler_nativeDoRunLoopOnce
base::MessageLoop::DoWork
base::MessageLoop::DeferOrRunPendingTask
base::MessageLoop::RunTask
base::debug::TaskAnnotator::RunTask
cc::SingleThreadProxy::BeginMainFrame
cc::SingleThreadProxy::DoBeginMainFrame
content::CompositorImpl::UpdateLayerTreeHost..............................#4
<.../base.odex>
Java_org_chromium_chrome_..._ToolbarSceneLayer_nativeUpdateToolbarLayer...#3
android::ToolbarSceneLayer::UpdateToolbarLayer
android::ToolbarLayer::PushResource
ui::ResourceManagerImpl::GetResource......................................#2
<.../base.odex>
Java_org_chromium_ui_resources_ResourceManager_nativeOnResourceReady......#1
ui::ResourceManagerImpl::OnResourceReady
gfx::CreateSkBitmapFromJavaBitmap
...more frames from Chrome...

Previously unwinding would've stopped at #1. Code added by JNI generator links
frames #1->#2 and #3->#4.

BUG=624362
==========

to

==========
Link stack frames of JNI stubs to JNI callbacks.

Chrome's native heap profiler relies on stack frame pointers to unwind. That
is fast, but requires that all code in a call chain is built with frame pointer
support. Which is not the case for Java / JNI code on Android, so unwinding
stops prematurely once it crosses JNI boundary.

This CL exploits the fact that both JNI stubs (which call into Java) and JNI
callbacks (which are called by Java) are generated. It changes JNI generator
to save stack frame pointer in a JNI stub, and later link saved frame pointer
to a frame pointer of a JNI callback. That repairs broken stack frame chain
and allows unwinder to yield complete trace.

Changes in this CL are active only for profiling builds (ones that build with
enable_profiling=true). Otherwise JNI generator produces exactly the same code
as before (modulo removed empty lines).

Example of a complete trace made possible by this CL:

<.../base.odex>
Java_org_chromium_base_SystemMessageHandler_nativeDoRunLoopOnce
base::MessageLoop::DoWork
base::MessageLoop::DeferOrRunPendingTask
base::MessageLoop::RunTask
base::debug::TaskAnnotator::RunTask
cc::SingleThreadProxy::BeginMainFrame
cc::SingleThreadProxy::DoBeginMainFrame
content::CompositorImpl::UpdateLayerTreeHost..............................#4
<.../base.odex>
Java_org_chromium_chrome_..._ToolbarSceneLayer_nativeUpdateToolbarLayer...#3
android::ToolbarSceneLayer::UpdateToolbarLayer
android::ToolbarLayer::PushResource
ui::ResourceManagerImpl::GetResource......................................#2
<.../base.odex>
Java_org_chromium_ui_resources_ResourceManager_nativeOnResourceReady......#1
ui::ResourceManagerImpl::OnResourceReady
gfx::CreateSkBitmapFromJavaBitmap
...more frames from Chrome...

Previously unwinding would've stopped at #1. Code added by JNI generator links
frames #1->#2 and #3->#4.

BUG=624362

Committed: https://crrev.com/a8c951effbf899ae4951c67d606a634905f88ab4
Cr-Commit-Position: refs/heads/master@{#427534}
==========

[commit-bot: I haz the power, 2016-10-25 23:41:17]

Patchset 9 (id:??) landed as
https://crrev.com/a8c951effbf899ae4951c67d606a634905f88ab4
Cr-Commit-Position: refs/heads/master@{#427534}