Opened 11 years ago
Closed 7 years ago
#10257 closed enhancement (fixed)
Add checking memory functions
Reported by:  jdemeyer  Owned by:  tba 

Priority:  major  Milestone:  sage6.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 BSDinspired 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 8 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:2 Changed 8 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:3 Changed 8 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:4 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:5 Changed 7 years ago by
 Description modified (diff)
 Milestone changed from sage6.4 to sage6.5
 Summary changed from Change malloc to sage_malloc to Improve memory functions
comment:6 Changed 7 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 7 years ago by
 Commit set to f81f34909df2a6b2fba91c895b81ce66f8066435
 Description modified (diff)
 Status changed from new to needs_review
comment:8 Changed 7 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 7 years ago by
 Summary changed from Improve memory functions to Add checking memory functions
comment:10 Changed 7 years ago by
 Status changed from needs_review to needs_work
 Work issues set to merge conflicts
comment:11 Changed 7 years ago by
 Commit changed from cc5edd290bd3ef68b472128c194fdd0affc01a39 to 6ba47f0ef294af2b0ac5736a36144a5f77bb6c36
Branch pushed to git repo; I updated commit sha1. New commits:
6ba47f0  Merge remotetracking branch 'origin/develop' into ticket/10257

comment:12 Changed 7 years ago by
 Status changed from needs_work to needs_review
comment:13 Changed 7 years ago by
 Work issues merge conflicts deleted
comment:14 Changed 7 years ago by
 Status changed from needs_review to needs_work
Also conflicts with #12212
comment:15 Changed 7 years ago by
 Dependencies set to #12212, #17645
comment:16 Changed 7 years ago by
 Commit changed from 6ba47f0ef294af2b0ac5736a36144a5f77bb6c36 to cd8407bcf29c42dadade7bed183cc68ebf310b69
comment:17 Changed 7 years ago by
 Commit changed from cd8407bcf29c42dadade7bed183cc68ebf310b69 to fa0612d0a7ad58cc403f157b42877d29ba0371fa
Branch pushed to git repo; I updated commit sha1. New commits:
fa0612d  Merge remotetracking branch 'origin/develop' into ticket/10257

comment:18 Changed 7 years ago by
 Status changed from needs_work to needs_review
comment:19 Changed 7 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 followup: ↓ 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 ; followup: ↓ 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