-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
rebase: Throw OutOfMemoryError when gmp tries to allocate memory beyond the limit #31215
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
…imit. Only works with patched GMP; does nothing if linked agaist system GMP.
base/gmp.jl
Outdated
A reference that holds a boolean, if true, indicating julia is linked with a patched GMP that | ||
does not abort on huge allocation and throws OutOfMemoryError instead. | ||
""" | ||
const HAS_ALLOC_OVERFLOW_FUNCTION = Ref(false) |
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.
Is this name fine? Uppercase and all? What about ALLOC_OVERFLOW_PATCH
?
Fixed: I had to delete the srcache file to get julia to patch the gmp file. |
Alright this PR is ready to be merged when ready. I confirmed also that it works locally. |
Looks like an unrelated spawn failure on 32-bit windows. |
Restarted it |
thanks all green now |
@JeffBezanson you approved the original PR, does this now look good to merge? |
Hey all and sorry for bumping an old thread. I'm trying to collect what libraries that Julia depends upon, haven't included the patches you suggested them. I noticed that GMP 6.2.0, which was released long enough after this PR, hasn't included the patch here. See: https://gmplib.org/repo/gmp-6.2/file/tip/memory.c#l39 Is my observation correct? I'm curios because I'm trying to package Julia for my distro and I want to know what libraries it should be safe enough to use our distro's versions vs your patched versions. |
Indeed #8286 was marked as |
Is there a link to an upstream patch submission? No feedback at all from their side? I even consider nagging them myself (yea I know it's not easy since it's a mailing list and not a GitHub PR) but I need something to hold on, not to mention I'd need to check the patch applies to their current version... |
I mean that AFAICT nobody reported it upstream so far. Filing an issue with our patch it probably better than doing nothing, even if the patch may be slightly outdated. |
Agree. If you wouldn't mind doing that, it would be greatly appreciated. |
I emailed this to the gmp-bugs mailing list today because it didn't look like anyone else had done it yet. If the mailing list software didn't hate gmail then it should be publicly visible in the October archives in a few days: https://gmplib.org/list-archives/gmp-bugs/2020-October/thread.html |
@cmcaine I opened an upstream request last month actually https://gmplib.org/list-archives/gmp-bugs/2020-September/004865.html looks like there is a library bug, but the resolution wasn't clear on which route the library developers are planning on going regarding this patch. |
Woops! I don't know how I missed that :(
Edit:
Well, my repeat message prompted some new discussion about memory leaks, so that's okay. https://gmplib.org/list-archives/gmp-bugs/2020-October/004896.html
Vincent Lefevre wrote:
"Since the function could have allocated memory for intermediate computations, how do you avoid memory leaks?"
|
Not much you can do: chances are if you get to the point of OOMing, the process is going to crash, we just want the ability to do it more gracefully than a C abort call, which is something that a library should really never do. If GMP wants to be super careful, it could free anything that was allocated before returning an error, but it would also be fine to document that when an OOM error is returned some allocated memory may be lost. |
rebased #29029
and closes #8286
courtesy @grep0