Fix edge case where profiler would miss the caller of the top frame

Fix edge case where profiler would miss the caller of the top frame.

Some stubs (and intrinsics) do not push a frame onto the stack leaving
the frame pointer in the caller.

  PC -> STUB
  FP -> DART3  <-+
        DART2  <-|  <- TOP FRAME RETURN ADDRESS.
        DART1  <-|
        .....

In this case, traversing the linked stack frames will not collect a PC
inside DART3. The stack will incorrectly be: STUB, DART2, DART1.
In Dart code, after pushing the FP onto the stack, an IP in the current
function is pushed onto the stack as well. This stack slot is called
the PC marker. We can use the PC marker to insert DART3 into the stack
so that it will correctly be: STUB, DART3, DART2, DART1. Note the
inserted PC may not accurately reflect the true return address from STUB.

R=asiva@google.com, srdjan@google.com

Committed: https://code.google.com/p/dart/source/detail?r=35762

[srdjan, 2014-05-02 18:20:29]

lgtm

https://codereview.chromium.org/264933006/diff/60001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/264933006/diff/60001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:913: class FixTopFrameVisitor : public SampleVisitor {
Make SampleVisitor a ValueObject if possible so that it cannot be allocated but
on the stack.

https://codereview.chromium.org/264933006/diff/60001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:941: void FixMissingDartFrame(Sample* sample) {
s/FixMissingDartFrame/CheckForMissingDartFrame/

https://codereview.chromium.org/264933006/diff/60001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:996: Code& code_;
Code*. We try to use reference with const.

https://codereview.chromium.org/264933006/diff/60001/runtime/vm/profiler.h
File runtime/vm/profiler.h (right):

https://codereview.chromium.org/264933006/diff/60001/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:266: void VisitSamples(SampleVisitor* visitor) {
Remove one of VisitSamples/VisitSamplesByReference

https://codereview.chromium.org/264933006/diff/60001/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:295: sample = At(i);
You can put Sample* sample = At(i); here.
OR
const Sample& sample = *At(i)l

[Cutch, 2014-05-02 18:37:02]

https://codereview.chromium.org/264933006/diff/60001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/264933006/diff/60001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:913: class FixTopFrameVisitor : public SampleVisitor {
On 2014/05/02 18:20:29, srdjan wrote:
> Make SampleVisitor a ValueObject if possible so that it cannot be allocated
but
> on the stack.

Done.

https://codereview.chromium.org/264933006/diff/60001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:941: void FixMissingDartFrame(Sample* sample) {
On 2014/05/02 18:20:29, srdjan wrote:
> s/FixMissingDartFrame/CheckForMissingDartFrame/

Done.

https://codereview.chromium.org/264933006/diff/60001/runtime/vm/profiler.h
File runtime/vm/profiler.h (right):

https://codereview.chromium.org/264933006/diff/60001/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:266: void VisitSamples(SampleVisitor* visitor) {
On 2014/05/02 18:20:29, srdjan wrote:
> Remove one of VisitSamples/VisitSamplesByReference

Done.

https://codereview.chromium.org/264933006/diff/60001/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:295: sample = At(i);
On 2014/05/02 18:20:29, srdjan wrote:
> You can put Sample* sample = At(i); here.
> OR
> const Sample& sample = *At(i)l

Done.

[siva, 2014-05-02 20:43:36]

LGTM with some comments.

https://codereview.chromium.org/264933006/diff/60002/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/264933006/diff/60002/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:921: void VisitSample(Sample* sample) {
const

https://codereview.chromium.org/264933006/diff/60002/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:941: void CheckForMissingDartFrame(Sample* sample) {
const

https://codereview.chromium.org/264933006/diff/60002/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:964: if (code_.ContainsInstructionAt(pc_marker)) {
Should this be
if (pc_marker == 0 || code_.ContainsInstr....) {
  return;
}
to account for those stubs that have frames setup.
(It would have been filtered out by the ContainedInDartCodeHeaps checks below
but that is a more expensive check).

https://codereview.chromium.org/264933006/diff/60002/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:975: bool ContainedInDartCodeHeaps(uword pc) {
const

