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

Revert "bpo-43693: Add the MAKE_CELL opcode and interleave fast locals offsets. (gh-26396)" #26597

Merged
merged 1 commit into from
Jun 8, 2021

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Jun 8, 2021

This reverts commit 631f993 as it has broken the ASAN buildbots:

https://buildbot.python.org/all/#/builders/585

https://bugs.python.org/issue43693

@pablogsal
Copy link
Member Author

CC: @ericsnowcurrently @markshannon I have tried to find the error myself and debug this issue for 30 min, but the patch in 631f993 is quite involved and I need to catch up with the rest of changes made to the compiler so I was not able to find it quickly :(

@pablogsal
Copy link
Member Author

The problem is that PyFrame_LocalsToFast(frame, 1); calls into PyFrame_LocalsToFast where co->co_nlocals has a value of 3, and this causes co->co_cell2arg[i - co->co_nlocals]; to access outside the bounds of the array.

@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 8, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 01ff3e0 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 8, 2021
@pablogsal pablogsal merged commit 3fe921c into python:main Jun 8, 2021
@pablogsal pablogsal deleted the bpo-43693 branch June 8, 2021 12:17
@ericsnowcurrently
Copy link
Member

@pablogsal, sorry to impose again! I was watching the buildbots after but ran out of time to wait for them all to finish. Lesson learned.

@pablogsal
Copy link
Member Author

@pablogsal, sorry to impose again! I was watching the buildbots after but ran out of time to wait for them all to finish. Lesson learned.

No problem! I will try to improve the experience a bit better if I managed to find the time. Will try to propose some improvements so maybe the failures are reported in a comment or something

@ericsnowcurrently
Copy link
Member

What's the best way for me to run the address sanitizer locally, to reproduce the failure?

@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented Jun 8, 2021

...and thanks for the helpful analysis above.

@pablogsal
Copy link
Member Author

What's the best way for me to run the address sanitizer locally, to reproduce the failure?

I mentioned it in https://bugs.python.org/msg395321 but I should have mentioned it here as well:

$ export ASAN_OPTIONS=detect_leaks=0:allocator_may_return_null=1:handle_segv=0
$ ./configure --with-address-sanitizer --without-pymalloc
$ make -j -s
$ ./python -m test test_sys_settrace

@pablogsal
Copy link
Member Author

I used gcc 10 for this, but I think clang works as well as any recent version of gcc.

@pablogsal
Copy link
Member Author

giphy (1)

ericsnowcurrently added a commit to ericsnowcurrently/cpython that referenced this pull request Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants