Issue 316023002: --debug-code: sanity-checking instrumentation for Lithium object accesses

Issue 316023002: --debug-code: sanity-checking instrumentation for Lithium object accesses (Closed)

Created:
6 years, 6 months ago by Jakob Kummerow

Modified:
5 years, 1 month ago

Reviewers:
Igor Sheludko, titzer

CC:
v8-dev, danno

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

Visibility:
Public.

More Reviews

Description

--debug-code: add sanity-checking instrumentation for Lithium object accesses Only implemented for x64, but since this is supposed to catch platform-independent bugs, that should suffice.

Patch Set 2 : addressed Igor's comments #

Created: 6 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+491 lines, -108 lines)			Patch
M	src/code-stubs-hydrogen.cc	View		1 chunk	+3 lines, -1 line	0 comments	Download
M	src/hydrogen.cc	View	1	3 chunks	+7 lines, -10 lines	0 comments	Download
M	src/hydrogen-instructions.h	View	1	8 chunks	+152 lines, -60 lines	0 comments	Download
M	src/hydrogen-instructions.cc	View	1	4 chunks	+32 lines, -23 lines	0 comments	Download
M	src/objects.h	View	1	7 chunks	+13 lines, -7 lines	0 comments	Download
M	src/x64/lithium-codegen-x64.h	View	1	1 chunk	+9 lines, -0 lines	0 comments	Download
M	src/x64/lithium-codegen-x64.cc	View	1	10 chunks	+275 lines, -7 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Igor Sheludko

lgtm with nits: https://codereview.chromium.org/316023002/diff/20001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/316023002/diff/20001/src/hydrogen-instructions.cc#newcode4643 src/hydrogen-instructions.cc:4643: case AllocationSite::kTransitionInfoOffset: While we are here, ...

6 years, 6 months ago (2014-06-05 09:04:29 UTC) #2

titzer

On 2014/06/05 09:04:29, Igor Sheludko wrote: > lgtm with nits: > > https://codereview.chromium.org/316023002/diff/20001/src/hydrogen-instructions.cc > File ...

6 years, 6 months ago (2014-06-05 10:20:02 UTC) #3

Jakob Kummerow

6 years, 6 months ago (2014-06-05 14:33:30 UTC) #4

https://codereview.chromium.org/316023002/diff/20001/src/hydrogen-instruction...
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/316023002/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:4643: case AllocationSite::kTransitionInfoOffset:
On 2014/06/05 09:04:30, Igor Sheludko wrote:
> While we are here, does it make sense to merge common cases together?

Done.

https://codereview.chromium.org/316023002/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/316023002/diff/20001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:10499: BuildEmitObjectHeader(boilerplate_object, object);
On 2014/06/05 09:04:30, Igor Sheludko wrote:
> I think it is better to move BuildEmitObjectHeader() before the "If allocation
> ..." comment above.

Done.

https://codereview.chromium.org/316023002/diff/20001/src/hydrogen.cc#newcode1...
src/hydrogen.cc:10501: empty_fixed_array, INITIALIZING_STORE);
On 2014/06/05 09:04:30, Igor Sheludko wrote:
> I think it is cleaner not to pass INITIALIZING_STORE explicitly here as it is
> the default value.

Done. (I think this was a rebasing artifact, as I only moved stuff around.)

https://codereview.chromium.org/316023002/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/316023002/diff/20001/src/objects.h#newcode6329
src/objects.h:6329: return static_cast<uint32_t>(instance_type()) >=
FIRST_JS_OBJECT_TYPE;
On 2014/06/05 09:04:30, Igor Sheludko wrote:
> I think you can use IsJSObjectMap() predicate here and probably in all other
> places where we do similar instance type comparison.

Done. Thanks for the pointer :-)

https://codereview.chromium.org/316023002/diff/20001/src/x64/lithium-codegen-...
File src/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/316023002/diff/20001/src/x64/lithium-codegen-...
src/x64/lithium-codegen-x64.cc:3054: // Load backing store length, with implicit
Smi untagging.
On 2014/06/05 09:04:30, Igor Sheludko wrote:
> STATIC_ASSERT(kSmiTagSize + kSmiShiftSize == 32);

Done.

https://codereview.chromium.org/316023002/diff/20001/src/x64/lithium-codegen-...
src/x64/lithium-codegen-x64.cc:3093: // Don't use UNKNOWN_PURPOSE.
On 2014/06/05 09:04:30, Igor Sheludko wrote:
>     default:

Nope. We generally don't use "default:" cases when switching over enums, so the
compiler will complain if we forgot a case.

https://codereview.chromium.org/316023002/diff/20001/src/x64/lithium-codegen-...
src/x64/lithium-codegen-x64.cc:3255: // Load elements length, with implicit Smi
untagging.
On 2014/06/05 09:04:30, Igor Sheludko wrote:
> STATIC_ASSERT(kSmiTagSize + kSmiShiftSize == 32);

Done.

https://codereview.chromium.org/316023002/diff/20001/src/x64/lithium-codegen-...
src/x64/lithium-codegen-x64.cc:4258: 
On 2014/06/05 09:04:30, Igor Sheludko wrote:
> Extra empty line?

Done.

Expand Messages | Collapse Messages