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.

Sign up for GitHub

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

Jump to bottom

Update test_crashers #108297

Closed
vstinner opened this issue Aug 22, 2023 · 5 comments
Closed

Update test_crashers #108297

vstinner opened this issue Aug 22, 2023 · 5 comments
Labels
tests Tests in the Lib/test dir type-feature A feature request or enhancement

Comments

@vstinner
Copy link
Member

vstinner commented Aug 22, 2023 •
edited by bedevere-app bot
Loading

Feature or enhancement

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

No response

Proposal:

test_crashers is currently skipped and some scripts in Lib/test/crashers/ no longer crash.

Linked PRs
@vstinner vstinner added the type-feature A feature request or enhancement label Aug 22, 2023
vstinner added a commit to vstinner/cpython that referenced this issue Aug 22, 2023
@vstinner
pythongh-108297: Update test_crashers
e4dced8 
* Rename Lib/test/crashers/ to Lib/test/test_crashers/.
* Move  Lib/test/test_crashers.py to
  Lib/test/test_crashers/__init__.py.
* test_crashers is no longer skipped and makes sure that scripts do
  crash, and no simply fail with a non-zero exit code.
* Update bogus_code_obj.py to use CodeType.replace().
* Remove Lib/test/crashers/ scripts which no longer crash:

  * recursive_call.py: fixed by pythongh-89419
  * mutation_inside_cyclegc.py: fixed by pythongh-97922
  * trace_at_recursion_limit.py: fixed by Python 3.7
vstinner added a commit to vstinner/cpython that referenced this issue Aug 22, 2023
@vstinner
pythongh-108297: Update test_crashers
00b9633 
* Rename Lib/test/crashers/ to Lib/test/test_crashers/.
* Move  Lib/test/test_crashers.py to
  Lib/test/test_crashers/__init__.py.
* test_crashers is no longer skipped and makes sure that scripts do
  crash, and no simply fail with a non-zero exit code.
* Update bogus_code_obj.py to use CodeType.replace().
* Scripts crashing Python now uses SuppressCrashReport of
  test.support to not create coredump files.
* Remove Lib/test/crashers/ scripts which no longer crash:

  * recursive_call.py: fixed by pythongh-89419
  * mutation_inside_cyclegc.py: fixed by pythongh-97922
  * trace_at_recursion_limit.py: fixed by Python 3.7
@bedevere-bot bedevere-bot mentioned this issue Aug 22, 2023
Closed
@AlexWaygood AlexWaygood added the tests Tests in the Lib/test dir label Aug 22, 2023
@serhiy-storchaka
Copy link
Member

Victor, do you remember we measured the recursion limit for different kind of recursion, like via custom __getitem__, __getattr__, __call__, properties, partial(), etc, and you did an awesome work for reducing the stack consumption? Do you remember the issue number? I think that in many of such cases the recursion is still limited, and it would be nice to have tests for this, to avoid regression.

@vstinner
Copy link
Member Author

It was in 2017, search for "Stack consumption" at https://vstinner.github.io/contrib-cpython-2017q1.html

I added _testcapi.stack_pointer() in issue #75049: see script https://bugs.python.org/file46992/stack_overflow-3.py

On the main branch with a debug build (pydebug with gcc -O0), I get:

test_python_call: 498 calls before crash, stack: 8368 bytes/call
test_python_getitem: 109060 calls before crash, stack: 55 bytes/call
test_python_iterator: 746 calls before crash, stack: 8171 bytes/call

=> total: 110304 calls, 16594 bytes

On the main branch with a release build (gcc -O3), I get:

$ ./python stack_overflow-3.py 
test_python_call: 498 calls before crash, stack: 737 bytes/call
test_python_getitem: 109060 calls before crash, stack: 3 bytes/call
test_python_iterator: 746 calls before crash, stack: 528 bytes/call

=> total: 110304 calls, 1268 bytes

test_python_getitem memory usage is quite good, it's almost free :-)

I'm not sure where 109,060 number comes from.

vstinner added a commit to vstinner/cpython that referenced this issue Aug 22, 2023
@vstinner
pythongh-108297: Update test_crashers
895f2fa 
* Rename Lib/test/crashers/ to Lib/test/test_crashers/.
* Move  Lib/test/test_crashers.py to
  Lib/test/test_crashers/__init__.py.
