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-35566 Ensure compatibility with ARMv9 by updating .arch directive #3675

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cythe
Copy link

@cythe cythe commented Dec 4, 2024

The pmem_cvap() function currently uses the '.arch armv8.2-a' directive for the 'dc cvap' instruction. This will cause build errors below when compiling for ARMv9 systems. Update the '.arch' directive to 'armv9.4-a' to ensure compatibility with ARMv9 architectures.

{standard input}: Assembler messages:
{standard input}:169: Error: selected processor does not support retaa' {standard input}:286: Error: selected processor does not support retaa' make[2]: *** [storage/innobase/CMakeFiles/innobase_embedded.dir/build.make: 1644: storage/innobase/CMakeFiles/innobase_embedded.dir/sync/cache.cc.o] Error 1

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

Description

The pmem_cvap() function currently uses the '.arch armv8.2-a' directive
for the 'dc cvap' instruction. This will cause build errors below when
compiling for ARMv9 systems. Update the '.arch' directive to 'armv9.4-a'
to ensure compatibility with ARMv9 architectures.

{standard input}: Assembler messages:
{standard input}:169: Error: selected processor does not support `retaa'
{standard input}:286: Error: selected processor does not support `retaa'
make[2]: *** [storage/innobase/CMakeFiles/innobase_embedded.dir/build.make:
1644: storage/innobase/CMakeFiles/innobase_embedded.dir/sync/cache.cc.o]
Error 1

Release Notes

TODO: What should the release notes say about this change?
Include any changed system variables, status variables or behaviour. Optionally list any https://mariadb.com/kb/ pages that need changing.

How can this PR be tested?

TODO: modify the automated test suite to verify that the PR causes MariaDB to behave as intended.
Consult the documentation on "Writing good test cases".

If the changes are not amenable to automated testing, please explain why not and carefully describe how to test manually.

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.

The pmem_cvap() function currently uses the '.arch armv8.2-a' directive
for the 'dc cvap' instruction. This will cause build errors below when
compiling for ARMv9 systems. Update the '.arch' directive to 'armv9.4-a'
to ensure compatibility with ARMv9 architectures.

{standard input}: Assembler messages:
{standard input}:169: Error: selected processor does not support `retaa'
{standard input}:286: Error: selected processor does not support `retaa'
make[2]: *** [storage/innobase/CMakeFiles/innobase_embedded.dir/build.make:
1644: storage/innobase/CMakeFiles/innobase_embedded.dir/sync/cache.cc.o]
Error 1

Signed-off-by: Ruiqiang Hao <[email protected]>
@CLAassistant
Copy link

CLAassistant commented Dec 4, 2024

CLA assistant check
All committers have signed the CLA.

@cythe
Copy link
Author

cythe commented Dec 4, 2024 via email

__asm__ __volatile__(".arch armv8.2-a\n dc cvap, %0" :: "r"(u) : "memory");
__asm__ __volatile__(".arch armv9.4-a\n dc cvap, %0" :: "r"(u) : "memory");
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for this fix! Because I am not very familiar with this architecture and toolchains, I played with https://godbolt.org using a modified version of their default example:

#include <cstdint>
uintptr_t square(uintptr_t num) {
      __asm__ __volatile__(".arch armv9.4-a\n dc cvap, %0" :: "r"(num) : "memory");
    return num * num;
}

GCC would just blindly emit the __asm__ snippet to an external assembler, even something nonsensical like dc cvap, eax on x86. I have to enable "compile to binary" to actually invoke an assembler and show the disassembled output. And that is where it seems to be failing for me, even with GCC 14.2.0:

/tmp/ccie1qI1.s: Assembler messages:
/tmp/ccie1qI1.s:21: Error: unknown architecture `armv9.4-a'

I don’t know which assembler version they are using, though. We seem to have a similar problem on our CI as well, for example in https://buildbot.mariadb.org/#/builders/830/builds/458/steps/5/logs/stdio:

/tmp/ccHlQ7lL.s: Assembler messages:
/tmp/ccHlQ7lL.s:79: Error: unknown architecture `armv9.4-a'
/tmp/ccHlQ7lL.s:80: Error: selected processor does not support system register name 'cvap'
make[2]: *** [storage/innobase/CMakeFiles/innobase.dir/build.make:1616: storage/innobase/CMakeFiles/innobase.dir/sync/cache.cc.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:5340: storage/innobase/CMakeFiles/innobase.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

The oldest version of clang that recognizes .arch armv9.4-a is clang-16. At least back to clang-9, the sample program with .arch armv8.2-a would compile.

I think that we must support reasonably old compiler versions than that. For proper clang support, I suppose that we could check __clang_major__. But I see that there could be an even better trick:

#include <cstdint>
uintptr_t square(uintptr_t num) {
#if defined __ARM_ARCH && __ARM_ARCH == 9
      __asm__ __volatile__(".arch armv9.4-a\n dc cvap, %0" :: "r"(num) : "memory");
#else
      __asm__ __volatile__(".arch armv8.2-a\n dc cvap, %0" :: "r"(num) : "memory");
#endif
    return num * num;
}

Can you please revise this accordingly? Please rebase the fix on the 10.11 branch and edit the target branch of the pull request to that. That is the oldest version where this fix is applicable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: Not all our CI builders are reporting their status to GitHub. I found the above build failure in the grid view of this pull request.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your prompt reply.

This part of the code is compiled for the arm64 structure, so it is pointless to compile this code under x86 and arm32. Here is a simplified part of the code cache.cc file:

#include <cstdint>

#if defined __x86_64__ || defined __aarch64__ || defined __powerpc64__

/* x86_64 code */

#elif defined __aarch64__

/* ... */
__asm__ __volatile__(".arch armv9.4-a\n dc cvap, %0" :: "r"(num) : "memory");
/* ... */

#elif defined __powerpc64__

/* powerpc64 code */

#endif

I see, we want to compile on older versions of the compiler, right? I will resend a pull request later.

@vuvova vuvova changed the title Ensure compatibility with ARMv9 by updating .arch directive MDEV-35566 Ensure compatibility with ARMv9 by updating .arch directive Dec 4, 2024
@cvicentiu cvicentiu added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements.
Development

Successfully merging this pull request may close these issues.

4 participants