Fix GH-16168: php 8.1 and earlier crash immediately when compiled with Xcode 16 clang on macOS 15 #16348
Conversation
…with Xcode 16 clang on macOS 15 The inline assembly uses labels with the prefix `.L`. On Linux systems this is the local label prefix. It appears that macOS uses `L` as a local prefix, which means that the prefix used in the inline assembly is not local for macOS systems [1]. This causes the compiler to get confused and move part of the inline assembly into another function. This is avoided on PHP 8.2 and up by the fact that it uses `zend_never_inline NOIPA`, but nothing guarantees that compiler changes won't affect this as well. To solve this issue, we instead use local labels. These will make the compiler pick the correct prefix, preventing the issue. Additionally, while here, we also change the computation of `delta`. It is undefined behaviour to compute the pointer difference between two different objects. To circumvent this, we cast first to `uintptr_t`. This change is cleanly backportable to 8.1 for vendors to pick up. [1] php#16168 (comment)
I mischaracterized this in my report. It doesn't get moved. The assembly code ends up in five different places due to inlining. The fifth occurrence is the entire correct code. The four earlier instances are missing one part; the optimizer apparently decided that it could merge the five instances of that part of the function into one and adjust the jump instructions to match. This of course means that control passes incorrectly from those first four functions into the middle of the fifth function. |
| size_t delta = (const char*)s2 - (const char*)s1; | ||
| uintptr_t delta = (uintptr_t) s2 - (uintptr_t) s1; |
There was a problem hiding this comment.
Just a question: what was wrong with size_t.
Probably it would be better to use ssize_t (or offset_t).
Now, may be intptr_t is better (I'm not sure).
There was a problem hiding this comment.
I think size_t is good too. On the x86 platforms they are the same size anyway. I just changed it to match the types of the right hand side without thinking too much about it. I can change it back to size_t.
There was a problem hiding this comment.
No problem. uintptr_t is good as well. (size_t and pointers have the same size, according to some standard).
|
@ryandesign right, I'll update the description, thanks for checking |
Agreed by RM: #16168 (comment) The inline assembly uses labels with the prefix `.L`. On Linux systems this is the local label prefix. It appears that macOS uses `L` as a local prefix, which means that the prefix used in the inline assembly is not local for macOS systems [1]. When combined with inlining, this causes the compiler to get confused and merge a part of the inline assembly between different functions, causing control flow to jump from one function to another function. This is avoided on PHP 8.2 and up by the fact that it uses `zend_never_inline NOIPA`, but nothing guarantees that compiler changes won't affect this as well. To solve this issue, we instead use local labels. These will make the compiler pick the correct prefix, preventing the issue. Additionally, while here, we also change the computation of `delta`. It is undefined behaviour to compute the pointer difference between two different objects. To circumvent this, we cast first to `uintptr_t`. This change is cleanly backportable to 8.1 for vendors to pick up. [1] #16168 (comment) With the help of investigation and testing of @ryandesign. Closes GH-16348.
The inline assembly uses labels with the prefix
.L. On Linux systems this is the local label prefix. It appears that macOS usesLas a local prefix, which means that the prefix used in the inline assembly is not local for macOS systems [1].This causes the compiler to get confused and move part of the inline assembly into another function. This is avoided on PHP 8.2 and up by the fact that it uses
zend_never_inline NOIPA, but nothing guarantees that compiler changes won't affect this as well.To solve this issue, we instead use local labels. These will make the compiler pick the correct prefix, preventing the issue.
Additionally, while here, we also change the computation of
delta. It is undefined behaviour to compute the pointer difference between two different objects. To circumvent this, we cast first touintptr_t.This change is cleanly backportable to 8.1 for vendors to pick up.
[1] #16168 (comment)
This was tested by @ryandesign on their macOS machine with their setup and this patch works for them on old versions too.