http://codereview.chromium.org/466033/diff/1/6 File src/fast-codegen.cc (right):
http://codereview.chromium.org/466033/diff/1/6#newcode235 src/fast-codegen.cc:235: I wrote a big comment here, but I came to the conclusion that this design is OK, as long as the EnvironmentEffect subclasses are created automatically on the stack, and destroyed automatically, removing themselves from the list. But is this possible? The head of the list of environments, which you have in environment_, but needs to be just somewhere in the codegen, can be passed to the constructor of the object, but to remove the object from the list, and restore the saved parent environment, in the destructor of the BreakTargetEnvironment, requires a pointer to codegen or to Environment in the BreakTargetEnvironment object (or a safe static access to the current codegen). That puts a dumb pointer to the codegen in every BreakTargetEnvironment. http://codereview.chromium.org/466033/diff/1/6#newcode238 src/fast-codegen.cc:238: BreakTargetEnvironment break_env(stmt, &end_of_block); On 2009/12/07 14:27:36, Kevin Millikin wrote: > All these classes have the protocol (1) construct, immediately (2) push on > environment_, bracketed by (3) pop. Perfect candidate for RAII so you just > write: Yes, exactly. > Break break_target(this, stmt, &end_of_block); > VisitStatements(stmt->statements()); > __ bind(&end_of_block); http://codereview.chromium.org/466033/diff/1/6#newcode245 src/fast-codegen.cc:245: environment_.Drop(1); To be more clear, this command is hard to do in the destructor of BreakTargetEnvironment, unless it has a pointer to environment_ or codegen. http://codereview.chromium.org/466033/diff/1/6#newcode307 src/fast-codegen.cc:307: __ Call(current->AsTryFinally()->finally_entry()); The introduction of Call(Label*) seems really dangerous to me. If we were calling to a label in the old codegen, it was as part of making an exception-handler item on the stack, I think. Putting an address on the stack is really dangerous to the GC, isn't it? The introduction of Call in the assembler makes it look like it is safe to call to someplace other than the beginning of a code object. If this code isn't doing exactly what the old code did, I doubt it is safe, and if it is, it should have warnings on the use of Call(Label). Am I misunderstanding this? http://codereview.chromium.org/466033/diff/1/6#newcode333 src/fast-codegen.cc:333: int target_offset = environment_.FindBreakTargetOffset(stmt->target()); There is a potential n^2 behavior here, but we are probably okay with that, right? Any problem about the number of nested scopes times the number of breaks that break them all is allowed to be there, I suppose. http://codereview.chromium.org/466033/diff/1/7 File src/fast-codegen.h (right): http://codereview.chromium.org/466033/diff/1/7#newcode173 src/fast-codegen.h:173: class Environment { On 2009/12/07 14:27:36, Kevin Millikin wrote: > Instead of a separate ZoneList of pointers to stack-allocated objects, I'd just > thread the objects through the stack with a pointer in each. > Probably just because I hate ZoneList. Are you saying that, for example, VisitIterationStatement would have an automatic (local, stack-allocated) ContinueTargetEnvironment object, that would be placed on Environment.stack_ by its constructor, and removed by its destructor? That seems quite reasonable. (Environment.stack_ would just be the top of the linked list, and the links are in the objects, or this would just be FastCodeGenerator::environment_stack. How about "NestedStackUser" instead of "Environment". Or StackStuff. StackDisciplinedStuff? RuntimeStackMarker? longjump_target_? http://codereview.chromium.org/466033/diff/1/7#newcode330 src/fast-codegen.h:330: void ExitEnvironments(int count); Does ExitEnvironments actually leave the environments, and remove them from the Environment stack, or does it emit runtime code to remove those environments' stack-allocated slots on one runtime control flow path (probably followed by a jump)? http://codereview.chromium.org/466033/diff/1/11 File src/x64/fast-codegen-x64.cc (right): http://codereview.chromium.org/466033/diff/1/11#newcode521 src/x64/fast-codegen-x64.cc:521: int return_offset = environment_.FindReturnOffset(); This can't be right in the case of try-finally. Should at least a TODO be put here so that this is fixed when try-finally is added? http://codereview.chromium.org/466033 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
