memory: fix memory alignment for dynamic allocation #20489
Conversation
jpalus
force-pushed
the
memory-alignment
branch
from
1474c27
to
29a0fab
Compare
|
lint failure |
jpalus
force-pushed
the
memory-alignment
branch
from
29a0fab
to
29cd6eb
Compare
src/nvim/memory.h
Outdated
| # define ARENA_ALIGN 16 | ||
| # else | ||
| # define ARENA_ALIGN ((2 * sizeof(size_t)) < \ | ||
| __alignof__(long double) ? __alignof__(long double) : (2 * sizeof(size_t))) |
There was a problem hiding this comment.
Is there a source that the recommended alignment on x86 is 16 and not 8? and that the required one on AARCH64 is 16 and not 8? (it can be assumed we never use weird or obsolete x87 doubles, nor 128 bit ints). just hardcoding ARENA_ALIGN to MAX(sizeof(void *), sizeof(double)) would have be been simpler.
Also __GNUC__ looks fishy. we only care what the CPU wants, not about gcc/glibc's ideas.
There was a problem hiding this comment.
Above logic is based purely on:
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/generic/malloc-alignment.h;h=ef0f381f772088d3e8a8648410bba85b1a970451;hb=c804cd1c00adde061ca51711f63068c103e94eef
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/generic/malloc-alignment.h;h=ef0f381f772088d3e8a8648410bba85b1a970451;hb=c804cd1c00adde061ca51711f63068c103e94eef
just hardcoding
ARENA_ALIGNtoMAX(sizeof(void *), sizeof(double))would have be been simpler.
I don't really know what are neovim's alignment requirements hence PR attempts to be as generic as possible. If you're sure it will never reach maximum then I guess it's fine.
Also
__GNUC__looks fishy. we only care what the CPU wants, not about gcc/glibc's ideas.
__GNUC__ is there to make sure __i386__ and __alignof__ are available.
There was a problem hiding this comment.
I think, we should try to just do MAX(sizeof(double), sizeof(void *)) here without any GNU-specific checks. the only issue I can see is that we use 4-byte alignment on ARMv7 where 8-byte is needed, due to 64-bit floats. if there is still an issue even with that people can report a new issue.
src/nvim/memory.c
Outdated
| if (arena_align_offset(arena->cur_blk + arena->pos, ARENA_ALIGN, &align_pos) < 0) { | ||
| alignment_ok = false; | ||
| } | ||
| } | ||
| if (arena->pos + size > arena->size || !arena->cur_blk) { | ||
| if (size > (ARENA_BLOCK_SIZE - sizeof(struct consumed_blk)) >> 1) { | ||
| if (arena->pos + align_pos + size > arena->size || !alignment_ok) { | ||
| if (size + (align ? (ARENA_ALIGN - 1) : 0) > (ARENA_BLOCK_SIZE - sizeof(struct consumed_blk)) | ||
| >> 1) { |
There was a problem hiding this comment.
This looks needlessly complicated. Expressing the constraint and doing overflow checking direct in arena->pos integer space was much simpler (instead of first converting to pointer so we can play-pretend "secure pointer handling" by then converting back to uintptr to do pointer-space overflow check.). Remember, this is an interface for known internal consumers, not a malloc implementation which needs be c-standard paranoia compliant.
There was a problem hiding this comment.
To be clear you'd like to drop "fix(memory): check for uintptr_t overflow" correct?
|
Commit with overflow reverted and |
|
Needs a rebase |
jpalus
force-pushed
the
memory-alignment
branch
from
05ae59f
to
ca7b2ee
Compare
|
Rebased and squashed. |
all pointers returned by arena_alloc residing in arena block should be properly aligned to meet neovim's alignment requirements but keeping it simple settle on ARENA_ALIGN = MAX(sizeof(void *), sizeof(double)).
bfredl
force-pushed
the
memory-alignment
branch
3 times, most recently
from
02d85f3
to
8b7247a
Compare
In particular, we can assume the xmalloc-ed pointer is at least double-aligned, otherwise nothing work work.
- fix memory alignment crash on armv7 (neovim/neovim#20489)
|
For some reason this didn't make it into 0.8.1. |
|
That's a shame, maybe we should consider releasing 0.8.2 |
|
This is the plan, unless 0.9 gets finished first. |
|
Successfully created backport PR #21127 for |
all pointers returned by arena_alloc residing in arena block should be properly aligned
also make ARENA_ALIGN reflect what glibc does (with MALLOC_ALIGNMENT) to have proper alignment for each platform. might not be fully accurate but it's best effort approach keeping c99 compatibility. with c11 it could be reduced to single:
Fixes #20437