Opened 12 years ago
Closed 7 years ago
#10257 closed enhancement (fixed)
Add checking memory functions
Reported by: | jdemeyer | Owned by: | tba |
---|---|---|---|
Priority: | major | Milestone: | sage-6.5 |
Component: | c_lib | Keywords: | malloc sage_malloc |
Cc: | Merged in: | ||
Authors: | Jeroen Demeyer | Reviewers: | Marc Mezzarobba, David Coudert |
Report Upstream: | N/A | Work issues: | |
Branch: | 352ffb8 (Commits, GitHub, GitLab) | Commit: | 352ffb830e05b0935dfe2e6ca26554d5bc388f10 |
Dependencies: | #17794 | Stopgaps: |
Description (last modified by )
We should introduce check_
variants of the Sage memory functions which raise MemoryError
upon failure.
I also propose to add a BSD-inspired sage_reallocarray() function.
Attached branch introduces the functions and uses them in a few places as proof of concept.
Change History (32)
comment:1 Changed 9 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:2 Changed 9 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:3 Changed 8 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:4 Changed 8 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:5 Changed 8 years ago by
- Description modified (diff)
- Milestone changed from sage-6.4 to sage-6.5
- Summary changed from Change malloc to sage_malloc to Improve memory functions
comment:6 Changed 8 years ago by
- Branch set to u/jdemeyer/ticket/10257
- Modified changed from 12/14/14 09:33:45 to 12/14/14 09:33:45
comment:7 Changed 8 years ago by
- Commit set to f81f34909df2a6b2fba91c895b81ce66f8066435
- Description modified (diff)
- Status changed from new to needs_review
comment:8 Changed 8 years ago by
- Commit changed from f81f34909df2a6b2fba91c895b81ce66f8066435 to cc5edd290bd3ef68b472128c194fdd0affc01a39
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
cc5edd2 | Add memory functions
|
comment:9 Changed 8 years ago by
- Summary changed from Improve memory functions to Add checking memory functions
comment:10 Changed 8 years ago by
- Status changed from needs_review to needs_work
- Work issues set to merge conflicts
comment:11 Changed 8 years ago by
- Commit changed from cc5edd290bd3ef68b472128c194fdd0affc01a39 to 6ba47f0ef294af2b0ac5736a36144a5f77bb6c36
Branch pushed to git repo; I updated commit sha1. New commits:
6ba47f0 | Merge remote-tracking branch 'origin/develop' into ticket/10257
|
comment:12 Changed 8 years ago by
- Status changed from needs_work to needs_review
comment:13 Changed 8 years ago by
- Work issues merge conflicts deleted
comment:14 Changed 8 years ago by
- Status changed from needs_review to needs_work
Also conflicts with #12212
comment:15 Changed 8 years ago by
- Dependencies set to #12212, #17645
comment:16 Changed 8 years ago by
- Commit changed from 6ba47f0ef294af2b0ac5736a36144a5f77bb6c36 to cd8407bcf29c42dadade7bed183cc68ebf310b69
comment:17 Changed 8 years ago by
- Commit changed from cd8407bcf29c42dadade7bed183cc68ebf310b69 to fa0612d0a7ad58cc403f157b42877d29ba0371fa
Branch pushed to git repo; I updated commit sha1. New commits:
fa0612d | Merge remote-tracking branch 'origin/develop' into ticket/10257
|
comment:18 Changed 8 years ago by
- Status changed from needs_work to needs_review
comment:19 Changed 8 years ago by
- Reviewers set to Marc Mezzarobba
- Status changed from needs_review to positive_review
comment:20 Changed 7 years ago by
- Dependencies changed from #12212, #17645 to #12212, #17645, #17794
comment:21 Changed 7 years ago by
- Status changed from positive_review to needs_work
On OSX:
sage -t --long src/sage/modules/vector_rational_dense.pyx ********************************************************************** File "src/sage/modules/vector_rational_dense.pyx", line 101, in sage.modules.vector_rational_dense.Vector_rational_dense._init Failed example: try: Vector_rational_dense(QQ^(2^56)) except (MemoryError, OverflowError): print "allocation failed" Expected: allocation failed Got: python(32767,0x7fff7d747300) malloc: *** mach_vm_map(size=2305843009213693952) failed (error code=3) *** error: can't allocate region *** set a breakpoint in malloc_error_break to debug allocation failed **********************************************************************
comment:22 Changed 7 years ago by
Volker, did you enable specific debugging flags? It looks like malloc
is just verbosely saying that the allocation failed, it that the default on OS X?
comment:23 Changed 7 years ago by
Seems to be the default on OSX, I didn't enable any special debugging.
comment:24 Changed 7 years ago by
See also http://bugs.python.org/issue5614
comment:25 Changed 7 years ago by
- Commit changed from fa0612d0a7ad58cc403f157b42877d29ba0371fa to c37e933111f3cc5ccd4c3e673806782c32bbfd74
comment:26 Changed 7 years ago by
- Dependencies changed from #12212, #17645, #17794 to #17794
- Status changed from needs_work to needs_review
comment:27 follow-up: ↓ 28 Changed 7 years ago by
Hello,
Why not using this ?
cdef inline void* check_allocarray(size_t nmemb, size_t size) except? NULL: """ Allocate memory for ``nmemb`` elements of size ``size``. """ return check_malloc( mul_overflowcheck(nmemb, size) ) cdef inline void* check_malloc(size_t n) except? NULL: """ Allocate ``n`` bytes of memory. """ if n == 0: return NULL if n == <size_t>(-1): raise MemoryError("cannot allocate %s bytes" % n) cdef void* ret = sage_malloc(n) if ret == NULL: raise MemoryError("failed to allocate %s bytes" % n) return ret
At least, check_malloc
should have the test if n==<size_t>-1
(if not already in sage_malloc
), and other methods as well.
David.
comment:28 in reply to: ↑ 27 ; follow-up: ↓ 29 Changed 7 years ago by
Replying to dcoudert:
Hello,
Why not using this ?
cdef inline void* check_allocarray(size_t nmemb, size_t size) except? NULL: """ Allocate memory for ``nmemb`` elements of size ``size``. """ return check_malloc( mul_overflowcheck(nmemb, size) ) cdef inline void* check_malloc(size_t n) except? NULL: """ Allocate ``n`` bytes of memory. """ if n == 0: return NULL if n == <size_t>(-1): raise MemoryError("cannot allocate %s bytes" % n) cdef void* ret = sage_malloc(n) if ret == NULL: raise MemoryError("failed to allocate %s bytes" % n) return ret
One reason is that the error message would be less clear this way: any overflow would lead to the same error message. With my code, the error message mentions exactly which allocation failed.
At least,
check_malloc
should have the test ifn==<size_t>-1
(if not already insage_malloc
)
Why? The argument to malloc()
is a number of bytes. Allocating <size_t>(-1)
bytes will very likely fail anyway.
comment:29 in reply to: ↑ 28 Changed 7 years ago by
Replying to jdemeyer:
At least,
check_malloc
should have the test ifn==<size_t>-1
(if not already insage_malloc
)Why? The argument to
malloc()
is a number of bytes. Allocating<size_t>(-1)
bytes will very likely fail anyway.
So we could avoid this test in check_allocarray
, right?
comment:30 Changed 7 years ago by
- Commit changed from c37e933111f3cc5ccd4c3e673806782c32bbfd74 to 352ffb830e05b0935dfe2e6ca26554d5bc388f10
Branch pushed to git repo; I updated commit sha1. New commits:
352ffb8 | Remove check for -1
|
comment:31 Changed 7 years ago by
- Reviewers changed from Marc Mezzarobba to Marc Mezzarobba, David Coudert
- Status changed from needs_review to positive_review
Hello,
After a long compilation, I did a doc build and a sage -t --long src/sage/
(very very long...).
All tests pass on my MBA with OSX 10.10.
David.
comment:32 Changed 7 years ago by
- Branch changed from u/jdemeyer/ticket/10257 to 352ffb830e05b0935dfe2e6ca26554d5bc388f10
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Add memory functions