Skip to content

Conversation

jedisct1
Copy link
Member

emmalloc functions were returning NULL on error, but didn't set errno.

emmalloc functions were returning NULL on error, but didn't set
errno.
Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

@juj should we apply this upstream?

Do you know remember why we don't do already?

@juj
Copy link

juj commented Sep 24, 2025

Errno is not DCEable, requires an awkward TLS slot that user might not have asked for, and very rarely used in web space. (in the close to decade of emmalloc, nobody ever asked for errno support)

I think if something, we should make the alignment failures abort in development mode, rather than return 0. So that way if emmalloc return 0, one will know it was due to an OOM.

In -sABORTING_MALLOC builds, we could optimize out the return 0;s altogether. I.e. allocations will either return a valid pointer, or halt.

The main reason to use emmalloc instead of dlmalloc is to get smaller code size. So it would be best to optimize it for the smallest code size to keep that angle intact.

@sbc100
Copy link
Member

sbc100 commented Sep 24, 2025

I think if something, we should make the alignment failures abort in development mode, rather than return 0. So that way if emmalloc return 0, one will know it was due to an OOM.

I agree, except for the posx_memalign usage. That should probably still set errno and return on bad alignment.

@sbc100
Copy link
Member

sbc100 commented Sep 24, 2025

I think if something, we should make the alignment failures abort in development mode, rather than return 0. So that way if emmalloc return 0, one will know it was due to an OOM.

I agree, except for the posx_memalign usage. That should probably still set errno and return on bad alignment.

In fact malloc itself can only ever fail with ENOMEM, according the man page. No other errors are possible.

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

Successfully merging this pull request may close these issues.

4 participants