Skip to content

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

Closed
wants to merge 1 commit into from
Closed

fix cross compilation failure due to size_t typecast in define #5128

wants to merge 1 commit into from

Conversation

pmjdebruijn
Copy link
Contributor

@pmjdebruijn pmjdebruijn commented Jan 29, 2020

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"
 # define ZEND_MM_ALIGNMENT ((size_t) 8)
                                      ^
br-arm-full/build/php-7.4.2/ext/opcache/ZendAccelerator.c:1380:7:
note: in expansion of macro ‘ZEND_MM_ALIGNMENT’
 #elif ZEND_MM_ALIGNMENT < 8

@nikic
Copy link
Member

nikic commented Jan 29, 2020

This fix is not right. Probably just a matter of including whatever header defines size_t?

@pmjdebruijn
Copy link
Contributor Author

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.

@nikic
Copy link
Member

nikic commented Jan 29, 2020

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 #if ((size_t) 8) < 8 in one place, and I'm assuming doing a type cast inside #if is not legal...

@@ -27,12 +27,12 @@
#include "zend.h"

#ifndef ZEND_MM_ALIGNMENT
# define ZEND_MM_ALIGNMENT ((size_t) 8)
# define ZEND_MM_ALIGNMENT 8
Copy link
Member

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.

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?

@pmjdebruijn
Copy link
Contributor Author

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 #if ((size_t) 8) < 8 in one place, and I'm assuming doing a type cast inside #if is not legal...

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.

@pmjdebruijn
Copy link
Contributor Author

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...

@pmjdebruijn
Copy link
Contributor Author

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.

@nikic
Copy link
Member

nikic commented Jan 30, 2020

@pmjdebruijn Usually this constant is determined by configure, and only computed in zend_alloc.h if cross compilation is used.

@pmjdebruijn
Copy link
Contributor Author

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’
@php-pulls php-pulls closed this in f0f5c41 Jan 30, 2020
@nikic
Copy link
Member

nikic commented Jan 30, 2020

Out of curiousity, what is the consequence of getting this wrong? Subpar performance?

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants