Implement `calloc{,Bytes,Array,Array0}` allocators
ClosedPublic

Authored by ifesdjeen on Nov 26 2014, 8:43 AM.

Details

Summary

This adds zero-initialising versions of malloc{,Bytes,Array,Array0}

  • Add calloc and callocBytes to Foreign.Marshal.Alloc.
  • Add callocArray and callocArray0 to Foreign.Marshal.Array.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ifesdjeen updated this revision to Diff 1736.Nov 26 2014, 8:43 AM
ifesdjeen retitled this revision from to Add 'malloc0' and 'mallocBytes0' to Foreign.Marshal.Alloc..
ifesdjeen updated this object.
ifesdjeen edited the test plan for this revision. (Show Details)
hvr requested changes to this revision.Nov 26 2014, 8:49 AM
hvr edited edge metadata.
hvr added subscribers: duncan, ekmett, simonmar, ezyang.

...did you notice the naming suggested by @ekmett in D461#11540 ?

This revision now requires changes to proceed.Nov 26 2014, 8:51 AM
ifesdjeen updated this revision to Diff 1737.Nov 26 2014, 8:56 AM
ifesdjeen edited edge metadata.

Changin the naming scheme to one proposed by @ekmett

@hvr no, missed it, but now updated

ifesdjeen updated this revision to Diff 1738.Nov 26 2014, 9:05 AM
ifesdjeen edited edge metadata.

Add missing callocArray and callocArray0 suggested by @ekmett

Harbormaster failed remote builds in B2298: Diff 1738!
ifesdjeen updated this revision to Diff 1739.Nov 26 2014, 9:25 AM

Add import for callocBytes

hvr requested changes to this revision.Nov 26 2014, 9:55 AM
hvr edited edge metadata.
hvr added inline comments.
libraries/base/Foreign/Marshal/Alloc.hs
218

This doesn't match the type-sig for calloc(3), i.e.

void *calloc(size_t nmemb, size_t size);
This revision now requires changes to proceed.Nov 26 2014, 9:55 AM
ifesdjeen updated this revision to Diff 1740.Nov 26 2014, 3:04 PM
ifesdjeen edited edge metadata.

Corrected the usage of calloc, which is now correctly using
void * calloc(size_t count, size_t size);.

Although changes to Marshal.Array were reverted, since calloc
by default demads count, therefore is already acting pretty much
like, for example, mallocArray, which is receiving an amount of
elements to be allocated.

Also, with calloc, implementing an array with a special termination
element will not be possible. We can only create (normally) with
malloc and then fill it up with zeroes by using recently added
fillBytes.

Yes, my apologies. I was verifying it in a separate project and have mistakenly copied something wrong...

I've re-done it with CSize -> CSize.

Although (please see comment to the commit / revision change): looks like calloc belongs more to Marshal.Array, since it already allocates count chunks of memory of size size. Which is pretty much other methods in Marshall.Array do.

Also, making callocArray0 would not work, because we're not controlling size of the allocated memory chunk directly, we can only specify two quantifiers, (size and count), and receive a resulting array of size * chunk bytes.

We could add callocArray0 by combining fillBytes with zeroes and mallocArray0.

Please share your thoughts. I may have missed something..

ifesdjeen updated this revision to Diff 1741.Nov 26 2014, 3:17 PM
ifesdjeen edited edge metadata.

Adding missing stages.

hvr added a comment.Nov 26 2014, 3:37 PM

I'm not sure if calloc should really allow to allocate multiple elements. I'm hoping that @duncan et al. may comment...

libraries/base/Foreign/Marshal/Alloc.hs
108–110

the comment incorrectly refers to sizeOf

Sorry, maybe my wording was poor.

calloc (as Manpage says), does the following: receives size and count, and allocates enough space for count objects that are size bytes of memory _each_ and returns a pointer to the allocated memory.

So in the end it allocates just one chunk of memory, not many. It just has a size that's enough to hold count objects...

ifesdjeen updated this revision to Diff 1742.Nov 26 2014, 4:02 PM

Fix wording for allocBytes comment.

hvr added a comment.EditedNov 27 2014, 4:41 AM

Sorry, maybe my wording was poor.

calloc (as Manpage says), does the following: receives size and count, and allocates enough space for count objects that are size bytes of memory _each_ and returns a pointer to the allocated memory.

So in the end it allocates just one chunk of memory, not many. It just has a size that's enough to hold count objects...

What I'm trying to say is that that what you defined as

calloc :: Storable a => Int -> IO (Ptr a)

is actually the mallocArray-counterpart, and should rather be called callocArray :: Storable a=> Int -> IO (Ptr a)

calloc should rather have the same type-signature as malloc, i.e. ​calloc :: Storable a => IO (Ptr a)

and consequently callocArray0 is then obvious to implement as well

duncan edited edge metadata.Nov 27 2014, 4:46 AM

So this is the downside of us picking the calloc naming convention. We're actually only interested in the zeroing feature, but C calloc does that and this size * count stuff that we don't want. This is already confusing us, and could possibly confuse users too.

For the main alloc module we want functions that match the type sigs of malloc and mallocBytes, and have the same behaviour but with the addition of zeroing the memory.

For the array alloc module we can of course have versions that take an element count, like the existing array alloc functions do.

hvr requested changes to this revision.Nov 27 2014, 5:02 AM
hvr edited edge metadata.

So in summary, please perform the following changes:

add to Foreign.Marshal.Array:

callocArray :: Storable a => Int -> IO (Ptr a)
callocArray  = doCalloc undefined
  where
    doCalloc :: Storable a' => a' -> Int -> IO (Ptr a')
    doCalloc dummy size  = callocBytes (size * sizeOf dummy)

callocArray0 :: Storable a => Int -> IO (Ptr a)
callocArray0 size  = callocArray (size + 1)
libraries/base/Foreign/Marshal/Alloc.hs
96–100

change to

calloc :: Storable a => IO (Ptr a)
calloc = doCalloc undefined
  where
    doCalloc :: Storable b => b -> IO (Ptr b)
    doCalloc dummy = callocBytes (sizeOf dummy)
112

Change this to

callocBytes :: Int -> IO (Ptr a)
callocBytes size = failWhenNULL "calloc" $ _calloc (fromIntegral size) 1
This revision now requires changes to proceed.Nov 27 2014, 5:02 AM
ifesdjeen updated this revision to Diff 1760.Nov 28 2014, 4:13 PM
ifesdjeen edited edge metadata.

Make changes requested by @hvr

Done the change with one minor difference:

callocBytes size = failWhenNULL "calloc" $ _calloc (fromIntegral size) 1

actually was

callocBytes size = failWhenNULL "calloc" $ _calloc 1 (fromIntegral size)

Because the signature of calloc is calloc(size_t count, size_t size); and count comes on first place.

ifesdjeen updated this revision to Diff 1761.Nov 28 2014, 4:28 PM
ifesdjeen edited edge metadata.

Correct a typo

hvr requested changes to this revision.Nov 29 2014, 8:13 AM
hvr edited edge metadata.

The code looks good to me, the only thing left to address are the Haddock docstrings... :-)

Done the change with one minor difference:
...
Because the signature of calloc is calloc(size_t count, size_t size); and count comes on first place.

Fwiw, afaik, calloc(x,y) should be operationally equivalent to calloc(y,x), so this should be ok as well...

libraries/base/Foreign/Marshal/Alloc.hs
88–90

This comment does not accurately describe calloc. Best way (as it's confusing to see two descriptions which share a significant amount of text and have to visually find out where they actually differ) would be IMO to simply refer to malloc and describe how calloc differs from malloc (i.e. that the memory returned is zero-initialised).

Same suggestion applies to the other calloc* variants.

This revision now requires changes to proceed.Nov 29 2014, 8:13 AM
hvr retitled this revision from Add 'malloc0' and 'mallocBytes0' to Foreign.Marshal.Alloc. to Implement `calloc{,Bytes,Array,Array0}` allocators.Nov 29 2014, 8:16 AM
hvr updated this object.
hvr edited edge metadata.
hvr added a project: GHC.
hvr added a comment.Nov 29 2014, 8:19 AM

@ifesdjeen oh, and could you shot a follow-up library addition proposal to the libraries-mailinglist for these 4 functions? It'd be a shame not to have these available in GHC 7.10 as well...

ifesdjeen updated this revision to Diff 1766.Nov 29 2014, 9:55 AM
ifesdjeen edited edge metadata.
ifesdjeen updated this object.

Simplify comments.

@hvr done, posted the description to libraries@

austin accepted this revision.Dec 1 2014, 9:42 AM
austin edited edge metadata.

OK, this implementation LGTM, and the libraries discussion seems to be positive.

hvr added a comment.Dec 1 2014, 9:44 AM

@austin I think we should create a placeholder Trac ticket, associate this code-rev, and land it optimistically before branching off ghc-7.10. (if discussion result turns out different thatn expected, we can always git revert...)

Is there anything I could help out with here?

hvr added a comment.Dec 2 2014, 1:49 PM

@ifesdjeen you could create a Trac ticket and link it with this code-rev; also some simple test-case would be nice...

Created a track ticket (hope it's generally correct): https://ghc.haskell.org/trac/ghc/ticket/9859#ticket

Starting work on a test suite.

hvr accepted this revision.Dec 3 2014, 4:42 PM
hvr edited edge metadata.
hvr updated the Trac tickets for this revision.
hvr edited edge metadata.

(...accepted pending some simple test-case)

This revision is now accepted and ready to land.Dec 3 2014, 4:43 PM
This revision was automatically updated to reflect the committed changes.