https://codereview.chromium.org/264933006/diff/60002/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:984: RawCode* FindCodeForPC(uword pc) {
const

https://codereview.chromium.org/264933006/diff/60002/runtime/vm/profiler.h
File runtime/vm/profiler.h (right):

https://codereview.chromium.org/264933006/diff/60002/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:180: uword sp() {
const

https://codereview.chromium.org/264933006/diff/60002/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:188: uword fp() {
const

https://codereview.chromium.org/264933006/diff/60002/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:197: // Shift all entries down.
Maybe you should add a comment here indicating what is going on and why the slot
is always 1.

https://codereview.chromium.org/264933006/diff/60002/runtime/vm/profiler.h#ne...
runtime/vm/profiler.h:222: class LeafFrameIsDart : public BitField<bool, 1, 1>
{};
it would be more readable to enumerate the bits, e.g:
  enum StateBits {
    kConstBit = 0,
    kImplementedBit = 1,
    kTypeFinalizedBit = 2,
    kClassFinalizedPos = 3,
    kClassFinalizedSize = 2,
    kAbstractBit = kClassFinalizedPos + kClassFinalizedSize,  // = 5
    kPatchBit = 6,
    kSynthesizedClassBit = 7,
    kMarkedForParsingBit = 8,
    kMixinAppAliasBit = 9,
    kMixinTypeAppliedBit = 10,
    kFieldsMarkedNullableBit = 11,
    kCycleFreeBit = 12,
  };
  class ConstBit : public BitField<bool, kConstBit, 1> {};
  class ImplementedBit : public BitField<bool, kImplementedBit, 1> {};

[Cutch, 2014-05-05 15:06:26]

PTAL, I ended up cleaning up some comments in reusable_handles.h and adding a
reusable Code handle.

https://codereview.chromium.org/264933006/diff/60002/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/264933006/diff/60002/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:921: void VisitSample(Sample* sample) {
On 2014/05/02 20:43:37, siva wrote:
> const

this isn't a const method.

https://codereview.chromium.org/264933006/diff/60002/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:941: void CheckForMissingDartFrame(Sample* sample) {
On 2014/05/02 20:43:37, siva wrote:
> const

Done.

https://codereview.chromium.org/264933006/diff/60002/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:964: if (code_.ContainsInstructionAt(pc_marker)) {
On 2014/05/02 20:43:37, siva wrote:
> Should this be
> if (pc_marker == 0 || code_.ContainsInstr....) {
>   return;
> }
> to account for those stubs that have frames setup.
> (It would have been filtered out by the ContainedInDartCodeHeaps checks below
> but that is a more expensive check).

I now check if pc_marker is 0 in the VisitSample method and immediately return.

https://codereview.chromium.org/264933006/diff/60002/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:975: bool ContainedInDartCodeHeaps(uword pc) {
On 2014/05/02 20:43:37, siva wrote:
> const

Done.

https://codereview.chromium.org/264933006/diff/60002/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:984: RawCode* FindCodeForPC(uword pc) {
On 2014/05/02 20:43:37, siva wrote:
> const

Done.

[siva, 2014-05-05 19:54:39]

lgtm

https://codereview.chromium.org/264933006/diff/160001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/264933006/diff/160001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:922: REUSABLE_CODE_HANDLESCOPE(isolate());
You could move this REUSABLE_CODE_HANDLESCOPE after the 'if (..)' check

https://codereview.chromium.org/264933006/diff/160001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:1766: uword GetCurrentFramePcMarker(uword* fp) {
const ?

[Cutch, 2014-05-05 20:02:09]

https://codereview.chromium.org/264933006/diff/160001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/264933006/diff/160001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:922: REUSABLE_CODE_HANDLESCOPE(isolate());
On 2014/05/05 19:54:39, siva wrote:
> You could move this REUSABLE_CODE_HANDLESCOPE after the 'if (..)' check

Done.

https://codereview.chromium.org/264933006/diff/160001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:1766: uword GetCurrentFramePcMarker(uword* fp) {
On 2014/05/05 19:54:39, siva wrote:
> const ?

Done.

[Cutch, 2014-05-05 20:04:21]

Committed patchset #11 manually as r35762 (presubmit successful).