[profiler] Make no frame region detection code more robust [x86/x64]

[profiler] Make no frame region detection code more robust

Upon collection of the stack trace if the current PC falls into
the frame building code, the top frame might be in a non-consistent
state. That leads to some of the frames could be missing from the
stack trace.

The patch makes it check instructions under current PC and if they
look like the frame setup/destroy code, it skips the entire sample.

Support for x86/x64

BUG=chromium:529931
LOG=N

Committed: https://crrev.com/12c7bc9a226859c3200609495689592a675a21af
Cr-Commit-Position: refs/heads/master@{#30777}

[alph, 2015-09-15 22:20:32]

alph@chromium.org changed reviewers:
+ bmeurer@chromium.org, yurys@chromium.org

[Benedikt Meurer, 2015-09-16 03:41:45]

lgtm

[alph, 2015-09-16 04:42:16]

The CQ bit was checked by alph@chromium.org

[commit-bot: I haz the power, 2015-09-16 04:42:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1341413002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1341413002/1

[commit-bot: I haz the power, 2015-09-16 05:04:52]

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

[commit-bot: I haz the power, 2015-09-16 05:04:52]

Try jobs failed on following builders:
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/8077)

[alph, 2015-09-16 05:10:12]

The CQ bit was checked by alph@chromium.org

[commit-bot: I haz the power, 2015-09-16 05:10:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1341413002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1341413002/1

[commit-bot: I haz the power, 2015-09-16 05:42:07]

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

[commit-bot: I haz the power, 2015-09-16 05:42:07]

Try jobs failed on following builders:
  v8_linux_dbg on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/8683)

[yurys, 2015-09-16 14:42:01]

https://codereview.chromium.org/1341413002/diff/1/src/sampler.cc
File src/sampler.cc (right):

https://codereview.chromium.org/1341413002/diff/1/src/sampler.cc#newcode212
src/sampler.cc:212: if (!memcmp(pc - *offset, pattern->bytes,
pattern->bytes_count))
what about the potential case that we discussed offline, when pc is begin of a
page and we cannot access pc -1 ?

[alph, 2015-09-16 17:28:51]

https://codereview.chromium.org/1341413002/diff/1/src/sampler.cc
File src/sampler.cc (right):

https://codereview.chromium.org/1341413002/diff/1/src/sampler.cc#newcode212
src/sampler.cc:212: if (!memcmp(pc - *offset, pattern->bytes,
pattern->bytes_count))
On 2015/09/16 14:42:01, yurys wrote:
> what about the potential case that we discussed offline, when pc is begin of a
> page and we cannot access pc -1 ?

It should never happen for JS code, because of Code object preceding  and in
fact every heap allocated objects are preceded with the system info.

As for the native code, I looked at the memory map for Chrome process and all
the code segments have ELF prologue, e.g.
(gdb) x/8c 0x7fb6cc498000
0x7fb6cc498000:	127 '\177'	69 'E'	76 'L'	70 'F'	2 '\002'	1 '\001'	1 '\001'	0
'\000'

So it looks like we should never hit an unallocated page. Not sure how the
loading behaves on Windows.

In any case we can monitor it for crashes. Or I can still add a check code just
to be on the safe side, but I don't think it's worth.

Wdyt?

[yurys, 2015-09-16 20:30:59]

lgtm

[alph, 2015-09-16 20:56:38]

On 2015/09/16 17:28:51, alph wrote:
> https://codereview.chromium.org/1341413002/diff/1/src/sampler.cc
> File src/sampler.cc (right):
> 
> https://codereview.chromium.org/1341413002/diff/1/src/sampler.cc#newcode212
> src/sampler.cc:212: if (!memcmp(pc - *offset, pattern->bytes,
> pattern->bytes_count))
> On 2015/09/16 14:42:01, yurys wrote:
> > what about the potential case that we discussed offline, when pc is begin of
a
> > page and we cannot access pc -1 ?
> 
> It should never happen for JS code, because of Code object preceding  and in
> fact every heap allocated objects are preceded with the system info.
> 
> As for the native code, I looked at the memory map for Chrome process and all
> the code segments have ELF prologue, e.g.
> (gdb) x/8c 0x7fb6cc498000
> 0x7fb6cc498000:	127 '\177'	69 'E'	76 'L'	70 'F'	2 '\002'	1 '\001'	1 '\001'	0
> '\000'
> 
> So it looks like we should never hit an unallocated page. Not sure how the
> loading behaves on Windows.
> 
> In any case we can monitor it for crashes. Or I can still add a check code
just
> to be on the safe side, but I don't think it's worth.
> 
> Wdyt?

Looks like I just hit one in the test. ;-)

There's an code section mapped right at the page boundary:
(gdb) disassemble 0x3ab70000,+10
Dump of assembler code from 0x3ab70000 to 0x3ab70064:
   0x3ab70000:	push   %edi
   0x3ab70001:	push   %esi
   0x3ab70002:	mov    0xc(%esp),%edi
$ cat /proc/37002/maps
....
3ab70000-3ab71000 r-xp 00000000 00:00 0 

I'll add a check.

[alph, 2015-09-16 22:29:52]

ptal

[yurys, 2015-09-16 22:39:43]

lgtm

https://codereview.chromium.org/1341413002/diff/20001/src/sampler.cc
File src/sampler.cc (right):

https://codereview.chromium.org/1341413002/diff/20001/src/sampler.cc#newcode178
src/sampler.cc:178: const uint32_t kPageSize = 4096;
Would be nice to have this constant defined in a single place rather than
copying the declaration all around v8 code.

[alph, 2015-09-16 23:06:33]

https://codereview.chromium.org/1341413002/diff/20001/src/sampler.cc
File src/sampler.cc (right):

https://codereview.chromium.org/1341413002/diff/20001/src/sampler.cc#newcode178
src/sampler.cc:178: const uint32_t kPageSize = 4096;
On 2015/09/16 22:39:43, yurys wrote:
> Would be nice to have this constant defined in a single place rather than
> copying the declaration all around v8 code.

Sounds like a good idea, though it should go as a separate patch.

[alph, 2015-09-16 23:06:39]

The CQ bit was checked by alph@chromium.org

[alph, 2015-09-16 23:06:41]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1341413002/#ps20001
(title: "added a check for page boundary")

[commit-bot: I haz the power, 2015-09-16 23:06:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1341413002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1341413002/20001

[commit-bot: I haz the power, 2015-09-17 00:12:14]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-17 00:12:31]

Patchset 2 (id:??) landed as
https://crrev.com/12c7bc9a226859c3200609495689592a675a21af
Cr-Commit-Position: refs/heads/master@{#30777}

[alph, 2015-09-17 07:13:41]

SimulatorHelper seems to provide a bogus value of PC sometimes.
In most cases FP is set to zero, which means the simulator is updating
registers, e.g. like in this sample:

sim regs.sp:0x319afbc regs.fp:0x0 regs.pc:0xfffffffe

and we gracefully bail out in HandleProfilerSignal. But sometimes both SP and FP
are valid, but PC is still having that bogus value, e.g.:

sim regs.sp:0x319aea8 regs.fp:0x319aecc regs.pc:0xfffffffe

causing a crash.