-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Handle MemoryError for sequences #5418
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
Conversation
ff4df62
to
9ad4d8f
Compare
vm/src/vm/mod.rs
Outdated
@@ -53,6 +53,8 @@ pub use interpreter::Interpreter; | |||
pub(crate) use method::PyMethod; | |||
pub use setting::Settings; | |||
|
|||
pub const MAX_MEMORY_SIZE: usize = (usize::MAX >> 3) + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Is this constant came from CPython?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, I just copied it from the last PR. CPython seems to use max usize instead: https://github.com/python/cpython/blob/e0a41a5dd12cb6e9277b05abebac5c70be684dd7/Objects/listobject.c#L740, https://github.com/python/cpython/blob/e0a41a5dd12cb6e9277b05abebac5c70be684dd7/Include/pyport.h#L146
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks like isize::MAX
, since the first s in ssize_t
means signed. Which makes sense, since that's the max size an allocation can be for LLVM. I do think we could maybe just use isize::MAX
, because otherwise we're unnecessarily constraining allocation size to "just" 512GiB on 32-bit systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use isize::MAX
. Also updated to include the size of the type and fixed the potential integer overflow problem.
Added @coolreader18 as a reviewer who reviewed #1779 |
75a03e3
to
cf9f036
Compare
cf9f036
to
6fb19ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for contributing!
Fixes #1750
Based on #1779 which seems to be abandoned.