Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ast compile in old debug builds of python2.x #1554

Merged
merged 1 commit into from
May 21, 2019
Merged

Fix ast compile in old debug builds of python2.x #1554

merged 1 commit into from
May 21, 2019

Conversation

asottile
Copy link
Contributor

Resolves #1553

Reproducing this was difficult, the original post mentioned 2.7.9 which I could not reproduce on -- I was however able to reproduce using 2.7.8

I used docker to do this:

(to start the container)

docker run -v $PWD:/code:ro --rm -ti ubuntu:utopic bash

Once inside, I had to fix up apt so it would download using:

sed -i 's/archive\.ubuntu\.com/old-releases.ubuntu.com/g' /etc/apt/sources.list

Then I did:

apt update && apt install python-dbg nano curl
curl https://asottile.github.io/get-virtualenv.py | python-dbg - /venv

And from there I was able to reproduce the issue(s) pretty easily.

Note that if you want to use gdb in docker, you'll need to run the container with --privileged so gdb can attach to a process

@davidism
Copy link
Member

davidism commented May 20, 2019

Thanks!

Would you explain the fix a bit? What is the difference between Load and Param? Why is fix_missing_locations unreliable now?

@davidism davidism added this to the 0.15.5 milestone May 20, 2019
@asottile
Copy link
Contributor Author

Thanks!

Would you explain the fix a bit? What is the difference between Load and Param? Why is fix_missing_locations unreliable now?

Load vs Param

the Load vs Param was an oversight on my part :)

in python2, function parameters reuse the Name that variable de-refererences use as well. Name has two parts: an id (the thing to reference) and a "context" (how that name is being used). The two most common ones are Store() and Load() (for whether the variable is being assigned to or read from). The one I forgot about was Param which is supposed to be used in the parameterset.

In the standard non-debug build of python2, this attribute is completely ignored for function arguments (because it doesn't actually do anything!). The case where it's checked is only in the debug build (by way of assert) -- this is the assert that was triggered in the original report

fix_missing_locations

This one was a little bit subtle, and I wasn't able to completely dig into why the assertion exists -- only what was causing it.

# python t.py
python: ../Python/compile.c:3600: assemble_lnotab: Assertion `d_lineno >= 0' failed.
Aborted (core dumped)

Through reading the code a bit, I noticed some subtraction of line numbers. My guess was that there was ~partial line number information contained in some newer nodes which was causing older nodes to have negative line numbers (?). I ended up verifying this by dumping the ast of the q, params function and noticing some earlier nodes had line numbers 2, 3, and 4 while some later nodes went back to line numbers 1.

This assertion only fires in debug mode and the code functions fine without "lnotab` (some sort of line cache?) 🤷‍♂

My fix here is to not care about the line numbers at all since there isn't any actual source and to set them all to (1, 0) (this is what ast.fix_missing_locations did for anything that didn't already have line information)

@DOlearczyk
Copy link

I can confirm that this resolves the original issue #1553.
Thanks!

@davidism davidism merged commit 2000892 into pallets:0.15.x May 21, 2019
@asottile asottile deleted the name_assertion_error branch May 21, 2019 20:48
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants