-
Notifications
You must be signed in to change notification settings - Fork 7.9k
fix cross compilation failure due to size_t typecast in define #5128
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
Conversation
This fix is not right. Probably just a matter of including whatever header defines |
Thanks for your fast response. Adding an include for stddef.h or malloc.h to Zend/zend_alloc.h doesn't seem to help... However, I just realized br-arm-full target for buildroot actually uses uclibc as opposed to glibc. As a frame of reference, these builds are happening as part of the buildroot framework, which has a test script called "test-pkg", which builds a specific package for a predefined set of architecture and library combinations. |
Ah sorry, I didn't properly understand what the issue is: It's not the use of size_t itself, but the fact that we end up expanding to |
Zend/zend_alloc.h
Outdated
@@ -27,12 +27,12 @@ | |||
#include "zend.h" | |||
|
|||
#ifndef ZEND_MM_ALIGNMENT | |||
# define ZEND_MM_ALIGNMENT ((size_t) 8) | |||
# define ZEND_MM_ALIGNMENT 8 |
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.
As C unfortunately doesn't have a suffix for size_t
, I'd suggest replacing this with Z_UL(8)
. It may differ from size_t, but only on x32 ABI.
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 about just getting the size of size_t during the configure stage and making it a define?
As a sidenote, the build failure only occurs in specific scenarios, "crosscompiling" from x86-64 to x86-64 for a glibc target is fine for example. |
I just did a little more digging, maybe this is actually a GCC version issue? I just noticed that the reference compiler for br-arm-full is gcc 5.5.0, which is getting on the older side of things... |
So I just finished x86-64 "crosscompile" with uclibc and gcc 5.5.0, and it built just fine. So presumably that means this really it is an architecture specific issue. |
@pmjdebruijn Usually this constant is determined by configure, and only computed in zend_alloc.h if cross compilation is used. |
Out of curiousity, what is the consequence of getting this wrong? Subpar performance? |
The following commit introduces a cross-compilation failure: 93c728b "Try to control ZEND_MM_ALIGNED_SIZE type" br-arm-full/build/php-7.4.2/Zend/zend_alloc.h:30:38: error: missing binary operator before token "8" ^ br-arm-full/build/php-7.4.2/ext/opcache/ZendAccelerator.c:1380:7: note: in expansion of macro ‘ZEND_MM_ALIGNMENT’
To be honest, I'm not totally sure ... I think theoretically assuming a too large alignment can be a correctness issue, as we assume all allocations to have this alignment. As we use our own allocator (with controlled alignment) unless you explicitly disable it, and as the minimum alignment of system malloc is usually either 8 or 16, I don't think it matters in practice. In any case, I've merged a variant of this patch (with Z_L -> Z_UL) into 7.4. |
The following commit introduces a cross-compilation failure:
93c728b
"Try to control ZEND_MM_ALIGNED_SIZE type"