From 31c8dbfcf45cd6bbb0f7d47c5b0e69d5948c122c Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Fri, 3 Mar 2017 06:56:21 -0500 Subject: Simplify stack management in CTracer "file_data" used to be borrowed from data, but that was confusing. Now it's owned. We used to have a struct member which was a copy of the current stack entry. That just made it harder to reason about reference counting. Now we have a pointer to the entry on the stack. --- coverage/ctracer/datastack.c | 10 ++++++- coverage/ctracer/datastack.h | 10 +++---- coverage/ctracer/tracer.c | 67 +++++++++++++++++++++++--------------------- coverage/ctracer/tracer.h | 4 +-- 4 files changed, 50 insertions(+), 41 deletions(-) diff --git a/coverage/ctracer/datastack.c b/coverage/ctracer/datastack.c index 5a384e6..515ba92 100644 --- a/coverage/ctracer/datastack.c +++ b/coverage/ctracer/datastack.c @@ -4,7 +4,7 @@ #include "util.h" #include "datastack.h" -#define STACK_DELTA 100 +#define STACK_DELTA 20 int DataStack_init(Stats *pstats, DataStack *pdata_stack) @@ -18,6 +18,11 @@ DataStack_init(Stats *pstats, DataStack *pdata_stack) void DataStack_dealloc(Stats *pstats, DataStack *pdata_stack) { + int i; + + for (i = 0; i < pdata_stack->alloc; i++) { + Py_XDECREF(pdata_stack->stack[i].file_data); + } PyMem_Free(pdata_stack->stack); } @@ -35,6 +40,9 @@ DataStack_grow(Stats *pstats, DataStack *pdata_stack) pdata_stack->depth--; return RET_ERROR; } + /* Zero the new entries. */ + memset(bigger_data_stack + pdata_stack->alloc, 0, STACK_DELTA * sizeof(DataStackEntry)); + pdata_stack->stack = bigger_data_stack; pdata_stack->alloc = bigger; } diff --git a/coverage/ctracer/datastack.h b/coverage/ctracer/datastack.h index b63af2c..b2dbeb9 100644 --- a/coverage/ctracer/datastack.h +++ b/coverage/ctracer/datastack.h @@ -9,18 +9,16 @@ /* An entry on the data stack. For each call frame, we need to record all * the information needed for CTracer_handle_line to operate as quickly as - * possible. All PyObject* here are borrowed references. + * possible. */ typedef struct DataStackEntry { - /* The current file_data dictionary. Borrowed, owned by self->data. */ + /* The current file_data dictionary. Owned. */ PyObject * file_data; - /* The disposition object for this frame. If collector.py and control.py - * are working properly, this will be an instance of CFileDisposition. - */ + /* The disposition object for this frame. A borrowed instance of CFileDisposition. */ PyObject * disposition; - /* The FileTracer handling this frame, or None if it's Python. */ + /* The FileTracer handling this frame, or None if it's Python. Borrowed. */ PyObject * file_tracer; /* The line number of the last line recorded, for tracing arcs. diff --git a/coverage/ctracer/tracer.c b/coverage/ctracer/tracer.c index a517327..daca07c 100644 --- a/coverage/ctracer/tracer.c +++ b/coverage/ctracer/tracer.c @@ -71,8 +71,6 @@ CTracer_init(CTracer *self, PyObject *args_unused, PyObject *kwds_unused) self->pdata_stack = &self->data_stack; - self->cur_entry.last_line = -1; - self->context = Py_None; Py_INCREF(self->context); @@ -169,7 +167,7 @@ showlog(int depth, int lineno, PyObject * filename, const char * msg) static const char * what_sym[] = {"CALL", "EXC ", "LINE", "RET "}; #endif -/* Record a pair of integers in self->cur_entry.file_data. */ +/* Record a pair of integers in self->pcur_entry->file_data. */ static int CTracer_record_pair(CTracer *self, int l1, int l2) { @@ -182,7 +180,7 @@ CTracer_record_pair(CTracer *self, int l1, int l2) goto error; } - if (PyDict_SetItem(self->cur_entry.file_data, t, Py_None) < 0) { + if (PyDict_SetItem(self->pcur_entry->file_data, t, Py_None) < 0) { goto error; } @@ -301,14 +299,14 @@ CTracer_check_missing_return(CTracer *self, PyFrameObject *frame) goto error; } if (self->pdata_stack->depth >= 0) { - if (self->tracing_arcs && self->cur_entry.file_data) { - if (CTracer_record_pair(self, self->cur_entry.last_line, -self->last_exc_firstlineno) < 0) { + if (self->tracing_arcs && self->pcur_entry->file_data) { + if (CTracer_record_pair(self, self->pcur_entry->last_line, -self->last_exc_firstlineno) < 0) { goto error; } } SHOWLOG(self->pdata_stack->depth, frame->f_lineno, frame->f_code->co_filename, "missedreturn"); - self->cur_entry = self->pdata_stack->stack[self->pdata_stack->depth]; self->pdata_stack->depth--; + self->pcur_entry = &self->pdata_stack->stack[self->pdata_stack->depth]; } } self->last_exc_back = NULL; @@ -352,9 +350,7 @@ CTracer_handle_call(CTracer *self, PyFrameObject *frame) if (DataStack_grow(&self->stats, self->pdata_stack) < 0) { goto error; } - - /* Push the current state on the stack. */ - self->pdata_stack->stack[self->pdata_stack->depth] = self->cur_entry; + self->pcur_entry = &self->pdata_stack->stack[self->pdata_stack->depth]; /* See if this frame begins a new context. */ if (self->should_start_context && self->context == Py_None) { @@ -370,7 +366,7 @@ CTracer_handle_call(CTracer *self, PyFrameObject *frame) PyObject * val; Py_DECREF(self->context); self->context = context; - self->cur_entry.started_context = TRUE; + self->pcur_entry->started_context = TRUE; STATS( self->stats.pycalls++; ) val = PyObject_CallFunctionObjArgs(self->switch_context, context, NULL); if (val == NULL) { @@ -380,11 +376,11 @@ CTracer_handle_call(CTracer *self, PyFrameObject *frame) } else { Py_DECREF(context); - self->cur_entry.started_context = FALSE; + self->pcur_entry->started_context = FALSE; } } else { - self->cur_entry.started_context = FALSE; + self->pcur_entry->started_context = FALSE; } /* Check if we should trace this line. */ @@ -512,7 +508,6 @@ CTracer_handle_call(CTracer *self, PyFrameObject *frame) goto error; } ret2 = PyDict_SetItem(self->data, tracename, file_data); - Py_DECREF(file_data); if (ret2 < 0) { goto error; } @@ -525,9 +520,14 @@ CTracer_handle_call(CTracer *self, PyFrameObject *frame) } } } + else { + /* PyDict_GetItem gives a borrowed reference. Own it. */ + Py_INCREF(file_data); + } - self->cur_entry.file_data = file_data; - self->cur_entry.file_tracer = file_tracer; + Py_XDECREF(self->pcur_entry->file_data); + self->pcur_entry->file_data = file_data; + self->pcur_entry->file_tracer = file_tracer; /* Make the frame right in case settrace(gettrace()) happens. */ Py_INCREF(self); @@ -535,22 +535,23 @@ CTracer_handle_call(CTracer *self, PyFrameObject *frame) SHOWLOG(self->pdata_stack->depth, frame->f_lineno, filename, "traced"); } else { - self->cur_entry.file_data = NULL; - self->cur_entry.file_tracer = Py_None; + Py_XDECREF(self->pcur_entry->file_data); + self->pcur_entry->file_data = NULL; + self->pcur_entry->file_tracer = Py_None; SHOWLOG(self->pdata_stack->depth, frame->f_lineno, filename, "skipped"); } - self->cur_entry.disposition = disposition; + self->pcur_entry->disposition = disposition; /* A call event is really a "start frame" event, and can happen for * re-entering a generator also. f_lasti is -1 for a true call, and a * real byte offset for a generator re-entry. */ if (frame->f_lasti < 0) { - self->cur_entry.last_line = -frame->f_code->co_firstlineno; + self->pcur_entry->last_line = -frame->f_code->co_firstlineno; } else { - self->cur_entry.last_line = frame->f_lineno; + self->pcur_entry->last_line = frame->f_lineno; } ok: @@ -674,22 +675,22 @@ CTracer_handle_line(CTracer *self, PyFrameObject *frame) STATS( self->stats.lines++; ) if (self->pdata_stack->depth >= 0) { SHOWLOG(self->pdata_stack->depth, frame->f_lineno, frame->f_code->co_filename, "line"); - if (self->cur_entry.file_data) { + if (self->pcur_entry->file_data) { int lineno_from = -1; int lineno_to = -1; /* We're tracing in this frame: record something. */ - if (self->cur_entry.file_tracer != Py_None) { + if (self->pcur_entry->file_tracer != Py_None) { PyObject * from_to = NULL; STATS( self->stats.pycalls++; ) - from_to = PyObject_CallMethodObjArgs(self->cur_entry.file_tracer, str_line_number_range, frame, NULL); + from_to = PyObject_CallMethodObjArgs(self->pcur_entry->file_tracer, str_line_number_range, frame, NULL); if (from_to == NULL) { goto error; } ret2 = CTracer_unpack_pair(self, from_to, &lineno_from, &lineno_to); Py_DECREF(from_to); if (ret2 < 0) { - CTracer_disable_plugin(self, self->cur_entry.disposition); + CTracer_disable_plugin(self, self->pcur_entry->disposition); goto ok; } } @@ -701,7 +702,7 @@ CTracer_handle_line(CTracer *self, PyFrameObject *frame) for (; lineno_from <= lineno_to; lineno_from++) { if (self->tracing_arcs) { /* Tracing arcs: key is (last_line,this_line). */ - if (CTracer_record_pair(self, self->cur_entry.last_line, lineno_from) < 0) { + if (CTracer_record_pair(self, self->pcur_entry->last_line, lineno_from) < 0) { goto error; } } @@ -712,14 +713,14 @@ CTracer_handle_line(CTracer *self, PyFrameObject *frame) goto error; } - ret2 = PyDict_SetItem(self->cur_entry.file_data, this_line, Py_None); + ret2 = PyDict_SetItem(self->pcur_entry->file_data, this_line, Py_None); Py_DECREF(this_line); if (ret2 < 0) { goto error; } } - self->cur_entry.last_line = lineno_from; + self->pcur_entry->last_line = lineno_from; } } } @@ -743,8 +744,10 @@ CTracer_handle_return(CTracer *self, PyFrameObject *frame) if (CTracer_set_pdata_stack(self) < 0) { goto error; } + self->pcur_entry = &self->pdata_stack->stack[self->pdata_stack->depth]; + if (self->pdata_stack->depth >= 0) { - if (self->tracing_arcs && self->cur_entry.file_data) { + if (self->tracing_arcs && self->pcur_entry->file_data) { /* Need to distinguish between RETURN_VALUE and YIELD_VALUE. Read * the current bytecode to see what it is. In unusual circumstances * (Cython code), co_code can be the empty string, so range-check @@ -759,14 +762,14 @@ CTracer_handle_return(CTracer *self, PyFrameObject *frame) } if (bytecode != YIELD_VALUE) { int first = frame->f_code->co_firstlineno; - if (CTracer_record_pair(self, self->cur_entry.last_line, -first) < 0) { + if (CTracer_record_pair(self, self->pcur_entry->last_line, -first) < 0) { goto error; } } } /* If this frame started a context, then returning from it ends the context. */ - if (self->cur_entry.started_context) { + if (self->pcur_entry->started_context) { PyObject * val; Py_DECREF(self->context); self->context = Py_None; @@ -782,8 +785,8 @@ CTracer_handle_return(CTracer *self, PyFrameObject *frame) /* Pop the stack. */ SHOWLOG(self->pdata_stack->depth, frame->f_lineno, frame->f_code->co_filename, "return"); - self->cur_entry = self->pdata_stack->stack[self->pdata_stack->depth]; self->pdata_stack->depth--; + self->pcur_entry = &self->pdata_stack->stack[self->pdata_stack->depth]; } ret = RET_OK; diff --git a/coverage/ctracer/tracer.h b/coverage/ctracer/tracer.h index 438317b..c174ae5 100644 --- a/coverage/ctracer/tracer.h +++ b/coverage/ctracer/tracer.h @@ -54,8 +54,8 @@ typedef struct CTracer { int data_stacks_used; DataStack * pdata_stack; - /* The current file's data stack entry, copied from the stack. */ - DataStackEntry cur_entry; + /* The current file's data stack entry. */ + DataStackEntry * pcur_entry; /* The parent frame for the last exception event, to fix missing returns. */ PyFrameObject * last_exc_back; -- cgit v1.2.1