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.

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

pcre2 misdetects __builtin___clear_cache() on some platforms with LLVM #92

Closed
pkubaj opened this issue Mar 25, 2022 · 12 comments
Closed

pcre2 misdetects __builtin___clear_cache() on some platforms with LLVM #92

pkubaj opened this issue Mar 25, 2022 · 12 comments

Comments

@pkubaj
Copy link

pkubaj commented Mar 25, 2022

In https://github.com/PhilipHazel/pcre2/blob/master/src/sljit/sljitConfigInternal.h#L322, pcre2 checks for existence of __builtin___clear_cache() function. LLVM provides that function for all platforms, even those that don't have implemented flushing the cache: https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/builtins/clear_cache.c#L61

This leads to runtime errors on e.g. powerpc (32-bit) with __clear_cache() calling compilerrt_abort(), resulting in SIGABRT.
@zherczeg
Copy link
Collaborator

Summon @carenas

@carenas
Copy link
Contributor

carenas commented Mar 26, 2022 •
edited
Loading

This leads to runtime errors on e.g. powerpc (32-bit) with __clear_cache() calling compilerrt_abort(), resulting in SIGABRT

could you provide more details about the system and compiler version where this is being triggered?, FWIW debian bookworm / ppc64 with clang 8 (the oldest available, but all versions installed for that system would behave the same) doesn't seem to trigger it (using the upstream sljit to test, which includes required fixes for gcc) :

$ git clone https://github.com/zherczeg/sljit.git
$ cd sljit && make CC='clang-8 -m32'
$ bin/sljit_test -s
SLJIT tests: all tests are PASSED on PowerPC 32bit (big endian + unaligned) (with fpu)

@pkubaj
Copy link
Author

pkubaj commented Mar 26, 2022

It's FreeBSD 13.1-BETA3 with LLVM 13.0.0. For me also all the tests pass, but it looks like the tests don't test SLJIT_CACHE_FLUSH.

@carenas
Copy link
Contributor

carenas commented Mar 26, 2022 •
edited
Loading

SLJIT_CACHE_FLUSH is tested implicitly by sljit tests 11 and 12, as they need to clear the cache after the code self modifies.

did you get the failure with a specific pcre2 version?, the sljit code I pointed at is still not integrated into pcre2's HEAD and has a couple of fixes that corrected similar issues with gcc in sparc and power (the builtin is not implemented at all so those tests would fail as the cache was never cleared, IMHO clang's plan with their "abort" was to get reports of this builtin use so it will be implemented where needed AFAIK)

@pkubaj
Copy link
Author

pkubaj commented Mar 26, 2022

Here https://github.com/zherczeg/sljit/blob/master/sljit_src/sljitConfigInternal.h#L327 you clearly set that this code is not compiled when using clang. This explains why it works correctly. Indeed this check is missing from pcre2.

I have the last 10.39 version of pcre2.

@carenas
Copy link
Contributor

carenas commented Mar 26, 2022 •
edited
Loading

@zherczeg: this is addressed already by the latest code in sljit and therefore could be provided as a patch for next pcre2 release.

@pkubaj: this is still worth reporting to LLVM so the clang implementation for the builtin is fixed and no longer
abort at runtime in 32-bit power (which affects at least two platforms from FreeBSD), pcre2/sljit have fallback code that can be used when the builtin is unreliable, but other project might not.

it is also worth noting that it at least doesn't abort in 32-bit powerpc under Linux, but at least Debian doesn't provide a 32-bit compiler-rt and therefore avoid this bug.

@pkubaj
Copy link
Author

pkubaj commented Mar 26, 2022

I fix packages for ppc* on FreeBSD so I will know rather quickly what doesn't build. The issue here is quite easy to spot during building - compiled binaries fail with SIGABRT.
Obviously, they may compile successfully (like php) and just abort during building a reverse dependency (like php), but anyway, I'll probably pick up 99% of the packages affected.
BTW, pcre is also affected. Could there be a release for the legacy pcre as well?

@carenas
Copy link
Contributor

carenas commented Mar 29, 2022 •
edited
Loading

a proposed patch for compiler-rt's builtin that has been verified to solve the problem with a manually built clang 14 based compiler in Debian 12 PowerPC64 (big endian) is available in :

https://reviews.llvm.org/D122640

note that unlike FreeBSD, to be able to use the fix in Linux using a modified sljit, EXTRA_LDFLAGS=--rtlib=compiler-rt is required

jsji pushed a commit to llvm/llvm-project that referenced this issue Mar 31, 2022
@carenas
[compiler-rt] Implement __clear_cache on FreeBSD/powerpc
81f5c62 
dd91734 (Add clear_cache implementation for ppc64. Fix buffer to
meet ppc64 alignment., 2017-07-28), adds an implementation for
__builtin___clear_cache on powerpc64, which was promptly ammended to
also be used with big endian mode in f67036b (This ppc64 implementation
of clear_cache works for both big and little endian., 2017-08-02)

clang will use this implementation for it's builtin on FreeBSD and result
in an abort() in the cases where 32-bit generation was requested (ex in
macppc or when the big endian powerpc64 build was done with "-m32") and as
reported[1] recently with pcre2, but there is no reason why the same code
couldn't be used in those cases, so use instead the more generic identifier
for the PowerPC architecture.

While at it, update the comment to reflect that POWER8/9 have a 128 byte
wide cache line and so the code could instead use 64 byte windows instead
but that possible optimization has been punted for now.

[1] PCRE2Project/pcre2#92

Reviewed By: jhibbits, #powerpc, MaskRay

