[Patches] [ python-Patches-587993 ] SET_LINENO killer

noreply@sourceforge.net noreply@sourceforge.net
Wed, 27 Nov 2002 01:44:01 -0800


Patches item #587993, was opened at 2002-07-29 12:27
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=587993&group_id=5470

Category: Core (C code)
Group: Python 2.3
>Status: Closed
Resolution: Accepted
Priority: 5
Submitted By: Michael Hudson (mwh)
Assigned to: Michael Hudson (mwh)
Summary: SET_LINENO killer

Initial Comment:
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!

----------------------------------------------------------------------

>Comment By: Michael Hudson (mwh)
Date: 2002-11-27 09:44

Message:
Logged In: YES 
user_id=6656

I have! See #597919 and go pester Jeremy :)

----------------------------------------------------------------------

Comment By: Gustavo Niemeyer (niemeyer)
Date: 2002-11-26 22:45

Message:
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.


----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-30 16:03

Message:
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.

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-08-30 14:59

Message:
Logged In: YES 
user_id=6380

Thanks, Michael!

Do you think the import.c MAGIC needs to be changed again???


----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-30 14:14

Message:
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...

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-08-29 22:48

Message:
Logged In: YES 
user_id=6380

Little time to ponder here, but if we could get rid of
RETURN_NONE, that would be nice.

----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2002-08-29 21:05

Message:
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.

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-29 11:36

Message:
Logged In: YES 
user_id=6656

Sigh, *here's* the patch.

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-29 11:34

Message:
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.

----------------------------------------------------------------------

Comment By: Armin Rigo (arigo)
Date: 2002-08-28 13:46

Message:
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).

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-28 12:56

Message:
Logged In: YES 
user_id=6656

guess I must have forgotten to check the box...

----------------------------------------------------------------------

Comment By: Armin Rigo (arigo)
Date: 2002-08-28 01:27

Message:
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.

----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2002-08-28 00:21

Message:
Logged In: YES 
user_id=33168

Michael, did you forget to upload the patch?  The last patch
is from the 15th (#8).

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-27 14:15

Message:
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.



----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-27 12:09

Message:
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?

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-08-26 15:27

Message:
Logged In: YES 
user_id=6380

Reopening. I'm sure MWH can deal with these.

----------------------------------------------------------------------

Comment By: Armin Rigo (arigo)
Date: 2002-08-26 15:14

Message:
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.

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-08-15 16:16

Message:
Logged In: YES 
user_id=6380

Thanks! Woo hoo!

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-15 16:03

Message:
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


----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-08-15 15:50

Message:
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.

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-15 11:36

Message:
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...

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-15 10:23

Message:
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.

----------------------------------------------------------------------

Comment By: Martin v. Löwis (loewis)
Date: 2002-08-15 00:08

Message:
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.

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-08-14 21:16

Message:
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?

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-05 15:42

Message:
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...


----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-08-05 15:39

Message:
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... :-(

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-05 15:29

Message:
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.


----------------------------------------------------------------------

Comment By: Neil Schemenauer (nascheme)
Date: 2002-08-05 15:23

Message:
Logged In: YES 
user_id=35752

Why was the RETURN_NONE opcode added?

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-05 13:20

Message:
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.


----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2002-08-05 12:56

Message:
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. :-(

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-05 10:09

Message:
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.

----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2002-08-04 20:50

Message:
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.

----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2002-08-03 21:06

Message:
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



----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-03 16:52

Message:
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.

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-02 14:34

Message:
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

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-01 16:43

Message:
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


----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-01 16:41

Message:
Logged In: YES 
user_id=6656

And finally, docs.

This touches:

Doc/lib/libdis.tex
Doc/tut/tut.tex


----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-01 16:40

Message:
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.

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-08-01 16:38

Message:
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.

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-07-30 23:09

Message:
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. :-(

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2002-07-30 22:54

Message:
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.)

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-07-30 10:59

Message:
Logged In: YES 
user_id=6656

Guido should see this, assuming he still isn't subscribed to
patches@python.org.

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-07-30 10:58

Message:
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.

----------------------------------------------------------------------

Comment By: Tim Peters (tim_one)
Date: 2002-07-29 21:18

Message:
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).

----------------------------------------------------------------------

Comment By: Michael Hudson (mwh)
Date: 2002-07-29 16:34

Message:
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...

----------------------------------------------------------------------

Comment By: Neil Schemenauer (nascheme)
Date: 2002-07-29 16:18

Message:
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.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=587993&group_id=5470