Issue587993
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2002-07-29 11:27 by mwh, last changed 2022-04-10 16:05 by admin. This issue is now closed.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | |
setlineno-7.diff | mwh, 2002-08-05 12:20 | another update! | ||
setlineno-8.diff | mwh, 2002-08-15 10:36 | update 2002-08-15 | ||
setlineno-fixup-1.diff | mwh, 2002-08-28 11:56 | first fixup attempt | ||
setlineno-fixup-2.diff | mwh, 2002-08-29 10:36 | fixup 2 |
Messages (47) | |||
---|---|---|---|
msg40679 - (view) | Author: Michael Hudson (mwh) | Date: 2002-07-29 11:27 | |
This patch is a proof-of-concept of another way to remove the SET_LINENO patch (as opposed to Vladimir's ancient one). Instead of rewriting bytecode (ick!) we poke into the c_lnotab to see if we've moved onto a different line. The c_lnotab is not the most transparent of data structures, it has to be said. I'm not sure this patch is 100% correct -- but I think the idea can definitely fly. There will be some more overhead to tracing than before, but I hope not too much. I haven't tested these aspects. Comments welcome! |
|||
msg40680 - (view) | Author: Neil Schemenauer (nascheme) * | Date: 2002-07-29 15:18 | |
Logged In: YES user_id=35752 Moving the "int io, instr_ub = -1, instr_lb = 0;" declaration and the "io = INSTR_OFFSET();"| statement below the "if (tstate-c_tracefunc ...)" test gives a small speedup on my machine and is a little neater, IMHO. I was worried that this would slow down "python -O". That doesn't seem to be the case (at least I can't measure it). Well done. |
|||
msg40681 - (view) | Author: Michael Hudson (mwh) | Date: 2002-07-29 15:34 | |
Logged In: YES user_id=6656 Uhh, the instr_[lu]b variables need to keep their values around the loop; otherwise we might just as well call PyCode_Addr2Line each time around. I have another version of the patch that does that, but I assumed the overhead of doing so was deemed too high, or someone else would have done this by now. It's certainly easier. Glad to hear it doesn't affect python -O too much. I was doing this away from the internet and forgot to keep a clean copy of the source around for doing comparisons with... |
|||
msg40682 - (view) | Author: Tim Peters (tim.peters) * | Date: 2002-07-29 20:18 | |
Logged In: YES user_id=31435 Dropping out of "vacation mode" long enough to say "mondo cool!" and encourage this. Guido may not agree, but I also encourage you to redefine c_lnotab if it can make life easier and quicker here. That subtle compression scheme has been the source of several nasty bugs, both in the core C code and in Jeremy's compiler pkg (cut 'n paste bugs abound here, because few people understand what's really needed, so flawed code gets copied with little thought). |
|||
msg40683 - (view) | Author: Michael Hudson (mwh) | Date: 2002-07-30 09:58 | |
Logged In: YES user_id=6656 I worked out why some of the code in ceval.c wasn't making sense to me -- it didn't make sense, period. I've also fixed a number of silly and not so silly bugs in my patch. I'm now 99% certain this idea can fly. The patch isn't *finished* but the hard bit is done, IMHO. There are some other points to make, but I think I'll raise them on python-dev. |
|||
msg40684 - (view) | Author: Michael Hudson (mwh) | Date: 2002-07-30 09:59 | |
Logged In: YES user_id=6656 Guido should see this, assuming he still isn't subscribed to patches@python.org. |
|||
msg40685 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-07-30 21:54 | |
Logged In: YES user_id=6380 Cool idea, but I get "unknown opcode" errors... Keep me posted though! (I wil ll now see any changes to this patch item.) |
|||
msg40686 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-07-30 22:09 | |
Logged In: YES user_id=6380 Never mind the errors, I hadn't done a cvs update in weeks on the machine where I tested it. :-( |
|||
msg40687 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-01 15:38 | |
Logged In: YES user_id=6656 Yeah, the obvious response to that was "delete your .pycs"... Right, I'm going to upload my latest, and hopefully final patch in three bits. First, I'm attaching the "interesting" patch. This touches: Objects/frameobject.c -- adds f_lineno descr Python/ceval.c -- the obvious Python/compile.c -- don't emit SET_LINENO Lib/dis.py -- use c_lnotab for line numbers Tools/scripts/trace.py -- ditto This is the most subtle patch, and the one that I'd most like review on. |
|||
msg40688 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-01 15:40 | |
Logged In: YES user_id=6656 Here's the "boring" patch, more mechanical stuff. It touches: Include/opcode.h Lib/traceback.py Modules/_hotshot.c Python/frozen.c Python/traceback.c This should still be reviewed, of course, but I really shouldn't have messed any of this up. |
|||
msg40689 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-01 15:41 | |
Logged In: YES user_id=6656 And finally, docs. This touches: Doc/lib/libdis.tex Doc/tut/tut.tex |
|||
msg40690 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-01 15:43 | |
Logged In: YES user_id=6656 Hang on, lets get all the doc changes: Doc/lib/libdis.tex Doc/tut/tut.tex Doc/whatsnew/whatsnew23.tex Misc/NEWS |
|||
msg40691 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-02 13:34 | |
Logged In: YES user_id=6656 Here's an update (just for ceval.c): - moves tracing code out of line - more expository comments - don't cause line trace events in the function epilogue |
|||
msg40692 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-03 15:52 | |
Logged In: YES user_id=6656 Update. New this time: - adds RETURN_NONE opcode for the epilgoue. - don't call line trace events on it. - bumps MAGIC this is the output of "cvs diff" at the top level, so it's all in there. |
|||
msg40693 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2002-08-03 20:06 | |
Logged In: YES user_id=33168 Overall, the patch looks very good. It would be nice to check in some of the changes separate from this patch: * com_addoparg(SET_LINENO) -> com_set_lineno() * the updated comments and whitespace * pystone * dis.py, disassemble(), perhaps * RETURN_NONE, perhaps Here's other minor comments: * Need to add RETURN_NONE opcode into dis.py * Need to add doc for RETURN_NONE in libdis.tex * It would be nice if the code from inspect.getlineno could be reused in disassemble() (not sure this can be done, though--i saw at least 4 variations of this code, 2 in python and 2 in C) * Should frame.f_lineno be initialized to 0, -1 or something invalid? * scripts/trace.py reimplements getting the lineno, why not reuse code? * docstring in scripts/trace._find_LINENO_from_code needs to remove ref to SET_LINENO |
|||
msg40694 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2002-08-04 19:50 | |
Logged In: YES user_id=33168 Attached is a patch to test_hotshot which is necessary because the duplicate line #s at the beginning of the function no longer exist. WIth this patch test_hotshot passes. |
|||
msg40695 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-05 09:09 | |
Logged In: YES user_id=6656 Thanks for the comments, Neal! I'm not sure it's possible to separate the changes in the way you describe. For instance, com_addoparg -> com_set_lineno stops SET_LINENO being generated, so breaks tracing without the VM changes, but the VM changes make SET_LINENO into an unknown opcode... I didn't intend to upload the pystone change. About the comments: - the last little RETURN_NONE changes are easy. Thanks for the pointer. - agree there are too many bits of code grovelling co_lnotab. however, it's not clear that they can easily be refactored. maybe a generator would be useful... hmm. let me think about this one. - did I take out the initialisation of frame.f_lineno entirely? oops. probably set it to co_firstlineno. - fixed trace.py docstring. reusing code not all that easy, because different uses have subtly different requirements. hmm, inspect.getlineno is now pointless... was there any particular reason you unassigned the patch? Just curious as the main reason it was assigned to Guido was to make sure he saw it. |
|||
msg40696 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2002-08-05 11:56 | |
Logged In: YES user_id=33168 Re-assigning to Guido. I think what happened was I was reviewing the patch before it was assigned to Guido. Sorry about that. You're right about the set_lineno, I thought the opcode was also generated in there. I agree about the code reuse--it's probably too hard. If I had a better idea, I would have shared it. :-( |
|||
msg40697 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-05 12:20 | |
Logged In: YES user_id=6656 Here's another update. I've also deleted the old patches. Changes this time: + doesn't include pystone LOOPS boost + finish RETURN_NONE off: - add to dis.py - add to libdis.tex + removed now-unecessary co_lnotab grovelling in inspect and traceback. left the functions for compatibility. + removed the WE_WANT_THE_HACK hack + incudes Neal's test_hotshot patch + more trace.py tweaks. Lib/compiler is still broken. |
|||
msg40698 - (view) | Author: Neil Schemenauer (nascheme) * | Date: 2002-08-05 14:23 | |
Logged In: YES user_id=35752 Why was the RETURN_NONE opcode added? |
|||
msg40699 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-05 14:29 | |
Logged In: YES user_id=6656 It was discuessed on python-dev: http://mail.python.org/pipermail/python-dev/2002-August/027261.html and followups. Adding a new opcode was Guido's idea, but I'm not totally sure this is what he meant. Also see the comments around line 2933 of ceval.c. |
|||
msg40700 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-08-05 14:39 | |
Logged In: YES user_id=6380 That's pretty much what I suggested, yes. I'll have to take more time to review all this code... :-( |
|||
msg40701 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-05 14:42 | |
Logged In: YES user_id=6656 That's good to know :) No great hurry with the review. I wanted to finish the patch while everything was still fresh in my mind, and I think I've done this now. Of course, I've said that before... |
|||
msg40702 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-08-14 20:16 | |
Logged In: YES user_id=6380 Looks good to me. Michael, why don't you hang onto it for another day and then check it in unless someone speaks out against it? |
|||
msg40703 - (view) | Author: Martin v. Löwis (loewis) * | Date: 2002-08-14 23:08 | |
Logged In: YES user_id=21627 Random comments: - whatsnew23.tex: replace VERSION - ceval.c; lltrace: Why does it drop the comparison with NULL: lltrace is int, not PyObject* - it appears that RETURN_NONE does more than just returning None; it also suppresses trace calls. This should be carefully documented, or else somebody might suggest to generate RETURN_NONE for a plain return statement. |
|||
msg40704 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-15 09:23 | |
Logged In: YES user_id=6656 Yeah, the whatsnew section needs expanding. I'll get to this, but it probably shouldn't hold up the rest of the patch. ceval.c: oversight. Good spot. There's an attempt at explaining the restrictions on RETURN_NONE in opcode.h, but it's very short. I'll expand the comments in maybe_call_line_trace & and pointers to this in the dis docs and other places. |
|||
msg40705 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-15 10:36 | |
Logged In: YES user_id=6656 OK, update. ceval.c: rewrote the comments about the exceptions for POP_TOP and RETURN_NONE somewhat. Fixed up the lltrace test. compile.c: use RETURN_NONE a touch more freely. Remove a com_pop that shouldn't have been there anymore. whatsnew23.tex: expanded, clarified, moved from "Other Language Changes" to "Other Changes And Fixes". Not sure this is the right section either... opcode.h, libdis.tex: point people interested in RETURN_NONE to the comments in ceval.c. It will be nice to stop having to de-conflict Misc/NEWS every day... |
|||
msg40706 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-08-15 14:50 | |
Logged In: YES user_id=6380 Michael, please check this in. We can perfect is more easily when it's in CVS, and it'll get more eyeballs that way too. What's here looks very good to me. |
|||
msg40707 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-15 15:03 | |
Logged In: YES user_id=6656 Checked in as: Doc/lib/libdis.tex revision 1.39 Doc/lib/libtraceback.tex revision 1.16 Doc/tut/tut.tex revision 1.170 Doc/whatsnew/whatsnew23.tex revision 1.45 Include/opcode.h revision 2.40 Lib/dis.py revision 1.42 Lib/inspect.py revision 1.38 Lib/pdb.py revision 1.55 Lib/traceback.py revision 1.28 Lib/test/test_hotshot.py revision 1.12 Misc/NEWS revision 1.470 Modules/_hotshot.c revision 1.26 Objects/frameobject.c revision 2.64 Python/ceval.c revision 2.324 Python/compile.c revision 2.258 Python/frozen.c revision 1.13 Python/import.c revision 2.209 Python/traceback.c revision 2.39 Tools/scripts/trace.py revision 1.8 |
|||
msg40708 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-08-15 15:16 | |
Logged In: YES user_id=6380 Thanks! Woo hoo! |
|||
msg40709 - (view) | Author: Armin Rigo (arigo) * | Date: 2002-08-26 14:14 | |
Logged In: YES user_id=4771 Two bugs. Try single-stepping in the following code: x = 5 del x while 0: bla print "done." It skips the "del x" line and stops on the "bla" line. The former is caused by a simple typo in maybe_call_trace_line() where the test 'frame->f_lasti > *instr_ub' should be '>=', skipping altogether lines corresponding to a single opcode. The origin of the 2nd bug is that the POP_TOP and RETURN_NONE special-casing trick is limited; there are situations where the compiler emits other "trailing" opcodes as well. The above example can be solved by adding POP_BLOCK to the list. However I would claim that a more long-term-reliable solution is needed. A close emulation of what SET_LINENO used to do could easily be obtained by discarding any line change event which jumps directly in the middle of a line. After all, no SET_LINENO used to be detected in this case either. (This would make RETURN_NONE's purpose obsolete.) Finally, the running condition of first 'while' loop in maybe_call_line_trace() should not be 'size>=0' but 'size>0'. Just ask and I will submit a patch for all this. Also, I don't understand yet why it stops twice on the first line after a 'pdb.run("import spam")'. In Python 2.1 it also stops an extra time but first on line number 0, which is slightly less disturbing -- but it stops twice on the 'while 0'. Go figure. |
|||
msg40710 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-08-26 14:27 | |
Logged In: YES user_id=6380 Reopening. I'm sure MWH can deal with these. |
|||
msg40711 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-27 11:09 | |
Logged In: YES user_id=6656 Argh. Don't know how the bounds check typo crept in; I'm sure that was right in earlier incarnations of the patch. Easy to fix, though. Wrt the exceptions list: yes, I too would like a better solution. There are situations where execution jumps into the middle of a line; for loops for one (maybe the only one). Look at the output of dis.py (either before or after this patch). Currently, the line beginning a for loop gets two "co_lnotab blocks" (if that makes sense) which could be used to get this right; we only call the trace function if the current offset exactly matches a byte increment in co_lnotab. That's spectacularly subtle, though. I'm afriad the last comment doesn't make any sense to me. Can you elaborate? What's in spam.py? |
|||
msg40712 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-27 13:15 | |
Logged In: YES user_id=6656 Attached a patch. This removes the need for RETURN_NONE, implementing a scheme somewhat like Armin suggests. Needs more expository comments, and probably some refactoring. Also adds a test case. Armin, why do you say size >= 0 should be size > 0? Things stop working if I make that change. |
|||
msg40713 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2002-08-27 23:21 | |
Logged In: YES user_id=33168 Michael, did you forget to upload the patch? The last patch is from the 15th (#8). |
|||
msg40714 - (view) | Author: Armin Rigo (arigo) * | Date: 2002-08-28 00:27 | |
Logged In: YES user_id=4771 mwh: "spam.py" was the 5 lines of code above in the message. The weird thing about the while(size>=0) loop is that it may be executed 'size+1' times, as stated. There is an off-by-one bug somewhere. If it works like this, it must obscurely rely on something like Python string arrays being always terminated by an extra NULL character. |
|||
msg40715 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-28 11:56 | |
Logged In: YES user_id=6656 guess I must have forgotten to check the box... |
|||
msg40716 - (view) | Author: Armin Rigo (arigo) * | Date: 2002-08-28 12:46 | |
Logged In: YES user_id=4771 Michael, I meant the other while loop! The *first* one. I have no objection about the second loop. A minor comment about your patch: your extra variable "trace_called" looks like an indirect way to say that we must only invoke call_trace() if after the loop we have (frame->f_lasti==*instr_lb). |
|||
msg40717 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-29 10:34 | |
Logged In: YES user_id=6656 Thanks for the comments! I was being dumb. Patch attached, though I don't really know why; I'm confident this is right. Now, do we rip RETURN_NONE out again? That's one for Guido, I think. |
|||
msg40718 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-29 10:36 | |
Logged In: YES user_id=6656 Sigh, *here's* the patch. |
|||
msg40719 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2002-08-29 20:05 | |
Logged In: YES user_id=33168 The test doesn't need to import pprint or os. I'd like to see RETURN_NONE removed, it seemed like a hack, and if it's not necessary... Unrelated to the changes in the patch, could/should the PyStrring_AsString and _Size be changed to the macro versions? The return values aren't checked and it should be faster. Although this code is only executed when tracing. |
|||
msg40720 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-08-29 21:48 | |
Logged In: YES user_id=6380 Little time to ponder here, but if we could get rid of RETURN_NONE, that would be nice. |
|||
msg40721 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-30 13:14 | |
Logged In: YES user_id=6656 Right, RETURN_NONE is gone again. Fixes checked in as Doc/lib/libdis.tex revision 1.40 Doc/whatsnew/whatsnew23.tex revision 1.49 Include/opcode.h revision 2.41 Lib/dis.py revision 1.44 Lib/test/test_trace.py revision 1.1 Python/ceval.c revision 2.333 Python/compile.c revision 2.263 Thanks for the comments! Closing again... |
|||
msg40722 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2002-08-30 13:59 | |
Logged In: YES user_id=6380 Thanks, Michael! Do you think the import.c MAGIC needs to be changed again??? |
|||
msg40723 - (view) | Author: Michael Hudson (mwh) | Date: 2002-08-30 15:03 | |
Logged In: YES user_id=6656 In a word, no. Just run find $(srcdir) -name '*.pyc" | xargs rm and be done with it. You can change it if you care. |
|||
msg40724 - (view) | Author: Gustavo Niemeyer (niemeyer) * | Date: 2002-11-26 22:45 | |
Logged In: YES user_id=7887 Michael, could you please have a look at the compiler package? It's currently broken because it still using SET_LINENO. |
|||
msg40725 - (view) | Author: Michael Hudson (mwh) | Date: 2002-11-27 09:44 | |
Logged In: YES user_id=6656 I have! See #597919 and go pester Jeremy :) |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-10 16:05:32 | admin | set | github: 36946 |
2002-07-29 11:27:43 | mwh | create |