Review of the C parts of the patch. http://bugs.python.org/review/13405/diff/3743/12151 File Include/code.h (right): http://bugs.python.org/review/13405/diff/3743/12151#newcode10 Include/code.h:10: #include ...
http://bugs.python.org/review/13405/diff/3743/12151
File Include/code.h (right):
http://bugs.python.org/review/13405/diff/3743/12151#newcode10
Include/code.h:10: #include "pyconfig.h"
On 2011/12/02 00:17:36, loewis wrote:
> If that's the only change to this file: why is this needed?
Done.
http://bugs.python.org/review/13405/diff/3743/12152
File Include/dynamic_annotations.h (right):
http://bugs.python.org/review/13405/diff/3743/12152#newcode328
Include/dynamic_annotations.h:328: #define _Py_ANNOTATE_RWLOCK_CREATE(lock) /*
empty */
On 2011/12/02 00:17:36, loewis wrote:
> That's white-space only changes. Please remove them from the patch.
This is because a bug in the dtrace compiler. I already patched it in mainline.
See issue #13488.
This "diff" will vanish when I merge from mainline,
http://bugs.python.org/review/13405/diff/3743/12153
File Include/frameobject.h (right):
http://bugs.python.org/review/13405/diff/3743/12153#newcode49
Include/frameobject.h:49: int f_calllineno; /* line number of call site */
On 2011/12/02 00:17:36, loewis wrote:
> How is this different from f_lineno?
I didn't want to mess with the "instr_lb", "instr_prev", etc., logic, moreover
when my code is conditionally compiled. Also the previous comment pushed me to
not depend of that field.
Anyway this code will dissaperar in the next iteration of the patch, when I get
rid of the performance hit when probes are compiled in but disabled (that is,
99.9999% of the time :).
http://bugs.python.org/review/13405/diff/3743/12159
File Makefile.pre.in (right):
http://bugs.python.org/review/13405/diff/3743/12159#newcode683
Makefile.pre.in:683: if test "$(DTRACE)" != "" ; then \
On 2011/12/02 00:17:36, loewis wrote:
> Why is it necessary to have this test? phelper.o won't be built if DTRACEOBJS
is
> empty, right?
I want to move all this code to conditional execution. The build instructions
now are a bit fragile.
http://bugs.python.org/review/13405/diff/3743/12160
File Modules/dtracemodule.c (right):
http://bugs.python.org/review/13405/diff/3743/12160#newcode33
Modules/dtracemodule.c:33: if (PyModule_AddObject(m, "available", v) < 0) {
On 2011/12/02 00:17:36, loewis wrote:
> Having a module for this looks like overkill. Adding it to sys.flags should be
> sufficient.
I kind of agree, but I am thinking about providing explicit dtrace abilities to
python scripts. That is, that python scripts can communicate with external
dtrace environment. Kind of logging module on steroids.
For this I need a placeholder, so the module.
I will think about it. Thanks for pointing out.
http://bugs.python.org/review/13405/diff/3743/12161
File Modules/gcmodule.c (right):
http://bugs.python.org/review/13405/diff/3743/12161#newcode990
Modules/gcmodule.c:990: Py_ssize_t value;
On 2011/12/02 00:17:36, loewis wrote:
> incorrect indent.
Done.
http://bugs.python.org/review/13405/diff/3743/12162
File Objects/codeobject.c (right):
http://bugs.python.org/review/13405/diff/3743/12162#newcode141
Objects/codeobject.c:141:
On 2011/12/02 00:17:36, loewis wrote:
> whitespace change
Cognitive help. Related code blocked together.
http://bugs.python.org/review/13405/diff/3743/12162#newcode151
Objects/codeobject.c:151: PyUnicode_AsUTF8(filename);
On 2011/12/02 00:17:37, loewis wrote:
> Is this really necessary? There must be a better way.
>
> If it is necessary, consider that it may fail.
It is needed for the dtrace stack helper. It is called from kernel space and
can't call any python function.
This code makes the UTF8 representation available to the helper, if necessary,
running in kernel space.
If these functions fail, the python code will work normally and we only lose the
augmented stack dump, if necessary. Nothing will crash, neither the dtrace
script. It will simply show the C stack dump, with no python annotations.
http://bugs.python.org/review/13405/diff/3743/12164
File Objects/typeobject.c (right):
http://bugs.python.org/review/13405/diff/3743/12164#newcode724
Objects/typeobject.c:724: mod_name = PyUnicode_AsUTF8(mod);
On 2011/12/02 00:17:37, loewis wrote:
> Consider that this may fail.
Even if that happens, the DTrace script will not crash. DTrace magic :-).
But better play safe. Good catch.
http://bugs.python.org/review/13405/diff/3743/12165
File Python/ceval.c (right):
http://bugs.python.org/review/13405/diff/3743/12165#newcode580
Python/ceval.c:580: arg = NULL;
On 2011/12/02 00:17:37, loewis wrote:
> arg is already NULL here.
Done.
http://bugs.python.org/review/13405/diff/3743/12165#newcode800
Python/ceval.c:800: lineno = PyCode_Addr2Line(f->f_code, f->f_lasti);
On 2011/12/02 00:17:37, loewis wrote:
> indentation error.
Done.
http://bugs.python.org/review/13405/diff/3743/12165#newcode1049
Python/ceval.c:1049: if (!lltrace && !_Py_TracingPossible) &&
!PYTHON_LINE_ENABLED()) { \
On 2011/12/02 00:17:37, loewis wrote:
> Is it possible to rename the prefix of these to either Py_XXX or PyDtrace_XXX?
Not easily.
The macros are generated automatically by dtrace framework. We could run a macro
over the macros, but seems a bit overkill.
We are balancing here python conventions with dtrace conventions.
http://bugs.python.org/review/13405/diff/3743/12166
File Python/sysmodule.c (right):
http://bugs.python.org/review/13405/diff/3743/12166#newcode1507
Python/sysmodule.c:1507:
On 2011/12/02 00:17:37, loewis wrote:
> whitespace change
Done.
