First set of comments. -ip
http://codereview.chromium.org/14158/diff/1/2 File src/virtual-frame-ia32.cc (right): http://codereview.chromium.org/14158/diff/1/2#newcode573 Line 573: // one can be popped, bet first we may have to adjust the stack bet->but http://codereview.chromium.org/14158/diff/1/2#newcode584 Line 584: Immediate((stack_pointer_ - i) * kPointerSize)); At this point stack_pointer_ is (i + 1). Thus stack_pointer_ - i == (i + 1) - i == 1 is the value being added to esp. I do not think this is the intention. Are you sure this code was tested? I can imagine that it is pretty hard to generate such a case which makes it really mandatory to ensure that this code works. Otherwise you can run for years with such a bug in the code generator without ever finding out about it. More comments about your intentions would probably let you come to the same conclusion while writing the code. Alternatively you could be less frugal with your local variables and make your intentions explicit in the code (aka comments get stale, code at least has to compile): int new_stack_pointer = i + 1; ASSERT((stack_pointer_ - new_stack_pointer) > 0); // I know this assertion is a duplicate of the above if check but it documents the precondition for the add here and it ensures that the if check does not get changed. __ add(Operand(esp), Immediate((stack_pointer_ - new_stack_pointer) * kPointerSize); stack_pointer_ = new_stack_pointer; http://codereview.chromium.org/14158/diff/1/2#newcode601 Line 601: } Unless I am missing something about merging frames, both stack_pointer_ values should be equal at this point. Please assert that, alternatively this and everything else should be asserted in MergeTo(). http://codereview.chromium.org/14158 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
