Use native traceback mutation on python 3.7

Since python/cpython#4793 was merged, it looks like traceback objects will support Python-level instantiation and mutation in 3.7. We should use that instead of the current ctypes horrors.

We should also hassle the PyPy devs about implementing this. (In some ways this is even more interesting, because we're more aggressive about dropping support for old PyPy versions.)

We should also hassle the PyPy devs about implementing this.

Is there an assumption that they won't make this assignable when they support 3.7 even though it's documented now?

I'm currently fixing Jinja's traceback rewriting due to the changes in 3.7 and an issue with ctypes on debug builds (pallets/jinja#1050, pallets/jinja#1051, pallets/jinja#1104), and it would be nice if I can assume any 3.7 means tb_next is assignable. PyPy's docs for tproxy now list it as deprecated.

@mattip

Another argument in favor of this is that we're currently using PyPy transparent proxies for concat_tb:

trio/trio/_core/_multierror.py

Lines 249 to 356 in 4f65ff2

    
           ################################################################ 
        
           # concat_tb 
        
           ################################################################ 
        
           # We need to compute a new traceback that is the concatenation of two existing 
        
           # tracebacks. This requires copying the entries in 'head' and then pointing 
        
           # the final tb_next to 'tail'. 
        
           # 
        
           # NB: 'tail' might be None, which requires some special handling in the ctypes 
        
           # version. 
        
           # 
        
           # The complication here is that Python doesn't actually support copying or 
        
           # modifying traceback objects, so we have to get creative... 
        
           # 
        
           # On CPython, we use ctypes. On PyPy, we use "transparent proxies". 
        
           # 
        
           # Jinja2 is a useful source of inspiration: 
        
           #   https://github.com/pallets/jinja/blob/master/jinja2/debug.py 
        
           try: 
        
               import tputil 
        
           except ImportError: 
        
               have_tproxy = False 
        
           else: 
        
               have_tproxy = True 
        
           if have_tproxy: 
        
               # http://doc.pypy.org/en/latest/objspace-proxies.html 
        
               def copy_tb(base_tb, tb_next): 
        
                   def controller(operation): 
        
                       # Rationale for pragma: I looked fairly carefully and tried a few 
        
                       # things, and AFAICT it's not actually possible to get any 
        
                       # 'opname' that isn't __getattr__ or __getattribute__. So there's 
        
                       # no missing test we could add, and no value in coverage nagging 
        
                       # us about adding one. 
        
                       if operation.opname in [ 
        
                           "__getattribute__", "__getattr__" 
        
                       ]:  # pragma: no cover 
        
                           if operation.args[0] == "tb_next": 
        
                               return tb_next 
        
                       return operation.delegate() 
        
                   return tputil.make_proxy(controller, type(base_tb), base_tb) 
        
           else: 
        
               # ctypes it is 
        
               import ctypes 
        
               # How to handle refcounting? I don't want to use ctypes.py_object because 
        
               # I don't understand or trust it, and I don't want to use 
        
               # ctypes.pythonapi.Py_{Inc,Dec}Ref because we might clash with user code 
        
               # that also tries to use them but with different types. So private _ctypes 
        
               # APIs it is! 
        
               import _ctypes 
        
               class CTraceback(ctypes.Structure): 
        
                   _fields_ = [ 
        
                       ("PyObject_HEAD", ctypes.c_byte * object().__sizeof__()), 
        
                       ("tb_next", ctypes.c_void_p), 
        
                       ("tb_frame", ctypes.c_void_p), 
        
                       ("tb_lasti", ctypes.c_int), 
        
                       ("tb_lineno", ctypes.c_int), 
        
                   ] 
        
               def copy_tb(base_tb, tb_next): 
        
                   # TracebackType has no public constructor, so allocate one the hard way 
        
                   try: 
        
                       raise ValueError 
        
                   except ValueError as exc: 
        
                       new_tb = exc.__traceback__ 
        
                   c_new_tb = CTraceback.from_address(id(new_tb)) 
        
                   # At the C level, tb_next either pointer to the next traceback or is 
        
                   # NULL. c_void_p and the .tb_next accessor both convert NULL to None, 
        
                   # but we shouldn't DECREF None just because we assigned to a NULL 
        
                   # pointer! Here we know that our new traceback has only 1 frame in it, 
        
                   # so we can assume the tb_next field is NULL. 
        
                   assert c_new_tb.tb_next is None 
        
                   # If tb_next is None, then we want to set c_new_tb.tb_next to NULL, 
        
                   # which it already is, so we're done. Otherwise, we have to actually 
        
                   # do some work: 
        
                   if tb_next is not None: 
        
                       _ctypes.Py_INCREF(tb_next) 
        
                       c_new_tb.tb_next = id(tb_next) 
        
                   assert c_new_tb.tb_frame is not None 
        
                   _ctypes.Py_INCREF(base_tb.tb_frame) 
        
                   old_tb_frame = new_tb.tb_frame 
        
                   c_new_tb.tb_frame = id(base_tb.tb_frame) 
        
                   _ctypes.Py_DECREF(old_tb_frame) 
        
                   c_new_tb.tb_lasti = base_tb.tb_lasti 
        
                   c_new_tb.tb_lineno = base_tb.tb_lineno 
        
                   return new_tb 
        
           def concat_tb(head, tail): 
        
               # We have to use an iterative algorithm here, because in the worst case 
        
               # this might be a RecursionError stack that is by definition too deep to 
        
               # process by recursion! 
        
               head_tbs = [] 
        
               pointer = head 
        
               while pointer is not None: 
        
                   head_tbs.append(pointer) 
        
                   pointer = pointer.tb_next 
        
               current_head = tail 
        
               for head_tb in reversed(head_tbs): 
        
                   current_head = copy_tb(head_tb, tb_next=current_head) 
        
               return current_head

They're documented and Trio uses them since 2017-02. However, in 2018-05, @mattip marked them as deprecated. It does not look like they're planning to remove them right now, but migrating away from tputil is probably still desirable.

@davidism

@davidism Hey David, sorry I missed your message.

Is there an assumption that they won't make this assignable when they support 3.7 even though it's documented now?

They probably will eventually, since the CPython 3.7 test suite has tests for it, and PyPy will eventually import those tests when adding 3.7 support. But they might do it quicker if we hassle them :-)

When I talked to the PyPy devs about this originally, they told me they knew that jinja2 depends on the deprecated "transparent proxies" code, and they weren't planning to break jinja2, so even though it was deprecated I could expect at least this specific case to keep working. That's why I felt OK using it in Trio :-). But I'm sure they wouldn't mind if we all switched to something more sensible.

