Issue 3226014: Add functionality for finding code objects from a pc that points into...

Issue 3226014: Add functionality for finding code objects from a pc that points into... (Closed)

Created:
10 years, 3 months ago by Rico

Modified:
9 years, 6 months ago

Reviewers:
Kasper Lund

CC:
v8-dev

Base URL:
http://v8.googlecode.com/svn/branches/bleeding_edge/

Visibility:
Public.

Description

Add functionality for finding code objects from a pc that points into the code object's instructions. This allows us to find a code object using just the pc. This approach uses a cache (PcToCodeCache) to make sure we don't continuously have to iterate heap pages. This change eliminates the need for cooking and uncooking of stack frames. Committed: http://code.google.com/p/v8/source/detail?r=5369

Patch Set 1 #

Total comments: 20

Patch Set 2 : '' #

Patch Set 3 : '' #

Created: 10 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+290 lines, -386 lines)			Patch
M	src/arm/frames-arm.cc	View	1 2	2 chunks	+0 lines, -63 lines	0 comments	Download
M	src/frames.h	View	1 2	8 chunks	+51 lines, -26 lines	0 comments	Download
M	src/frames.cc	View	1 2	11 chunks	+137 lines, -56 lines	0 comments	Download
M	src/frames-inl.h	View	1 2	2 chunks	+4 lines, -11 lines	0 comments	Download
M	src/heap.h	View	1 2	4 chunks	+20 lines, -1 line	0 comments	Download
M	src/heap.cc	View	1 2	6 chunks	+30 lines, -11 lines	0 comments	Download
M	src/ia32/frames-ia32.cc	View	1 2	2 chunks	+0 lines, -63 lines	0 comments	Download
M	src/liveedit.cc	View	1 2	2 chunks	+0 lines, -25 lines	0 comments	Download
M	src/mark-compact.h	View	1 2	1 chunk	+6 lines, -0 lines	0 comments	Download
M	src/mark-compact.cc	View	1 2	3 chunks	+6 lines, -4 lines	0 comments	Download
M	src/memory.h	View	1 2	1 chunk	+4 lines, -0 lines	0 comments	Download
M	src/spaces.h	View	1 2	2 chunks	+6 lines, -0 lines	0 comments	Download
M	src/spaces.cc	View	1 2	2 chunks	+22 lines, -0 lines	0 comments	Download
M	src/top.h	View	1 2	2 chunks	+0 lines, -9 lines	0 comments	Download
M	src/top.cc	View	1 2	2 chunks	+0 lines, -34 lines	0 comments	Download
M	src/v8-counters.h	View	1 2	1 chunk	+4 lines, -1 line	0 comments	Download
M	src/v8threads.h	View	1 2	1 chunk	+0 lines, -2 lines	0 comments	Download
M	src/v8threads.cc	View	1 2	1 chunk	+0 lines, -22 lines	0 comments	Download
M	src/x64/frames-x64.cc	View	1 2	2 chunks	+0 lines, -58 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Kasper Lund

LGTM with a few comments: http://codereview.chromium.org/3226014/diff/1/4 File src/frames.cc (right): http://codereview.chromium.org/3226014/diff/1/4#newcode158 src/frames.cc:158: if (type == StackFrame::JAVA_SCRIPT) ...

10 years, 3 months ago (2010-08-30 07:09:41 UTC) #2

Rico

10 years, 3 months ago (2010-08-30 08:54:36 UTC) #3

http://codereview.chromium.org/3226014/diff/1/4
File src/frames.cc (right):

http://codereview.chromium.org/3226014/diff/1/4#newcode158
src/frames.cc:158: if (type == StackFrame::JAVA_SCRIPT) {
On 2010/08/30 07:09:41, Kasper Lund wrote:
> Remove this code or guard it by #ifdef DEBUG and assert that we can always
find
> the code for JavaScript frames. As it is written now, it doesn't make much
> sense.

Done.

http://codereview.chromium.org/3226014/diff/1/4#newcode824
src/frames.cc:824: Address pc) {
On 2010/08/30 07:09:41, Kasper Lund wrote:
> Weird indentation. Use only 4 spaces.
Merged to single line

http://codereview.chromium.org/3226014/diff/1/4#newcode831
src/frames.cc:831: index++;
On 2010/08/30 07:09:41, Kasper Lund wrote:
> I'm not sure I understand why you increment index here. Index isn't used from
> this point forward.

Done.

http://codereview.chromium.org/3226014/diff/1/5
File src/frames.h (right):

http://codereview.chromium.org/3226014/diff/1/5#newcode51
src/frames.h:51: static const int kPcToCodeCacheSize = 256;
On 2010/08/30 07:09:41, Kasper Lund wrote:
> Does the size have to be public?

No, moved to private section

http://codereview.chromium.org/3226014/diff/1/5#newcode64
src/frames.h:64: //  static void FlushPcToCodeCache();
On 2010/08/30 07:09:41, Kasper Lund wrote:
> Remove comment or change it to something that makes sense.

Done.

http://codereview.chromium.org/3226014/diff/1/5#newcode70
src/frames.h:70: 
On 2010/08/30 07:09:41, Kasper Lund wrote:
> Remove one of the empty lines here.

Done.

http://codereview.chromium.org/3226014/diff/1/5#newcode73
src/frames.h:73: 
On 2010/08/30 07:09:41, Kasper Lund wrote:
> Remove two empty lines here.

Done.

http://codereview.chromium.org/3226014/diff/1/5#newcode191
src/frames.h:191: 
On 2010/08/30 07:09:41, Kasper Lund wrote:
> Remove one of the two newlines.

Done.

http://codereview.chromium.org/3226014/diff/1/5#newcode195
src/frames.h:195: return (PcToCodeCache::GetCacheEntry(pc))->code;
On 2010/08/30 07:09:41, Kasper Lund wrote:
> Remove one level of ()s.

Done.

http://codereview.chromium.org/3226014/diff/1/13
File src/spaces.cc (right):

http://codereview.chromium.org/3226014/diff/1/13#newcode2732
src/spaces.cc:2732: // TODO(kasperl): Change this implementation to only find
executable
On 2010/08/30 07:09:41, Kasper Lund wrote:
> File an issue and change the TODO to include the issue number.

Done.

Expand Messages | Collapse Messages