Skip to content

Fix GH-11188: Error when building TSRM in ARM64 #11236

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix GH-11188: Error when building TSRM in ARM64 #11236

wants to merge 1 commit into from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented May 12, 2023 •
edited
Loading

Although the issue mentioned FreeBSD, this is a broader problem:
the current ARM64 code to load the TLS offset assumes a setup with
the non-default TLS model. This problem can also apply on some
configurations on other platforms.

@nielsdos nielsdos linked an issue May 12, 2023 that may be closed by this pull request
Error when building TSRM in ARM64 #11188
Closed
@nielsdos nielsdos marked this pull request as draft May 12, 2023 18:26
@nielsdos nielsdos force-pushed the fix-11188 branch 4 times, most recently from 7deb52a to 068e554 Compare May 12, 2023 19:36
@nielsdos nielsdos marked this pull request as ready for review May 12, 2023 20:08
@Girgias Girgias requested a review from dstogov May 15, 2023 15:53
@arnaud-lb
Copy link
Member

arnaud-lb commented May 27, 2024 •
edited
Loading

I think the code to fetch the TCB offset is good, but unfortunately I don't think we can cache the TCB offset unless we are using the local-exec or initial-exec models, because this offset may be different in each thread (as the dynamic TLS block of the module is allocated separately in each thread).

We do it on some platforms, but I believe that are relying on surplus Static TLS Space in these platforms.

When loading apache mod_php on freebsd aarch64, our TLS block is allocated dynamically. I found about this because the assertion tsrm_ls_cache_tcb_offset <= LDR_STR_PIMM64 fails.

We should probably return 0 in the TSRM_TLS_MODEL_DEFAULT case, and update the JIT to cache only the TLS descriptor as we do in x86. (This code is broken currently on FreeBSD, but this should fix it: #13928. Additionally, on ARM the descriptor is 3 words.)

@nielsdos
Copy link
Member Author

Interesting, thanks for the review!
I guess I'll wait for your merge to adapt this. I'm currently busy with something else anyway...

@arnaud-lb
Copy link
Member

I've merged #13928

@nielsdos
Copy link
Member Author

Thanks, I will be able to allocate time for this tomorrow to rebase and finish this PR.

@nielsdos nielsdos changed the base branch from PHP-8.1 to PHP-8.2 May 30, 2024 18:32
@nielsdos nielsdos requested a review from iluuu1994 as a code owner May 30, 2024 18:33
@nielsdos
Copy link
Member Author

nielsdos commented May 30, 2024 •
edited
Loading

It took a little while longer because the software emulation slowed me down, but hopefully I did it right. At least it seems to work locally, but best double check.

arnaud-lb
arnaud-lb approved these changes May 31, 2024
View reviewed changes
Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

ext/opcache/jit/zend_jit_arm64.dasc Outdated
Comment on lines 2780 to 2784
/* https://github.com/freebsd/freebsd-src/blob/c52ca7dd09066648b1cc40f758289404d68ab886/libexec/rtld-elf/aarch64/reloc.c#L225-L233 */
TLSDescriptor *tlsdesc = where[1];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may mention the ABI here, as it specifies that where points to two GOT entries, and the second one holds the information we need. This information is platform-specific though:

The first entry holds a pointer to the variable's TLS descriptor resolver function and the second entry holds a platform-specific offset or pointer.

(https://github.com/ARM-software/abi-aa/blob/2a70c42d62e9c3eb5887fa50b71257f20daca6f9/aaelf64/aaelf64.rst)

@nielsdos
Fix phpGH-11188: Error when building TSRM in ARM64
e4d2a3e 
Although the issue mentioned FreeBSD, this is a broader problem:
the current ARM64 code to load the TLS offset assumes a setup with
the non-default TLS model. This problem can also apply on some
configurations on other platforms.
@nielsdos
Copy link
Member Author

nielsdos commented May 31, 2024 •
edited
Loading

Thanks for the link & review, updated the comment.
Looks like for master we'll need changes in the IR repo. So I'm proposing the following:

  • Merge this into 8.2 & 8.3, empty merge into master
  • Get a PR to IR to support the codegen part
  • Once IR is merged back into PHP master, create a PR that essentially recreates this PR on top of JIT-IR.

Sounds good?

@arnaud-lb
Copy link
Member

Yes this sound like a good plan

nielsdos added a commit to nielsdos/ir that referenced this pull request Jun 1, 2024
@nielsdos
Add support for AArch64 FreeBSD dynamic TLS
659e274 
This change is necessary to support
php/php-src#11236 for IR JIT.
@nielsdos nielsdos mentioned this pull request Jun 1, 2024
Merged
dstogov pushed a commit to dstogov/ir that referenced this pull request Jun 3, 2024
@nielsdos
Add support for AArch64 FreeBSD dynamic TLS (#87)
97555e1 
This change is necessary to support
php/php-src#11236 for IR JIT.
dstogov
dstogov approved these changes Jun 3, 2024
View reviewed changes
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't check this. Feel free to merge this. (the IR part already in master).

@nielsdos nielsdos closed this in 644d362 Jun 3, 2024
nielsdos added a commit to nielsdos/php-src that referenced this pull request Jun 3, 2024
@nielsdos
Fix phpGH-11188: Error when building TSRM in ARM64 (for IR JIT)
e438ca5 
This is phpGH-11236 for IR JIT.
@nielsdos nielsdos mentioned this pull request Jun 3, 2024
Merged
nielsdos added a commit that referenced this pull request Jun 4, 2024
@nielsdos
Fix GH-11188: Error when building TSRM in ARM64 (for IR JIT) (#14459)
89c4e4c 
This is GH-11236 for IR JIT.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arnaud-lb arnaud-lb arnaud-lb approved these changes

@dstogov dstogov dstogov approved these changes

@iluuu1994 iluuu1994 Awaiting requested review from iluuu1994

Assignees
No one assigned
Labels
Category: TSRM Extension: opcache
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error when building TSRM in ARM64
3 participants
@nielsdos @arnaud-lb @dstogov