rename UnwindSession to PythonUnwindSession, and add a few comments

src/codegen/ast_interpreter.cpp

@@ -41,7 +41,6 @@
 #include "runtime/long.h"
 #include "runtime/objmodel.h"
 #include "runtime/set.h"
-#include "runtime/traceback.h"
 #include "runtime/types.h"
 
 #ifndef NDEBUG

src/codegen/unwinding.cpp

@@ -58,9 +58,7 @@ struct uw_table_entry {
 
 namespace pyston {
 
-namespace {
 static BoxedClass* unwind_session_cls;
-}
 
 // Parse an .eh_frame section, and construct a "binary search table" such as you would find in a .eh_frame_hdr section.
 // Currently only supports .eh_frame sections with exactly one fde.
@@ -440,6 +438,9 @@ static inline unw_word_t get_cursor_bp(unw_cursor_t* cursor) {
     return get_cursor_reg(cursor, UNW_TDEP_BP);
 }
 
+// if the given ip/bp correspond to a jitted frame or
+// ASTInterpreter::execute_inner frame, return true and return the
+// frame information through the PythonFrameIteratorImpl* info arg.
 bool frameIsPythonFrame(unw_word_t ip, unw_word_t bp, unw_cursor_t* cursor, PythonFrameIteratorImpl* info) {
     CompiledFunction* cf = getCFForAddress(ip);
     bool jitted = cf != NULL;
@@ -474,7 +475,7 @@ bool frameIsPythonFrame(unw_word_t ip, unw_word_t bp, unw_cursor_t* cursor, Pyth
     return true;
 }
 
-class UnwindSession : public Box {
+class PythonUnwindSession : public Box {
     ExcInfo exc_info;
     bool skip;
     bool is_active;
@@ -482,9 +483,12 @@ class UnwindSession : public Box {
 public:
     DEFAULT_CLASS_SIMPLE(unwind_session_cls);
 
-    UnwindSession() : exc_info(NULL, NULL, NULL), skip(false), is_active(false) {}
+    PythonUnwindSession() : exc_info(NULL, NULL, NULL), skip(false), is_active(false) {}
 
-    ExcInfo* getExcInfoStorage() { return &exc_info; }
+    ExcInfo* getExcInfoStorage() {
+        RELEASE_ASSERT(is_active, "");
+        return &exc_info;
+    }
     bool shouldSkipFrame() const { return skip; }
     void setShouldSkipNextFrame(bool skip) { this->skip = skip; }
     bool isActive() const { return is_active; }
@@ -512,8 +516,13 @@ public:
     static void gcHandler(GCVisitor* v, Box* _o) {
         assert(_o->cls == unwind_session_cls);
 
-        UnwindSession* o = static_cast<UnwindSession*>(_o);
+        PythonUnwindSession* o = static_cast<PythonUnwindSession*>(_o);
 
+        // this is our hack for eventually collecting
+        // exceptions/tracebacks after the exception has been caught.
+        // If a collection happens and a given thread's
+        // PythonUnwindSession isn't active, its exception info can be
+        // collected.
         if (!o->is_active)
             return;
 
@@ -522,29 +531,29 @@ public:
         v->visitIf(o->exc_info.traceback);
     }
 };
-static __thread UnwindSession* cur_unwind;
+static __thread PythonUnwindSession* cur_unwind;
 
-UnwindSession* beginUnwind() {
+PythonUnwindSession* beginPythonUnwindSession() {
     if (!cur_unwind) {
-        cur_unwind = new UnwindSession();
+        cur_unwind = new PythonUnwindSession();
         pyston::gc::registerPermanentRoot(cur_unwind);
     }
     cur_unwind->begin();
     return cur_unwind;
 }
 
-UnwindSession* getUnwind() {
-    RELEASE_ASSERT(cur_unwind, "");
+PythonUnwindSession* getActivePythonUnwindSession() {
+    RELEASE_ASSERT(cur_unwind && cur_unwind->isActive(), "");
     return cur_unwind;
 }
 
-void endUnwind(UnwindSession* unwind) {
+void endPythonUnwindSession(PythonUnwindSession* unwind) {
     RELEASE_ASSERT(unwind && unwind == cur_unwind, "");
     unwind->end();
 }
-void* getExceptionStorage(UnwindSession* unwind) {
+void* getPythonUnwindSessionExceptionStorage(PythonUnwindSession* unwind) {
     RELEASE_ASSERT(unwind && unwind == cur_unwind, "");
-    UnwindSession* state = static_cast<UnwindSession*>(unwind);
+    PythonUnwindSession* state = static_cast<PythonUnwindSession*>(unwind);
     return state->getExcInfoStorage();
 }
 
@@ -559,14 +568,14 @@ static const LineInfo lineInfoForFrame(PythonFrameIteratorImpl* frame_it) {
 }
 
 void exceptionCaughtInInterpreter(LineInfo line_info, ExcInfo* exc_info) {
-    // basically the same as UnwindSession::addTraceback, but needs to
-    // be callable after an UnwindSession has ended.  The interpreter
+    // basically the same as PythonUnwindSession::addTraceback, but needs to
+    // be callable after an PythonUnwindSession has ended.  The interpreter
     // will call this from catch blocks if it needs to ensure that a
     // line is added.  Right now this only happens in
     // ASTInterpreter::visit_invoke.
 
     // It's basically the same except for one thing: we don't have to
-    // worry about the 'skip' (osr) state that UnwindSession handles
+    // worry about the 'skip' (osr) state that PythonUnwindSession handles
     // here, because the only way we could have gotten into the ast
     // interpreter is if the exception wasn't caught, and if there was
     // the osr frame for the one the interpreter is running, it would
@@ -578,7 +587,7 @@ void exceptionCaughtInInterpreter(LineInfo line_info, ExcInfo* exc_info) {
     BoxedTraceback::Here(line_info, reinterpret_cast<BoxedTraceback**>(&exc_info->traceback));
 }
 
-void unwindingThroughFrame(UnwindSession* unwind_session, unw_cursor_t* cursor) {
+void unwindingThroughFrame(PythonUnwindSession* unwind_session, unw_cursor_t* cursor) {
     unw_word_t ip = get_cursor_ip(cursor);
     unw_word_t bp = get_cursor_bp(cursor);
 
@@ -597,7 +606,7 @@ void unwindingThroughFrame(UnwindSession* unwind_session, unw_cursor_t* cursor)
 // C++11 range loops, for example).
 // Return true from the handler to stop iteration at that frame.
 template <typename Func> void unwindPythonStack(Func func) {
-    UnwindSession* unwind_session = new UnwindSession();
+    PythonUnwindSession* unwind_session = new PythonUnwindSession();
 
     unwind_session->begin();
 
@@ -1146,8 +1155,8 @@ llvm::JITEventListener* makeTracebacksListener() {
 }
 
 void setupUnwinding() {
-    unwind_session_cls = BoxedHeapClass::create(type_cls, object_cls, UnwindSession::gcHandler, 0, 0,
-                                                sizeof(UnwindSession), false, "unwind_session");
+    unwind_session_cls = BoxedHeapClass::create(type_cls, object_cls, PythonUnwindSession::gcHandler, 0, 0,
+                                                sizeof(PythonUnwindSession), false, "unwind_session");
     unwind_session_cls->freeze();
 }
 }

src/codegen/unwinding.h

@@ -41,12 +41,12 @@ CompiledFunction* getCFForAddress(uint64_t addr);
 
 BoxedTraceback* getTraceback();
 
-class UnwindSession;
-UnwindSession* beginUnwind();
-UnwindSession* getUnwind();
-void endUnwind(UnwindSession* unwind_session);
-void* getExceptionStorage(UnwindSession* unwind_session);
-void unwindingThroughFrame(UnwindSession* unwind_session, unw_cursor_t* cursor);
+class PythonUnwindSession;
+PythonUnwindSession* beginPythonUnwindSession();
+PythonUnwindSession* getActivePythonUnwindSession();
+void endPythonUnwindSession(PythonUnwindSession* unwind_session);
+void* getPythonUnwindSessionExceptionStorage(PythonUnwindSession* unwind_session);
+void unwindingThroughFrame(PythonUnwindSession* unwind_session, unw_cursor_t* cursor);
 
 void exceptionCaughtInInterpreter(LineInfo line_info, ExcInfo* exc_info);
 

src/runtime/cxx_unwind.cpp

@@ -20,7 +20,6 @@
 
 #include "llvm/Support/LEB128.h" // for {U,S}LEB128 decoding
 
-#include "asm_writing/assembler.h"   // assembler
 #include "codegen/ast_interpreter.h" // interpreter_instr_addr
 #include "codegen/unwinding.h"       // getCFForAddress
 #include "core/ast.h"
@@ -492,7 +491,7 @@ static inline void unwind_loop(ExcInfo* exc_data) {
     unw_getcontext(&uc);
     unw_init_local(&cursor, &uc);
 
-    auto unwind_token = getUnwind();
+    auto unwind_session = getActivePythonUnwindSession();
 
     while (unw_step(&cursor) > 0) {
         unw_proc_info_t pip;
@@ -507,7 +506,10 @@ static inline void unwind_loop(ExcInfo* exc_data) {
             print_frame(&cursor, &pip);
         }
 
-        unwindingThroughFrame(unwind_token, &cursor);
+        // let the PythonUnwindSession know that we're in a new frame,
+        // giving it a chance to possibly add a traceback entry for
+        // it.
+        unwindingThroughFrame(unwind_session, &cursor);
 
         // Skip frames without handlers
         if (pip.handler == 0) {
@@ -556,8 +558,23 @@ static inline void unwind_loop(ExcInfo* exc_data) {
 
         int64_t switch_value = determine_action(&info, &entry);
         if (switch_value != CLEANUP_ACTION) {
-            endUnwind(unwind_token);
+            // we're transfering control to a non-cleanup landing pad.
+            // i.e. a catch block.  thus ends our unwind session.
+            endPythonUnwindSession(unwind_session);
         }
+        // there is a python unwinding implementation detail leaked
+        // here - that the unwind session can be ended but its
+        // exception storage is still around.
+        //
+        // this manifests itself as this short window here where we've
+        // (possibly) ended the unwind session above but we still need
+        // to pass exc_data (which is the exceptionStorage for this
+        // unwind session) to resume().
+        //
+        // the only way this could bite us is if we somehow clobber
+        // the PythonUnwindSession's storage, or cause a GC to occur, before
+        // transfering control to the landing pad in resume().
+        //
         resume(&cursor, entry.landing_pad, switch_value, exc_data);
     }
 
@@ -612,30 +629,10 @@ extern "C" void* __cxa_allocate_exception(size_t size) noexcept {
     // we should only ever be throwing ExcInfos
     RELEASE_ASSERT(size == sizeof(pyston::ExcInfo), "allocating exception whose size doesn't match ExcInfo");
 
-    // Instead of allocating memory for this exception, we return a pointer to a pre-allocated thread-local variable.
-    //
-    // This variable, pyston::exception_ferry, is used only while we are unwinding, and should not be used outside of
-    // the unwinder. Since it's a thread-local variable, we *cannot* throw any exceptions while it is live, otherwise we
-    // would clobber it and forget our old exception.
-    //
-    // Q: Why can't we just use cur_thread_state.curexc_{type,value,traceback}?
-    //
-    // A: Because that conflates the space used to store exceptions during C++ unwinding with the space used to store
-    // them during C-API return-code based unwinding! This actually comes up in practice - the original version *did*
-    // use curexc_{type,value,traceback}, and it had a bug.
-    //
-    // In particular, we need to unset the C API exception at an appropriate point so as not to make C-API functions
-    // *think* an exception is being thrown when one isn't. The natural place is __cxa_begin_catch, BUT we need some way
-    // to communicate the exception info to the inside of the catch block - and all we get is the return value of
-    // __cxa_begin_catch, which is a single pointer, when we need three!
-    //
-    // You might think we could get away with only unsetting the C-API information in __cxa_end_catch, but you'd be
-    // wrong! Firstly, this would prohibit calling C-API functions inside a catch-block. Secondly, __cxa_end_catch is
-    // always called when leaving a catch block, even if we're leaving it by re-raising the exception. So if we store
-    // our exception info in curexc_*, and then unset these in __cxa_end_catch, then we'll wipe our exception info
-    // during unwinding!
-
-    return pyston::getExceptionStorage(pyston::beginUnwind());
+    // we begin the unwind session here rather than in __cxa_throw
+    // because we need to return the session's exception storage
+    // from this method.
+    return pyston::getPythonUnwindSessionExceptionStorage(pyston::beginPythonUnwindSession());
 }
 
 // Takes the value that resume() sent us in RAX, and returns a pointer to the exception object actually thrown. In our