Skip to content
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

Zend/asm: adjust XCOFF asm files for AIX assembler #7579

Closed
wants to merge 1 commit into from

Conversation

Helflym
Copy link

@Helflym Helflym commented Oct 15, 2021

AIX assembler is a bit more strict than GNU assembler. Thus, adjust
the XCOFF asm files to be able to accept both assembler.

Fix #81507

@nikic nikic requested a review from trowski October 15, 2021 07:54
@trowski
Copy link
Member

trowski commented Oct 15, 2021

@NattyNarwhal Does this look fine to you?

@NattyNarwhal
Copy link
Member

I tested the 64-bit files (using the AIX assembler because binutils is busted), but not the 32-bit files, because my build environment is 64-bit only. I'll try this patch and see if 64-bit changes.

Relatedly, if these changes work, please upstream them to boost/context.

@Helflym
Copy link
Author

Helflym commented Oct 18, 2021

I tested the 64-bit files (using the AIX assembler because binutils is busted), but not the 32-bit files, because my build environment is 64-bit only. I'll try this patch and see if 64-bit changes.

Are the fiber tests working fine on your side ? I'm seeing a lot of errors in them, but I didn't have time to investigate further.
About binutils toolchain, I'm planning to fix it too. I might have an idea for where it comes. I'll warn you when it's working fine if you wish.
Note that we'll continue to build PHP for our communities (both BullFreeware and AIXToolbox) with AIX assembler even if Binutils toolchain starts working. So I guess it would be better to continue testing with AIX assembler anyway.

Relatedly, if these changes work, please upstream them to boost/context.

Yes, I'm planning to do that. But there is another file and I'll have to test it so it might take a bit of time.

@NattyNarwhal
Copy link
Member

Are the fiber tests working fine on your side ? I'm seeing a lot of errors in them, but I didn't have time to investigate further.

They were when I tested them in #7338, though now in RC4 I'm getting some inane symbol errors relating to mysql* that only occurs in the test environment and thus cause errors, so We'll See™.

About binutils toolchain, I'm planning to fix it too. I might have an idea for where it comes. I'll warn you when it's working fine if you wish.

I am interested in working binutils, so probably message me off this issue.

Yes, I'm planning to do that. But there is another file and I'll have to test it so it might take a bit of time.

To provide a bit of context to help, I think what's wrong is (I didn't look at 32-bit, only speaking for 64-bit), the calling convention in the context code from Boost does not match (it was basically 32-bit in random places, things out of order, etc.) the system. At least with ELFv2 sharing the calling convention, what I did for 64-bit was make the AIX version match the ELF version exactly but with AIX assembler syntax. 32-bit, that may be trickier because you don't have anything to copy from (except the 64-bit version?). Of course, being that 32-bit AIX is unique, it may be not actually messed up.

@NattyNarwhal
Copy link
Member

