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

gh-116088: Insert bottom checks after all sym_set_...() calls #116089

Merged
merged 6 commits into from
Feb 29, 2024

Conversation

gvanrossum
Copy link
Member

Looking for reviews from @Fidget-Spinner and @markshannon. See the issue (gh-116088) for discussion.

@gvanrossum
Copy link
Member Author

FWIW I'm lukewarm about this. Likely the policy that all queries return false/NULL for bottom is enough. So we could also close this (and the issue) without action. @Fidget-Spinner @markshannon thoughts?

@gvanrossum
Copy link
Member Author

Note: test_type_inconsistency is failing here -- it triggers one of these tests. It's too bad for the test, but possibly points to an advantage of the bottom checks.

@Fidget-Spinner
Copy link
Member

test_type_inconsistency failing is correct. It's not creating an executor in this case. Don't we want that to happen?

@gvanrossum
Copy link
Member Author

test_type_inconsistency failing is correct. It's not creating an executor in this case. Don't we want that to happen?

Yes, that's correct behavior with these checks. So should I just change that test to assert that the second call doesn't create an executor?

@Fidget-Spinner
Copy link
Member

Yes, that's correct behavior with these checks. So should I just change that test to assert that the second call doesn't create an executor?

Yes.

@Fidget-Spinner
Copy link
Member

Note to future readers: whether the test creates an executor or doesn't create one isn't too important to me IMO. As long as CPython doesn't execute incorrect behavior, everything is fine.

@gvanrossum gvanrossum enabled auto-merge (squash) February 29, 2024 18:08
@gvanrossum
Copy link
Member Author

Note to future readers: whether the test creates an executor or doesn't create one isn't too important to me IMO. As long as CPython doesn't execute incorrect behavior, everything is fine.

Yeah, all these tests are way over-specified, and we regularly have to adjust them when the implementation changes. They are using internal introspection APIs to verify that the implementation did a specific thing. Unfortunately with optimizers you have to do such things -- if you just test the correct outcome, you'll never catch it when the optimizer just does nothing, and timing tests are out of the question (too flaky).

@gvanrossum gvanrossum merged commit 0656509 into python:main Feb 29, 2024
57 checks passed
@gvanrossum gvanrossum deleted the bottom-checks branch February 29, 2024 18:55
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
…ython#116089)

This changes the `sym_set_...()` functions to return a `bool` which is `false`
when the symbol is `bottom` after the operation.

All calls to such functions now check this result and go to `hit_bottom`,
a special error label that prints a different message and then reports
that it wasn't able to optimize the trace. No executor will be produced
in this case.
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
…ython#116089)

This changes the `sym_set_...()` functions to return a `bool` which is `false`
when the symbol is `bottom` after the operation.

All calls to such functions now check this result and go to `hit_bottom`,
a special error label that prints a different message and then reports
that it wasn't able to optimize the trace. No executor will be produced
in this case.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
…ython#116089)

This changes the `sym_set_...()` functions to return a `bool` which is `false`
when the symbol is `bottom` after the operation.

All calls to such functions now check this result and go to `hit_bottom`,
a special error label that prints a different message and then reports
that it wasn't able to optimize the trace. No executor will be produced
in this case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants