Skip to content
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

Add std.internal.mmm and reap benefits in @safe-ty and OOM checking #3031

Closed
wants to merge 5 commits into from

Conversation

JakobOvrum
Copy link
Member

This patch adds a new internal module std.internal.mmm with the following functions:

// Unsafe as T has pointers
T* checkedMalloc(T)() @system if (hasIndirections!T);

// Ditto
T[] checkedMallocArray(T)(size_t n) @system if (hasIndirections!T);

// Known to be safe because T has no pointers
T* checkedMalloc(T)() @trusted if (!hasIndirections!T);

// Ditto
T[] checkedMallocArray(T)(size_t n) @trusted if (!hasIndirections!T);

// Attributes depend on emplaceRef
T* checkedMalloc(T, Args...)(auto ref Args args);

// Attributes depend on the range primitives of `Range` as well as
// the construction and destruction of `ElementType!Range`
T[] checkedMallocArray(T, Range)(Range range)
if (isInputRange!Range && hasLength!Range && is(ElementType!Range : T));

// Ditto; same as above but with inferred element type
ElementType!Range[] checkedMallocArray(Range)(Range range)
if (isInputRange!Range && hasLength!Range);

// Always safe as calloc zero-initializes memory
T* checkedCalloc(T)() @trusted;

// Ditto
T[] checkedCallocArray(T)(size_t n) @trusted;

// Destroy all elements in the given array
void[] destroyArray(T)(ref T[] array);

Additionally, inferred purity can be supported if malloc and calloc are marked pure in core.stdc.stdlib in the spirit of memory allocation being practically pure.

Included are a few commits demonstrating how to reap benefits from this module. They can be moved to a different PR if desirable - the point is that they demonstrate the module's usefulness (hopefully).

It might be best to review this PR commit-wise.

edit:

These functions also handle OOM correctly; in Phobos, there are more examples of incorrect handling of OOM, like trying to dynamically allocate an exception, either directly or through enforce, rather than use onOutOfMemoryError.

@JakobOvrum
Copy link
Member Author

I'm aware of the test failures and I'll fix them as soon as I can.

@JakobOvrum
Copy link
Member Author

Test success depends on #3032.

@WalterBright
Copy link
Member

How does test coverage look?

dmd std/internal/mmm -unittest -main -cov

@JakobOvrum
Copy link
Member Author

How does test coverage look?

I used DMD's coverage analysis when writing the tests. IIRC all paths are tested except the OOM paths. I'll confirm when all the dependencies are figured out.

@JakobOvrum
Copy link
Member Author

This passes now, finally. Review would be appreciated.

I want to patch unique and RefCounted to use this, but I don't know if I should put the commits in this PR or a new one that depends on this...

// collectException(destroy(e)); // Issue 12647
try
destroy(e);
catch(Exception) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should have a utility function for this, destroyArrayReverse or so.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@MartinNowak
Copy link
Member

Looks great

@DmitryOlshansky
Copy link
Member

Would be awesome to address minor comments and move this in

@schveiguy
Copy link
Member

I have no skin in this game, but is there a better name we can use instead of mmm? That just doesn't seem right :)

@JakobOvrum
Copy link
Member Author

Updated:

  • tabs -> spaces
  • Added destroyArray utility function and changed schwartzSort to use it
  • Added some more tests
  • Added documentation (but the module is still internal)

Passes the autotester again now.

@DmitryOlshansky
Copy link
Member

Maybe we should make destroy array deallocate the array itself, keeping in mind the use cases in schwartzSort.

@JakobOvrum
Copy link
Member Author

Maybe we should make destroy array deallocate the array itself, keeping in mind the use cases in schwartzSort.

Destruction and deallocation need to be separate so that the caller can manually vet the memory safety of the deallocation with @trusted, while leaving the memory safety of destruction to attribute inference.


Authors: Jakob Øvrum
*/
module std.internal.mmm;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually try to stay away from trivial matters, but mmm is quite out there. Where does it even come from?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posterity, it was meant to be "Manual Memory Management". It's an internal module so I didn't put much thought into it

@andralex
Copy link
Member

I think this needs to be redone as a layer working with std.experimental.allocator. Then the functionality in these functions is immediate to achieve by using Mallocator.

@andralex andralex closed this Dec 29, 2015
@JakobOvrum
Copy link
Member Author

If I'm not mistaken, the two interesting/useful functions in this are essentially the same as make and makeArray from std.alloactor, except hardcoded to use malloc. So we should definitely make this work with std.allocator instead.

@JakobOvrum
Copy link
Member Author

I see that make returns null on failure. In the example use cases from this PR, there is nothing that can be done on OOM except throw OutOfMemoryError, and you'd think this is fairly common in both libraries and applications when used with something like malloc. So maybe this use case should be reflected in std.allocator.package; a kind of makeOrFail wrapper function. It's orthogonal to memory safety but I mention it since it was part of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants