search

python / typed_ast

Modified fork of CPython's ast module that parses `# type:` comments
Other
227 stars 54 forks source link

ast27: prefix exported symbols #152

Closed hauntsaninja closed 3 years ago

hauntsaninja commented 3 years ago

Fixes #151 and (probably) fixes #139

Linking shenanigans caused ast27 to link against Python's asdl.c, instead of ast27/Python/asdl.c. Python's asdl.c uses Py_ssize_t for asdl_seq's size whereas ast27/Python/asdl.c uses int. This means we were interpreting memory differently in asdl.c and ast.c. On 64-bit big-endian systems, this caused us to misplace asdl.c's writes to asdl_seq->size, since Py_ssize_t is eight bytes, but int is four. The fix mirrors what we do in ast3, that is, manually namespace definitions to avoid linking conflicts.

sebix commented 3 years ago

I can confirm this fixes both the ppc64 and the s390x builds / tests that we perform in openSUSE for packaging! Thanks for your work!

JelleZijlstra commented 3 years ago

Is there a reason for the difference between typed_ast's and Python's asdl.c? Could there (shudders) be a bug in typed_ast that comes up only if you try to make a tuple with more than INT_MAX elements?

hauntsaninja commented 3 years ago

@JelleZijlstra The struct has the same declaration in ast27/Python/asdl.c as in Python 2.7's asdl.c, so if there is an issue in parsing a file with more than INT_MAX of a thing, it probably applies to Python 2.7 as well.

...That said, it looks like there is a difference in the signature of asdl_seq_new. In ast27, it's: asdl_seq *asdl_seq_new(Py_ssize_t size, PyArena *arena); whereas in Python 2.7 it's asdl_seq *asdl_seq_new(int size, PyArena *arena);. This goes back to a3f63eb6, which introduced ast27/Python/asdl.c. I'm not sure this matters, but I can change it for consistency and peaceful sleep.

Finally, looking at the macros also made me realise that ast27's ast.c currently prefers to use the macro asdl_seq_new instead of _Ta27_asdl_seq_new. On the other hand, being clever and not using obviously namespaced symbols is what caused the bug, so maybe I should get rid of the asdl_seq_new macro altogether. On the third hand, this PR has already been tested on various systems I don't have access to and so I'm reluctant to do refactoring solely for style.

Let me know!

JelleZijlstra commented 3 years ago

Thanks for the explanation! It's probably not worth it to go through any more refactoring for Python 2.7 code—let's just let 2.7 die a peaceful death.

hauntsaninja commented 3 years ago

As far as I can tell https://github.com/python/typed_ast/blob/master/release_process.md is up to date, other than missing a mention of Python 3.9 (and 3.5? setup.py claims to support it still, as does mypy, so we should probably still build a macOS wheel) :-)