I'm currently fixing Jinja's traceback rewriting due to the changes in 3.7 and an issue with ctypes on debug builds (pallets/jinja#1050, pallets/jinja#1051, pallets/jinja#1104), and it would be nice if I can assume any 3.7 means tb_next is assignable. PyPy's docs for tproxy now list it as deprecated.

Oh yeah, uh... I actually fixed this already when I was cribbing from jinja2. I probably should have sent you a note at some point, sorry :-). It's actually very simple to fix, though it needs some explanation:

trio/trio/_core/_multierror.py

Lines 303 to 304 in ae557cc

    
           _fields_ = [ 
        
               ("PyObject_HEAD", ctypes.c_byte * object().__sizeof__()),

The idea is that we know a Python object is just a bare PyObject_HEADER without any additional fields, so whatever size the object struct is, that's the size of a PyObject_HEADER. And if you don't care about accessing the fields inside it (refcount etc.), then you don't need to actually spell out those fields for ctypes; you just need a blob of "something" the right size so ctypes can figure out where the subsequent fields you actually care about are located. So if we tell ctypes that the first field is a byte array with object().__sizeof__() elements in it, then everything Just Works, on every Python version. And you even get to delete the gross hacks that try to guess how CPython is compiled by looking for sys.getobjects or whatever.

@arigo

Ping @arigo

Thanks for the explanation. I was scared off by the use of _ctypes, so I ended up using py_object and pythonapi, since it was just as opaque to me as everything else, and at least seemed like the "intended" way. Here's the implementation I ended up with: https://github.com/pallets/jinja/blob/e08dadd220563609a67bc32d4d1c0abe91699231/src/jinja2/debug.py#L238. Support for 3.7+ and pypy is above that. I was able to greatly simplify the entire traceback rewriting implementation.

Navigation Menu

Repository navigation

Use native traceback mutation on python 3.7 #405

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	################################################################
	# concat_tb
	################################################################

	# We need to compute a new traceback that is the concatenation of two existing
	# tracebacks. This requires copying the entries in 'head' and then pointing
	# the final tb_next to 'tail'.
	#
	# NB: 'tail' might be None, which requires some special handling in the ctypes
	# version.
	#
	# The complication here is that Python doesn't actually support copying or
	# modifying traceback objects, so we have to get creative...
	#
	# On CPython, we use ctypes. On PyPy, we use "transparent proxies".
	#
	# Jinja2 is a useful source of inspiration:
	# https://github.com/pallets/jinja/blob/master/jinja2/debug.py

	try:
	import tputil
	except ImportError:
	have_tproxy = False
	else:
	have_tproxy = True

	if have_tproxy:
	# http://doc.pypy.org/en/latest/objspace-proxies.html
	def copy_tb(base_tb, tb_next):
	def controller(operation):
	# Rationale for pragma: I looked fairly carefully and tried a few
	# things, and AFAICT it's not actually possible to get any
	# 'opname' that isn't __getattr__ or __getattribute__. So there's
	# no missing test we could add, and no value in coverage nagging
	# us about adding one.
	if operation.opname in [
	"__getattribute__", "__getattr__"
	]: # pragma: no cover
	if operation.args[0] == "tb_next":
	return tb_next
	return operation.delegate()

	return tputil.make_proxy(controller, type(base_tb), base_tb)
	else:
	# ctypes it is
	import ctypes
	# How to handle refcounting? I don't want to use ctypes.py_object because
	# I don't understand or trust it, and I don't want to use
	# ctypes.pythonapi.Py_{Inc,Dec}Ref because we might clash with user code
	# that also tries to use them but with different types. So private _ctypes
	# APIs it is!
	import _ctypes

	class CTraceback(ctypes.Structure):
	_fields_ = [
	("PyObject_HEAD", ctypes.c_byte * object().__sizeof__()),
	("tb_next", ctypes.c_void_p),
	("tb_frame", ctypes.c_void_p),
	("tb_lasti", ctypes.c_int),
	("tb_lineno", ctypes.c_int),
	]

	def copy_tb(base_tb, tb_next):
	# TracebackType has no public constructor, so allocate one the hard way
	try:
	raise ValueError
	except ValueError as exc:
	new_tb = exc.__traceback__
	c_new_tb = CTraceback.from_address(id(new_tb))

	# At the C level, tb_next either pointer to the next traceback or is
	# NULL. c_void_p and the .tb_next accessor both convert NULL to None,
	# but we shouldn't DECREF None just because we assigned to a NULL
	# pointer! Here we know that our new traceback has only 1 frame in it,
	# so we can assume the tb_next field is NULL.
	assert c_new_tb.tb_next is None
	# If tb_next is None, then we want to set c_new_tb.tb_next to NULL,
	# which it already is, so we're done. Otherwise, we have to actually
	# do some work:
	if tb_next is not None:
	_ctypes.Py_INCREF(tb_next)
	c_new_tb.tb_next = id(tb_next)

	assert c_new_tb.tb_frame is not None
	_ctypes.Py_INCREF(base_tb.tb_frame)
	old_tb_frame = new_tb.tb_frame
	c_new_tb.tb_frame = id(base_tb.tb_frame)
	_ctypes.Py_DECREF(old_tb_frame)

	c_new_tb.tb_lasti = base_tb.tb_lasti
	c_new_tb.tb_lineno = base_tb.tb_lineno

	return new_tb


	def concat_tb(head, tail):
	# We have to use an iterative algorithm here, because in the worst case
	# this might be a RecursionError stack that is by definition too deep to
	# process by recursion!
	head_tbs = []
	pointer = head
	while pointer is not None:
	head_tbs.append(pointer)
	pointer = pointer.tb_next
	current_head = tail
	for head_tb in reversed(head_tbs):
	current_head = copy_tb(head_tb, tb_next=current_head)
	return current_head

Use native traceback mutation on python 3.7 #405

Description

Activity

davidism commented on Nov 27, 2019

pquentin commented on Feb 6, 2020

njsmith commented on Feb 6, 2020

mattip commented on Feb 6, 2020

davidism commented on Feb 6, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions