with statements now handle exceptions properly

src/codegen/compvars.cpp

@@ -738,6 +738,8 @@ CompilerVariable* makeFunction(IREmitter& emitter, CLFunction* f, CompilerVariab
 
     llvm::Value* isGenerator_v = llvm::ConstantInt::get(g.i1, isGenerator, false);
 
+    // We know this function call can't throw, so it's safe to use emitter.getBuilder()->CreateCall() rather than
+    // emitter.createCall().
     llvm::Value* boxed = emitter.getBuilder()->CreateCall(
         g.funcs.boxCLFunction,
         std::vector<llvm::Value*>{ embedConstantPtr(f, g.llvm_clfunction_type_ptr), closure_v, isGenerator_v, scratch,

src/codegen/compvars.h

@@ -286,7 +286,7 @@ private:
 
 protected:
     void drop(IREmitter& emitter) override { type->drop(emitter, this); }
-    void grab(IREmitter& emmitter) override { type->grab(emmitter, this); }
+    void grab(IREmitter& emitter) override { type->grab(emitter, this); }
 
 public:
     ValuedCompilerVariable(T* type, V value, bool grabbed) : CompilerVariable(grabbed), type(type), value(value) {

src/codegen/irgen/irgenerator.cpp

@@ -291,6 +291,7 @@ private:
 
     llvm::BasicBlock* curblock;
     IREmitterImpl emitter;
+    // symbol_table tracks which (non-global) python variables are bound to which CompilerVariables
     SymbolTable symbol_table;
     std::unordered_map<CFGBlock*, llvm::BasicBlock*>& entry_blocks;
     CFGBlock* myblock;
@@ -883,6 +884,7 @@ private:
 
             return closure->getattr(emitter, getEmptyOpInfo(unw_info), &node->id.str(), false);
         } else {
+            // vst is one of {FAST, CLOSURE, NAME}
             if (symbol_table.find(node->id) == symbol_table.end()) {
                 // classdefs have different scoping rules than functions:
                 if (vst == ScopeInfo::VarScopeType::NAME) {
@@ -1225,6 +1227,12 @@ private:
         cur = val;
     }
 
+    // whether a Python variable FOO might be undefined or not is determined by whether the corresponding is_defined_FOO
+    // variable is present in our symbol table. If it is, then it *might* be undefined. If it isn't, then it either is
+    // definitely defined, or definitely isn't.
+    //
+    // to check whether a variable is in our symbol table, call _getFake with allow_missing = true and check whether the
+    // result is NULL.
     CompilerVariable* _getFake(InternedString name, bool allow_missing = false) {
         assert(name.str()[0] == '!');
         auto it = symbol_table.find(name);
@@ -1243,6 +1251,7 @@ private:
         return rtn;
     }
 
+    // only updates symbol_table if we're *not* setting a global
     void _doSet(InternedString name, CompilerVariable* val, UnwindInfo unw_info) {
         assert(name.str() != "None");
 
@@ -1702,7 +1711,7 @@ private:
         // that case asking it to convert to itself ends up just being an incvref
         // and doesn't end up emitting an incref+decref pair.
         // This could also be handled by casting from the CompilerVariable to
-        // ConcreteCOmpilerVariable, but this way feels a little more robust to me.
+        // ConcreteCompilerVariable, but this way feels a little more robust to me.
         ConcreteCompilerType* opt_rtn_type = irstate->getReturnType();
         if (irstate->getReturnType()->llvmType() == val->getConcreteType()->llvmType())
             opt_rtn_type = val->getConcreteType();
@@ -2060,9 +2069,9 @@ private:
         SourceInfo* source = irstate->getSourceInfo();
         ScopeInfo* scope_info = irstate->getScopeInfo();
 
-        // Additional names to remove; remove them after iteration is done to new mess up the iterators
+        // Additional names to remove; remove them after iteration is done to not mess up the iterators
         std::vector<InternedString> also_remove;
-        for (SymbolTable::iterator it = symbol_table.begin(); it != symbol_table.end();) {
+        for (auto it = symbol_table.begin(); it != symbol_table.end();) {
             if (allowableFakeEndingSymbol(it->first)) {
                 ++it;
                 continue;
@@ -2073,7 +2082,7 @@ private:
 
             if (!source->liveness->isLiveAtEnd(it->first, myblock)) {
                 // printf("%s dead at end of %d; grabbed = %d, %d vrefs\n", it->first.c_str(), myblock->idx,
-                // it->second->isGrabbed(), it->second->getVrefs());
+                //        it->second->isGrabbed(), it->second->getVrefs());
                 also_remove.push_back(getIsDefinedName(it->first));
 
                 it->second->decvref(emitter);
@@ -2208,6 +2217,8 @@ public:
             return EndingState(st, phi_st, curblock);
         }
 
+        // We have one successor, but they have more than one predecessor.
+        // We're going to sort out which symbols need to go in phi_st and which belong inst.
         for (SymbolTable::iterator it = st->begin(); it != st->end();) {
             if (allowableFakeEndingSymbol(it->first) || source->phis->isRequiredAfter(it->first, myblock)) {
                 ASSERT(it->second->isGrabbed(), "%s", it->first.c_str());
@@ -2344,12 +2355,24 @@ public:
     }
 
     void run(const CFGBlock* block) override {
+        if (VERBOSITY("irgenerator") >= 1) { // print starting symbol table
+            printf("  %d init:", block->idx);
+            for (auto it = symbol_table.begin(); it != symbol_table.end(); ++it)
+                printf(" %s", it->first.c_str());
+            printf("\n");
+        }
         for (int i = 0; i < block->body.size(); i++) {
             if (state == DEAD)
                 break;
             assert(state != FINISHED);
             doStmt(block->body[i], UnwindInfo(block->body[i], NULL));
         }
+        if (VERBOSITY("irgenerator") >= 1) { // print ending symbol table
+            printf("  %d fini:", block->idx);
+            for (auto it = symbol_table.begin(); it != symbol_table.end(); ++it)
+                printf(" %s", it->first.c_str());
+            printf("\n");
+        }
     }
 
     void doSafePoint() override { emitter.getBuilder()->CreateCall(g.funcs.allowGLReadPreemption); }

src/codegen/irgen/irgenerator.h

@@ -95,10 +95,14 @@ public:
     ParamNames* getParamNames() { return param_names; }
 };
 
+// turns CFGBlocks into LLVM IR
 class IRGenerator {
 private:
 public:
     struct EndingState {
+        // symbol_table records which Python variables are bound to what CompilerVariables at the end of this block.
+        // phi_symbol_table records the ones that will need to be `phi'd.
+        // both only record non-globals.
         SymbolTable* symbol_table;
         ConcreteSymbolTable* phi_symbol_table;
         llvm::BasicBlock* ending_block;
@@ -113,7 +117,7 @@ public:
 
     virtual void giveLocalSymbol(InternedString name, CompilerVariable* var) = 0;
     virtual void copySymbolsFrom(SymbolTable* st) = 0;
-    virtual void run(const CFGBlock* block) = 0;
+    virtual void run(const CFGBlock* block) = 0; // primary entry point
     virtual EndingState getEndingSymbolTable() = 0;
     virtual void doSafePoint() = 0;
     virtual void addFrameStackmapArgs(PatchpointInfo* pp, AST_stmt* current_stmt,

src/core/ast.h

@@ -118,7 +118,7 @@ enum AST_TYPE {
     DictComp = 15,
     Set = 43,
     Ellipsis = 87,
-    Expression = 88,
+    Expression = 88, // like Module, but used for eval.
 
     // Pseudo-nodes that are specific to this compiler:
     Branch = 200,
@@ -1007,14 +1007,14 @@ class AST_LangPrimitive : public AST_expr {
 public:
     enum Opcodes {
         ISINSTANCE,
-        LANDINGPAD,
+        LANDINGPAD, // grabs the info about the last raised exception
         LOCALS,
         GET_ITER,
         IMPORT_FROM,
         IMPORT_NAME,
         IMPORT_STAR,
         NONE,
-        NONZERO,
+        NONZERO, // determines whether something is "true" for purposes of `if'
         SET_EXC_INFO,
         UNCACHE_EXC_INFO,
     } opcode;

src/core/cfg.h

@@ -56,6 +56,7 @@ public:
     void unconnectFrom(CFGBlock* successor);
 
     void push_back(AST_stmt* node) { body.push_back(node); }
+    void print();
 };
 
 // Control Flow Graph
@@ -79,6 +80,9 @@ public:
         return block;
     }
 
+    // Creates a block which must be placed later, using placeBlock().
+    // Must be placed on same CFG it was created on.
+    // You can also safely delete it without placing it.
     CFGBlock* addDeferredBlock() {
         CFGBlock* block = new CFGBlock(this, -1);
         return block;

src/core/common.h

@@ -39,6 +39,8 @@
 #define _CAT(A, B) A##B
 #define CAT(A, B) _CAT(A, B)
 
+#define ARRAY_LEN(arr) (sizeof(arr) / sizeof((arr)[0]))
+
 // GCC and clang handle always_inline very differently;
 // we mostly only care about it for the stdlib, so just remove the attributes
 // if we're not in clang

src/core/util.h

@@ -52,7 +52,9 @@ bool endswith(const std::string& s, const std::string& pattern);
 
 void removeDirectoryIfExists(const std::string& path);
 
-template <class T1, class T2> void compareKeyset(T1* lhs, T2* rhs) {
+// Checks that lhs and rhs, which are iterables of InternedStrings, have the
+// same set of names in them.
+template <class T1, class T2> bool sameKeyset(T1* lhs, T2* rhs) {
     std::vector<InternedString> lv, rv;
     for (typename T1::iterator it = lhs->begin(); it != lhs->end(); it++) {
         lv.push_back(it->first);
@@ -89,7 +91,7 @@ template <class T1, class T2> void compareKeyset(T1* lhs, T2* rhs) {
         }
         good = false;
     }
-    assert(good);
+    return good;
 }
 }
 

src/runtime/types.cpp

@@ -366,6 +366,9 @@ std::string BoxedModule::name() {
     }
 }
 
+// This mustn't throw; our IR generator generates calls to it without "invoke" even when there are exception handlers /
+// finally-blocks in scope.
+// TODO: should we use C++11 `noexcept' here?
 extern "C" Box* boxCLFunction(CLFunction* f, BoxedClosure* closure, bool isGenerator,
                               std::initializer_list<Box*> defaults) {
     if (closure)

test/tests/exceptions_test.py

-# fail-if: '-n' in EXTRA_JIT_ARGS or '-O' in EXTRA_JIT_ARGS
-# we have an llvm codegen bug that this file triggers when we JIT
 class TestException(Exception):
     pass
 

test/tests/sys_test.py

+# expected: fail
 # allow-warning: converting unicode literal to str
 
 import sys

test/tests/try_class.py

@@ -27,5 +27,6 @@ def f2():
             print 'here'
     except:
         print 'impossible'
+        print D
         raise
 f2()

test/tests/try_def.py

+# fail-if: (('-O' in EXTRA_JIT_ARGS) or ('-n' in EXTRA_JIT_ARGS)) and 'release' not in IMAGE
 def f():
     try:
         def foo(): return 0

test/tests/with_class_redefine.py

-# expected: fail
+# fail-if: (('-O' in EXTRA_JIT_ARGS) or ('-n' in EXTRA_JIT_ARGS)) and 'release' not in IMAGE
 def f():
     C = 23
     try:

test/tests/with_ctxclass_order_of_access.py

@@ -5,7 +5,7 @@ class Mgr(object):
     def __enter__(self):
         print 'Mgr.__enter__ accessed'
         def enterer(*args):
-            print 'Mgr.__enter__%r called' % (args,)
+            print 'Mgr.__enter__ called'
             return 23
         return enterer
 
@@ -13,7 +13,7 @@ class Mgr(object):
     def __exit__(self):
         print 'Mgr.__exit__ accessed'
         def exiter(*args):
-            print 'Mgr.__exit__%r called' % (args,)
+            print 'Mgr.__exit__ called'
             return False
         return exiter
 

test/tests/with_functiondef.py

+# fail-if: (('-O' in EXTRA_JIT_ARGS) or ('-n' in EXTRA_JIT_ARGS)) and 'release' not in IMAGE
 def f():
-    # originally this exposed a bug in our irgen phase, so even `with None`
-    # failed here; the bug happened before actual execution. Just to test more
-    # things, though, we use an actual contextmanager here.
+    # this exposes a bug in our irgen phase, so even `with None` bugs out here;
+    # the bug happens before actual execution. Just to test more things, though,
+    # we use an actual contextmanager here.
     with open('/dev/null'):
         def foo():
-            # raises a syntaxerror
             pass
 
 f()