* test_crashers is no longer skipped and makes sure that scripts do
  crash, and no simply fail with a non-zero exit code.
* Update bogus_code_obj.py to use CodeType.replace().
* Scripts crashing Python now uses SuppressCrashReport of
  test.support to not create coredump files.
* Remove Lib/test/crashers/ scripts which no longer crash:

  * recursive_call.py: fixed by pythongh-89419
  * mutation_inside_cyclegc.py: fixed by pythongh-97922
  * trace_at_recursion_limit.py: fixed by Python 3.7
@serhiy-storchaka
Copy link
Member

Thank you Victor. Other issues:

where there are more examples of stack overflowing code. I think that they could be included instead of the removed recursive_call.py.

We can also add separate test which runs maximally deep recursion (with the limit depending on platform) to ensure that there is no regression in new versions.

vstinner added a commit to vstinner/cpython that referenced this issue Aug 30, 2023
@vstinner
pythongh-108297: Remove test_crashers
c1a2414 
The test was skipped since 2011,
commit 89ba56d. Crasher scripts do
not crash on a reliable way, they rely on undefined behaviors, like
state of the stack memory, and so may or may not crash.
vstinner added a commit to vstinner/cpython that referenced this issue Aug 30, 2023
@vstinner
pythongh-108297: Remove test_crashers
f6a5c91 
The test was skipped in 2011 by
commit 89ba56d.

Scripts in Lib/test/crashers/ do not crash on a reliable way. They
rely on undefined behaviors, like state of the stack memory, and so
may or may not crash. It is not worth it to make sure that they crash
in a continious integration, they should be run manually time to time
instead.
@bedevere-bot bedevere-bot mentioned this issue Aug 30, 2023
Merged
vstinner added a commit that referenced this issue Aug 30, 2023
@vstinner
gh-108297: Remove test_crashers (#108690)
f59c66e 
The test was skipped in 2011 by
commit 89ba56d.

Scripts in Lib/test/crashers/ do not crash on a reliable way. They
rely on undefined behaviors, like state of the stack memory, and so
may or may not crash. It is not worth it to make sure that they crash
in a continious integration, they should be run manually time to time
instead.
@vstinner
Copy link
Member Author

In this issue, I looked at crasher scripts and some of them no longer crash. They should be removed. I will try to come up with a change to remove them and maybe update the others based on the closed PR #108299.

@vstinner
Copy link
Member Author

Sadly, I failed track of this issue. While it may be nice to cleanup Lib/test/crashers/, it became a lower priority for me since I removed test_crashers, instead of running it. I close the issue. If someone is motivated to cleanup this directory, go ahead and propose a new issue and/or PR.

@bedevere-app bedevere-app bot mentioned this issue Jul 8, 2024
Merged
ncoghlan added a commit that referenced this issue Jul 8, 2024
@ncoghlan
gh-108297: Update crashers README for test_crashers removal (#121475)
59be79a 
Update Lib/test/crashers/README for test_crashers removal
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 8, 2024
@ncoghlan @miss-islington
pythongh-108297: Update crashers README for test_crashers removal (py…
e4769c2 
…thonGH-121475)

Update Lib/test/crashers/README for test_crashers removal
(cherry picked from commit 59be79a)

Co-authored-by: Alyssa Coghlan <ncoghlan@gmail.com>
@bedevere-app bedevere-app bot mentioned this issue Jul 8, 2024
Merged
ncoghlan added a commit that referenced this issue Jul 8, 2024
@miss-islington @ncoghlan
[3.13] gh-108297: Update crashers README for test_crashers removal (#…
3e786a4 
…121476)

Update Lib/test/crashers/README for test_crashers removal
(cherry picked from commit 59be79a)

Co-authored-by: Alyssa Coghlan <ncoghlan@gmail.com>
noahbkim pushed a commit to hudson-trading/cpython that referenced this issue Jul 11, 2024
@ncoghlan @noahbkim
pythongh-108297: Update crashers README for test_crashers removal (py…
e0d245c 
…thon#121475)

Update Lib/test/crashers/README for test_crashers removal
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
@ncoghlan @estyxx
pythongh-108297: Update crashers README for test_crashers removal (py…
ff2c1bf 
…thon#121475)

Update Lib/test/crashers/README for test_crashers removal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
tests Tests in the Lib/test dir type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@vstinner @serhiy-storchaka @AlexWaygood