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: 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:

Status badges

Description (last modified by jdemeyer)

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 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:2 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:3 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:4 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:5 Changed 7 years ago by jdemeyer

  • 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 7 years ago by jdemeyer

  • 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 jdemeyer

  • Authors set to Jeroen Demeyer
  • Commit set to f81f34909df2a6b2fba91c895b81ce66f8066435
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

f81f349Add memory functions

comment:8 Changed 7 years ago by git

  • Commit changed from f81f34909df2a6b2fba91c895b81ce66f8066435 to cc5edd290bd3ef68b472128c194fdd0affc01a39

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

cc5edd2Add memory functions

comment:9 Changed 7 years ago by jdemeyer

  • Summary changed from Improve memory functions to Add checking memory functions

comment:10 Changed 7 years ago by mmezzarobba

  • Status changed from needs_review to needs_work
  • Work issues set to merge conflicts

comment:11 Changed 7 years ago by git

  • Commit changed from cc5edd290bd3ef68b472128c194fdd0affc01a39 to 6ba47f0ef294af2b0ac5736a36144a5f77bb6c36

Branch pushed to git repo; I updated commit sha1. New commits:

6ba47f0Merge remote-tracking branch 'origin/develop' into ticket/10257

comment:12 Changed 7 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:13 Changed 7 years ago by jdemeyer

  • Work issues merge conflicts deleted

comment:14 Changed 7 years ago by mmezzarobba

  • Status changed from needs_review to needs_work

Also conflicts with #12212

comment:15 Changed 7 years ago by jdemeyer

  • Dependencies set to #12212, #17645

comment:16 Changed 7 years ago by git

  • Commit changed from 6ba47f0ef294af2b0ac5736a36144a5f77bb6c36 to cd8407bcf29c42dadade7bed183cc68ebf310b69

Branch pushed to git repo; I updated commit sha1. New commits:

307e526Revert raise MemoryError changes from #12212
cd8407bMerge branch 'ticket/12212_revert_raise' into ticket/10257

comment:17 Changed 7 years ago by git

  • Commit changed from cd8407bcf29c42dadade7bed183cc68ebf310b69 to fa0612d0a7ad58cc403f157b42877d29ba0371fa

Branch pushed to git repo; I updated commit sha1. New commits:

fa0612dMerge remote-tracking branch 'origin/develop' into ticket/10257

comment:18 Changed 7 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:19 Changed 7 years ago by mmezzarobba

  • Reviewers set to Marc Mezzarobba
  • Status changed from needs_review to positive_review

comment:20 Changed 7 years ago by jdemeyer

  • Dependencies changed from #12212, #17645 to #12212, #17645, #17794

comment:21 Changed 7 years ago by vbraun

  • 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 jdemeyer

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 vbraun

Seems to be the default on OSX, I didn't enable any special debugging.

comment:25 Changed 7 years ago by git

  • Commit changed from fa0612d0a7ad58cc403f157b42877d29ba0371fa to c37e933111f3cc5ccd4c3e673806782c32bbfd74

Branch pushed to git repo; I updated commit sha1. New commits:

c561e2eFix check_allocarray() failure doctest on OS X
6650ca7Add csage dependencies in Makefile
c37e933Merge ticket/17794 into ticket/10257

comment:26 Changed 7 years ago by jdemeyer

  • Dependencies changed from #12212, #17645, #17794 to #17794
  • Status changed from needs_work to needs_review

comment:27 follow-up: Changed 7 years ago by 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

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: Changed 7 years ago by jdemeyer

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 if n==<size_t>-1 (if not already in sage_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 dcoudert

Replying to jdemeyer:

At least, check_malloc should have the test if n==<size_t>-1 (if not already in sage_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 git

  • Commit changed from c37e933111f3cc5ccd4c3e673806782c32bbfd74 to 352ffb830e05b0935dfe2e6ca26554d5bc388f10

Branch pushed to git repo; I updated commit sha1. New commits:

352ffb8Remove check for -1

comment:31 Changed 7 years ago by dcoudert

  • 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 vbraun

  • Branch changed from u/jdemeyer/ticket/10257 to 352ffb830e05b0935dfe2e6ca26554d5bc388f10
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.