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

MDEV-34815 SIGILL error when executing mariadbd compiled for RISC-V with Clang #3661

Merged
merged 4 commits into from
Dec 4, 2024

Conversation

grooverdan
Copy link
Member

  • The Jira issue number for this PR is: MDEV-______

Description

RISC-V and Clang produce rdcycle for __builtin_readcyclecounter.

Since Linux kernel 6.6 this is a privileged instruction not available to userspace programs.

The use of __builtin_readcyclecounter is excluded from RISCV falling back to the rdtime/rdtimeh instructions provided in MDEV-33435.

Author: BINSZ on JIRA

Release Notes

Correct SIGILL on RISC-V for Linux kernels 6.6+ when compiled with clang.

How can this PR be tested?

Environment as above.

Basing the PR against the correct MariaDB version

  • This is a new feature or a refactoring, and the PR is based against the main branch.
  • This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

  • I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

@grooverdan grooverdan requested a review from vuvova November 25, 2024 22:43
On clang we use __builtin_readcyclecounter(), except for AARCH64.
On clang we use __builtin_readcyclecounter(), except for AARCH64 and RISC-V.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to see a justification for these exceptions, such as links to clang bug reports. I found that the RISC-V implementation was added in https://reviews.llvm.org/D64125 but I found no bug report about this. Can you please file one?

For ARMv8 there already is a bug report llvm/llvm-project#43652 where it is suggested that the instruction could work depending on the operating system kernel configuration.

These optionally privileged instructions could be usable if on the affected ISA we made my_timer_cycles() a function pointer. An initialization function could try to run the instruction under a SIGILL wrapper.

Copy link
Member Author

@grooverdan grooverdan Nov 26, 2024

Choose a reason for hiding this comment

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

Quite right, was thinking the same. Clang isn't just for kernels and root processes. I did compiler export the latest version and the behaviour is still there.

llvm/llvm-project#117701

@cvicentiu cvicentiu added the MariaDB Foundation Pull requests created by MariaDB Foundation label Nov 26, 2024
@@ -152,7 +152,7 @@ C_MODE_START
*/
static inline ulonglong my_timer_cycles(void)
{
# if __has_builtin(__builtin_readcyclecounter) && !defined (__aarch64__)
# if __has_builtin(__builtin_readcyclecounter) && !defined (__aarch64__) && !defined(__riscv)

Choose a reason for hiding this comment

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

This should be gated on linux, other operating systems such as FreeBSD don't make this inaccessible. Quite surprising to see that x86 rdtsc is permitted but the aarch64 counters are not.

Copy link
Member Author

@grooverdan grooverdan Nov 27, 2024

Choose a reason for hiding this comment

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

Noted, adjusted. Seems the Aarch64 was removed for performance reasons instead rather than Kernel limitation. Oops mea cupla. It was previous (<14.0.0) clang mapping of __builltin_readcycle counter that mapped to PMCCNTR_EL0 resulting in SIGILL - MDEV-23249).

…ith Clang

RISC-V and Clang produce rdcycle for __builtin_readcyclecounter.

Since Linux kernel 6.6 this is a privileged instruction not available
to userspace programs.

The use of __builtin_readcyclecounter is excluded from RISCV falling
back to the rdtime/rdtimeh instructions provided in MDEV-33435.

Thanks Alexander Richardson for noting it should be linux only in the
code and noting FreeBSD RISC-V permits rdcycle.

Author: BINSZ on JIRA
@grooverdan grooverdan force-pushed the MDEV-34815-no-rdcycle-clang branch from 1e0434a to 1d1520b Compare November 27, 2024 06:42
Copy link
Member

@knielsen knielsen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks, Daniel

@grooverdan grooverdan enabled auto-merge (rebase) December 4, 2024 14:57
@grooverdan grooverdan merged commit aca72b3 into MariaDB:10.11 Dec 4, 2024
11 of 13 checks passed
@grooverdan grooverdan deleted the MDEV-34815-no-rdcycle-clang branch December 4, 2024 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MariaDB Foundation Pull requests created by MariaDB Foundation
Development

Successfully merging this pull request may close these issues.

5 participants