Differential Revision: https://reviews.llvm.org/D122640
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Apr 5, 2022
@carenas @llvmbot
[compiler-rt] Implement __clear_cache on FreeBSD/powerpc
68ec275 
dd91734 (Add clear_cache implementation for ppc64. Fix buffer to
meet ppc64 alignment., 2017-07-28), adds an implementation for
__builtin___clear_cache on powerpc64, which was promptly ammended to
also be used with big endian mode in f67036b (This ppc64 implementation
of clear_cache works for both big and little endian., 2017-08-02)

clang will use this implementation for it's builtin on FreeBSD and result
in an abort() in the cases where 32-bit generation was requested (ex in
macppc or when the big endian powerpc64 build was done with "-m32") and as
reported[1] recently with pcre2, but there is no reason why the same code
couldn't be used in those cases, so use instead the more generic identifier
for the PowerPC architecture.

While at it, update the comment to reflect that POWER8/9 have a 128 byte
wide cache line and so the code could instead use 64 byte windows instead
but that possible optimization has been punted for now.

[1] PCRE2Project/pcre2#92

Reviewed By: jhibbits, #powerpc, MaskRay

Differential Revision: https://reviews.llvm.org/D122640

(cherry picked from commit 81f5c62)
@carenas
Copy link
Contributor

carenas commented Apr 10, 2022 •
edited
Loading

considering, I heard rumours this is fixed since FreeBSD 13.1-RC1-p1 would be a good idea to close this?

FreeBSD >= 13 might had been the only environment where this could had been triggered, since it uses clang as a base compiler in PowerPC AND supports 32-bit with that compiler (OpenBSD does use clang as a system compiler as well, but only 64-bit).

@pkubaj
Copy link
Author

pkubaj commented Apr 10, 2022

I merged your patch to FreeBSD, so 13.1 is now ok.

Regarding OpenBSD, 6.7 switched to clang on powerpc: https://www.openbsd.org/67.html

tstellar pushed a commit to llvmbot/llvm-project that referenced this issue Apr 19, 2022
@carenas @tstellar
[compiler-rt] Implement __clear_cache on FreeBSD/powerpc
09fba23 
dd91734 (Add clear_cache implementation for ppc64. Fix buffer to
meet ppc64 alignment., 2017-07-28), adds an implementation for
__builtin___clear_cache on powerpc64, which was promptly ammended to
also be used with big endian mode in f67036b (This ppc64 implementation
of clear_cache works for both big and little endian., 2017-08-02)

clang will use this implementation for it's builtin on FreeBSD and result
in an abort() in the cases where 32-bit generation was requested (ex in
macppc or when the big endian powerpc64 build was done with "-m32") and as
reported[1] recently with pcre2, but there is no reason why the same code
couldn't be used in those cases, so use instead the more generic identifier
for the PowerPC architecture.

While at it, update the comment to reflect that POWER8/9 have a 128 byte
wide cache line and so the code could instead use 64 byte windows instead
but that possible optimization has been punted for now.

[1] PCRE2Project/pcre2#92

Reviewed By: jhibbits, #powerpc, MaskRay

Differential Revision: https://reviews.llvm.org/D122640

(cherry picked from commit 81f5c62)
@PhilipHazel
Copy link
Collaborator

Is this issue still live, or can it be closed?

@pkubaj
Copy link
Author

pkubaj commented Jul 31, 2022

It's ok now, thanks!

@pkubaj pkubaj closed this as completed Jul 31, 2022
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
@carenas
[compiler-rt] Implement __clear_cache on FreeBSD/powerpc
9c1f02e 
dd91734 (Add clear_cache implementation for ppc64. Fix buffer to
meet ppc64 alignment., 2017-07-28), adds an implementation for
__builtin___clear_cache on powerpc64, which was promptly ammended to
also be used with big endian mode in f67036b (This ppc64 implementation
of clear_cache works for both big and little endian., 2017-08-02)

clang will use this implementation for it's builtin on FreeBSD and result
in an abort() in the cases where 32-bit generation was requested (ex in
macppc or when the big endian powerpc64 build was done with "-m32") and as
reported[1] recently with pcre2, but there is no reason why the same code
couldn't be used in those cases, so use instead the more generic identifier
for the PowerPC architecture.

While at it, update the comment to reflect that POWER8/9 have a 128 byte
wide cache line and so the code could instead use 64 byte windows instead
but that possible optimization has been punted for now.

[1] PCRE2Project/pcre2#92

Reviewed By: jhibbits, #powerpc, MaskRay

Differential Revision: https://reviews.llvm.org/D122640
@SolitaryGrass SolitaryGrass mentioned this issue May 31, 2023
Closed
arichardson pushed a commit to CTSRD-CHERI/compiler-rt that referenced this issue Sep 12, 2023
@carenas @arichardson
[compiler-rt] Implement __clear_cache on FreeBSD/powerpc
fcea6bc 
dd9173420f06 (Add clear_cache implementation for ppc64. Fix buffer to
meet ppc64 alignment., 2017-07-28), adds an implementation for
__builtin___clear_cache on powerpc64, which was promptly ammended to
also be used with big endian mode in f67036b62c0c (This ppc64 implementation
of clear_cache works for both big and little endian., 2017-08-02)

clang will use this implementation for it's builtin on FreeBSD and result
in an abort() in the cases where 32-bit generation was requested (ex in
macppc or when the big endian powerpc64 build was done with "-m32") and as
reported[1] recently with pcre2, but there is no reason why the same code
couldn't be used in those cases, so use instead the more generic identifier
for the PowerPC architecture.

While at it, update the comment to reflect that POWER8/9 have a 128 byte
wide cache line and so the code could instead use 64 byte windows instead
but that possible optimization has been punted for now.

[1] PCRE2Project/pcre2#92

Reviewed By: jhibbits, #powerpc, MaskRay

Differential Revision: https://reviews.llvm.org/D122640
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@carenas @zherczeg @pkubaj @PhilipHazel