-
-
Notifications
You must be signed in to change notification settings - Fork 411
Fix issue 19128 - argument to alloca may be too large #2258
Conversation
|
Thanks for your pull request and interest in making D better, @belka-ew! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + druntime#2258" |
| void* ptmp; | ||
| if (elementSize <= buf.sizeof) | ||
| { | ||
| ptmp = buf.ptr; |
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 would argue here that if we are going to fall back to malloc. Then having a pre-allocated buffer is pointless.
This should be:
if (elementSize <= maxAllocaSize)
ptmp = alloca(elementSize);
else
ptmp = malloc(elementSize);
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.
Or at the very least, this should just be a simple s/alloca/malloc/ swap.
void* ptmp = (elementSize > buf.sizeof) ? malloc(elementSize) : buf.ptr;
// Later...
if (ptmp != buf.ptr)
free(ptmp);
|
This is an excellent candidate for migration to a template. @belka-ew, how confident are you in modifying DMD to lower assignment expressions to something like |
At least I could try. This PR has to wait then. |
| { | ||
| ptmp = buf.ptr; | ||
| } | ||
| else if (elementSize < 1024) |
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.
How was 1024 chosen?
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.
Pretty random. Should be enough for a lot of data types, and not too much.
|
I recently noticed that DMD's |
|
From memory, no. You can still get a pointer returned from alloca that will crash if you try to dereference. Try setting the stack size to a low value and call alloca with a number slightly higher than that. |
|
Moving PR to #2409 |
In rt.arrayassign._d_arraysetassign alloca is called if the buffer should be allocated is larger than 16 bytes:
It is dangerous since alloca unavailable to allocate causes undefined behaviour, so the alloca man page states:
See related discussion:
D-Programming-GDC/gdc#699