-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
thirdparty: fix tcc __atomic_thread_fence #26185
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
|
@kbkpbot I think it should be fine now; I'll test on macos to be sure and in a VM asap. |
|
Thank you @spytheman |
|
Also will fix #25856 |
spytheman
left a comment
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.
Excellent work 🙇🏻 .
@tankf33der what do you think?
|
Since |
|
@spytheman - i take my words back. do not merge. i am testing. |
|
During the pause, I decided to check that the patch works. |
|
x64 alpine - ok |
|
some fail on one of the ARM |
|
what is on line |
|
Yes, due to |
|
Afaik, there is no policy restriction for .s files in thirdparty/ , but I am not sure if there will not be technical ones 🤔 . I have not tried using separate .s files till now. |
|
|
no. #include <stdio.h>
void main() {
__asm__ __volatile__("" : : : "memory")
} |
|
@spytheman let me know when i should start manual testing. |
spytheman
left a comment
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.
Excellent work.
Thank you @kbkpbot .
|
@tankf33der I think it is ready to be tested (and merged). |
I will start testing in 90mins. |
|
|
X64 on Alpine failed this way with |
|
RISCV64 on Alpine |
I need more info to find out what is really going on. please provide more info about your |
It seems you miss a prebuilt |
|
I've checked in a Docker container with Alpine x64 , 386deb8 fixes it for me, by adding a flag for the .S file, and changing |
|
For the Alpine X64 case, and for that clone: |
|
tcc d3e940c is from 2022-04-28 . We should update it indeed. It is currently 499 commits behind the tip of the |
|
Can we provide both |
I am not sure it can pass |
|
hm, if there is a pure V implementation (well with inline assembly), why do we need a separate .S file ? |
|
the vtcc implementation does not use a memory barrier 🤔 |
That is for |
|
The last compilation fails because of unrelated reasons - it is tuned to work on a glibc system, and imho making it work on a musl one, is out of scope for this PR. |
|
If the new |
|
The sanitized CI jobs failed due to an unrelated problem, that is fixed on master. |
|
Thank you @kbkpbot 🙇🏻 . |

Fix issue #25856
Fix issue #26158
This bug is caused by on
aarch64,tccremove the prefix__from__atomic_thread_fence, so a workaround needed here.Maybe a
tccfix is needed for upstream.ping @spytheman would you please help me verify that the test file is written in a correct format ? I just want to check it can be compiled under
tcc/arm64. Do it need to set aVFLAGS=-no-retry-compilation?