Just ran with php run-tests.php Zend/tests/fibers/*.phpt and that skipped the weird issues. All tests pass with this patch on 64-bit.

@krakjoe
Copy link
Member

krakjoe commented Oct 19, 2021

@trowski how much have our versions of these files diverged already from boost ? are we comfortable merging this here before upstream ?

@Helflym
Copy link
Author

Helflym commented Oct 19, 2021

Just ran with php run-tests.php Zend/tests/fibers/*.phpt and that skipped the weird issues. All tests pass with this patch on 64-bit.

Interesting. I'm rebuilding RC4 right now to check if there are still failing with our specfile. Meanwhile, I'm trying to rebuild boost and test with it. I only changed the "envelop" of asm so maybe there is something wrong within the asm code itself in 32bit. I'll check. Thus, this PR mgiht not be ready as is. When is the deadline for having merge in php 8.1.0 ?

@trowski
Copy link
Member

trowski commented Oct 19, 2021

@krakjoe The prior changes made by @NattyNarwhal were merged upstream to boost. I'm fine merging here before boost.

I'll ensure we stay in sync with boost. Looks like there's a fix for macOS + ARM upstream. I'll merge that before GA.

@NattyNarwhal
Copy link
Member

FWIW, you could also take the (risky?) proposition of not building a 32-bit PHP, and we could just remove the 32-bit assembly files instead. At least in the PASE world, all binaries distributed through RPM are 64-bit now, and I don't really see the point of building a 32-bit PHP when you have to have a 64-bit CPU anyways.

Of course, there might be some desire for 32-bit PHP due to libraries or what not.

@krakjoe
Copy link
Member

krakjoe commented Oct 19, 2021

When is the deadline for having merge in php 8.1.0 ?

Just before GA looks acceptable to me ... it would be nice if these changes saw at least one RC.

@trowski I'll leave this in your hands then, merge when ready ...

@Helflym
Copy link
Author

Helflym commented Oct 19, 2021

FWIW, you could also take the (risky?) proposition of not building a 32-bit PHP, and we could just remove the 32-bit assembly files instead. At least in the PASE world, all binaries distributed through RPM are 64-bit now, and I don't really see the point of building a 32-bit PHP when you have to have a 64-bit CPU anyways.

Of course, there might be some desire for 32-bit PHP due to libraries or what not.

From what I've heard some customers of IBM still need it. However, I'll discuss that with my IBM contact. If it's too time-consuming to support, it seems to be the logical approach.

However, in boost, we'll want to have both anyway, as it creates libraries.

@NattyNarwhal
Copy link
Member

Yeah, and if you fix Boost, you've fixed this.

FWIW there is the ucontext fallback, which may be a valid approach on 32-bit. I decided to get it working on 64-bit instead.

@Helflym
Copy link
Author

Helflym commented Oct 20, 2021

I've discussed with my IBM contact and we agree that 32bit can be dropped for AIX.
I don't know how much code is made for it and if it can be removed easily. But for now, that means that we can avoid fixing fibers for 32bit AIX. I'll change my patch to improve only 64bit files.

@krakjoe
Copy link
Member

krakjoe commented Oct 26, 2021

bump @trowski

@trowski
Copy link
Member

trowski commented Oct 26, 2021

So can I assume the 32-bit XCOFF assembly probably doesn't work? Maybe then we should change configure to fallback to ucontext on that platform. Unlikely to see much (any) use, but at least it would work.

@Helflym
Copy link
Author

Helflym commented Oct 27, 2021

So can I assume the 32-bit XCOFF assembly probably doesn't work? Maybe then we should change configure to fallback to ucontext on that platform. Unlikely to see much (any) use, but at least it would work.

If there was an old way to provide these fibers mechanisms, I guess it should be okay to keep that. PHP 32bit is working fine for a long time, it's just that it takes too much time to port/support new features for it. So, I'm fine falling back to an previous option.

@Helflym Helflym force-pushed the master branch 2 times, most recently from 7d305a7 to 87d44ba Compare November 3, 2021 10:20
@Helflym
Copy link
Author

Helflym commented Nov 3, 2021

I've updated boost context library including ppc32 files: boostorg/context#191.
This patch is now equivalent to what is in boost.

@nikic
Copy link
Member

nikic commented Nov 3, 2021

This doesn't seem to update the ppc32 files?

@Helflym
Copy link
Author

Helflym commented Nov 3, 2021

As we said above, the support for 32bit on AIX can be dropped. There is no real need and it will ease the support of AIX for PHP, only one architecture to work on instead of two.
That's why I didn't update the ppc32 files here.

@nikic
Copy link
Member

nikic commented Nov 3, 2021

Shouldn't the files be deleted in that case? I'd expect them to either be in sync with upstream or to not exist.

@Helflym
Copy link
Author

Helflym commented Nov 3, 2021

I can remove them for sure. But I'm expecting that some configuration files need to be updated in order to disable the asm version of the fiber feature.
@trowski was speaking of falling back to ucontext. But I'm not sure how to proceed on that.
Note that this can go in another patch. This one just fixing 64bit support on AIX.

@trowski
Copy link
Member

trowski commented Nov 3, 2021

@Helflym Looks like you pushed both 32 and 64-bit upstream, so lets update both of them here too.

AIX assembler is a bit more strict than GNU assembler. Thus, adjust
the XCOFF asm files to be able to accept both assembler.
@nikic nikic closed this in cfe8a45 Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants