-
-
Notifications
You must be signed in to change notification settings - Fork 63
changed behaviour of malloc(0) and optimized calloc #631
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
base: master
Are you sure you want to change the base?
Conversation
src/libc/allocator.src
Outdated
add hl, bc | ||
or a, a | ||
sbc hl, bc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the add here should never carry?
add hl, bc | |
or a, a | |
sbc hl, bc | |
add hl, bc | |
sbc hl, bc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only if you also move up the pop of the size (the pop of the parameter is undefined data because malloc may clobber it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this go off the assumption that malloc
won't return an address higher than 0xE40000 on the CE and that the allocation size is less than 0x1C0000 bytes or etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This relies on the seemingly sound assumption that the pointer to the allocated memory plus the allocation size does not overflow.
src/libc/allocator.src
Outdated
; test if the size is zero | ||
scf | ||
sbc hl, hl | ||
add hl, bc | ||
jr nc, .finish |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went down a bit of a rabbit hole here... I thought: "What's the point of malloc'ing 0 bytes? Could we treat that as an allocation failure and return null?" And sure enough, the C standard allows for exactly this:
If the size of the space requested is zero, the behavior is implementation-defined: either a null pointer is returned to indicate an error, or the behavior is as if the size were some nonzero value, except that the returned pointer shall not be used to access an object.
If we push the zero-size check into malloc, we can eliminate the check here. We'd have to conditionally keep the check here to be safe if _custom_malloc is being used, but otherwise the check can be cheaply pushed into __simple_malloc:
ex (sp),hl
push bc
ld de,(_heap_ptr)
+ dec hl
add hl,de
jr c,.null
- ld bc,___heaptop
+ ld bc,___heaptop-1
sbc hl,bc
jr nc,.null
add hl,bc
And _standard_malloc:
/* add size of block header to real size */
const size_t size = alloc_size + sizeof(block_t);
+ /* abort if alloc_size is 0 or size overflowed */
- if (size < alloc_size)
+ if (size <= alloc_size)
{
return NULL;
}
The same idea can be applied to the no hardware assumptions version of calloc.
__TICE__
only)432c6ba
to
07d546f
Compare
; inlined memset/bzero | ||
; assumes that malloc(0) returns NULL, so we can skip the check for zero size | ||
add hl, bc | ||
cpd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume it is okay for cpd
to read nonnull_ptr_from_malloc + size
calloc
(when__TICE__
is defined) now uses an inlined implementation ofbzero
which uses the$E40000
address to speed up the zero filling of memory.Otherwise, it will use the previous
memset
implementation when__TICE__
is undefined.