Refactor definition of ZEND_MM_ALIGNMENT #6981
Conversation
Girgias
force-pushed
the
confix-zend-mm-alginment
branch
from
b48a9b8 to
2f287cc
Compare
Zend/zend_alloc.h
Outdated
| # define ZEND_MM_ALIGNMENT_LOG2 Z_UL(2) | ||
| #else | ||
| # define ZEND_MM_ALIGNMENT Z_UL(CONFIG_ZEND_MM_ALIGNMENT) | ||
| # define ZEND_MM_ALIGNMENT_LOG2 Z_UL(CONFIG_ZEND_MM_ALIGNMENT_LOG2) |
There was a problem hiding this comment.
Shouldn't these be of type size_t rather than zend_ulong?
There was a problem hiding this comment.
They probably should, but ZEND_MM_ALIGNMENT_LOG2 seemed to be used with hashkeys which are zend_ulong, I'll look some more into this.
Girgias
force-pushed
the
confix-zend-mm-alginment
branch
2 times, most recently
from
9283962 to
844d6b9
Compare
Zend/zend_alloc.h
Outdated
| # define ZEND_MM_ALIGNMENT_LOG2 Z_L(3) | ||
| #ifndef CONFIG_ZEND_MM_ALIGNMENT | ||
| # define ZEND_MM_ALIGNMENT (size_t)8 | ||
| # define ZEND_MM_ALIGNMENT_LOG2 (size_t)3 | ||
| #elif ZEND_MM_ALIGNMENT < 4 |
There was a problem hiding this comment.
This check should be checking for CONFIG_ZEND_MM_ALIGNMENT.
Overall, I don't really like this change. configure should always determine an alignment (there is even a fallback for cross-compiling). I believe ZEND_MM_ALIGNMENT is currently only not defined on windows, where we should define it in config.w32.h.in.
I think we would be better off moving the check for alignment < 4 into
Line 259 in 941b55f
u suffixes to Line 269 in 941b55f
There was a problem hiding this comment.
Gah, it looks like just adding u will make ZEND_MM_ALIGNED_SIZE incorrect. It works right now because ZEND_MM_ALIGNMENT_MASK is a signed integer, so the mask is signed extended. It wouldn't work for an unsigned integer. So looks like that does need to be size_t specifically if we switch it to unsigned.
There was a problem hiding this comment.
ACK, I'll look into making the changes in config and dropping this.
Girgias
force-pushed
the
confix-zend-mm-alginment
branch
from
844d6b9 to
d33241d
Compare
Also add a new ZEND_MM_NEED_EIGHT_BYTE_REALIGNMENT definition. This fixes many [-Wsign-conversion] warnings. Co-authored-by: Guillaume Charifi <[email protected]>
Girgias
force-pushed
the
confix-zend-mm-alginment
branch
from
d33241d to
6774266
Compare
Co-authored-by: Bob Weinand <[email protected]>
Currently ZEND_MM_ALIGNMENT might get defined during config,
in which case it is an 'int' and not a 'zend_ulong'.
This fixes many [-Wsign-conversion] warnings.
Co-authored-by: Guillaume Charifi [email protected]