Skip to content

compiler-rt: Introduce runtime functions for emulated PAC. #133530

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

Open
wants to merge 6 commits into
base: users/pcc/spr/main.compiler-rt-introduce-runtime-functions-for-emulated-pac
Choose a base branch
from

Conversation

pcc
Copy link
Contributor

@pcc pcc commented Mar 28, 2025

The emulated PAC runtime functions emulate the ARMv8.3a pointer
authentication instructions and are intended for use in heterogeneous
testing environments. For more information, see the associated RFC:
https://discourse.llvm.org/t/rfc-emulated-pac/85557

Created using spr 1.3.6-beta.1
Copy link

github-actions bot commented Mar 28, 2025

✅ With the latest revision this PR passed the undef deprecator.

* Header File
* Copyright (C) 2012-2023 Yann Collet
*
* BSD 2-Clause License (https://www.opensource.org/licenses/bsd-license.php)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a license different from Apache-2.0 WITH LLVM-exception.
Therefore, the process described at https://llvm.org/docs/DeveloperPolicy.html#copyright-license-and-patents should be followed to check whether this is acceptable in this specific case.

That being said, xxhash is already present under llvm/lib/Support/xxhash.cpp, as you pointed out in the RFC.

Making sure we don't have multiple copies of non-Apache-2.0 WITH LLVM-exception code would be preferable. I'll tag @beanz, as he had ideas about how to better structure vendored third party code in LLVM.

I'm not sure if moving non-Apache-2.0 WITH LLVM-exception licensed code to a run-time library (for the first time?) triggers new concerns.

I'll note that other hashing algorithms, such as Blake3 and SipHash, which are available under the Apache-2.0 WITH LLVM-exception, are also already present in the LLVM Support library.
Would one of these preferably-licensed hashing algorithms be a good fit for the hashing functionality needed for this use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will contact board@llvm.org about this new usage.

I don't have a strong opinion about the hash algorithm. XXH3_64 was chosen for these reasons:

  1. It was the easiest to incorporate into compiler-rt as it was already available as a header-only library without dependencies.
  2. It was already being used in LLVM, so I imagined that licensing wouldn't be as much of a concern.
  3. It was faster than the other algorithms according to the xxhash homepage. BLAKE3 isn't listed there but we can extrapolate from BLAKE3's claim of being ~7x faster than BLAKE2. (I know I said elsewhere that performance is less of a concern for these functions, but all other things being equal, we may as well go with the algorithm with the best performance.)

If the board decides not to allow use in compiler-rt it should be possible to switch to one of the other algorithms after changing its code as needed to remove dependencies.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The introduction of BSD-licensed code to compiler-rt (and especially to compiler-rt's builtins library) poses significant licensing difficulty.

The BSD 2-clause license requires attribution when code is distributed in object form as well as source form, and since the compiler-rt builtins are linked into binaries built with LLVM (not just LLVM itself), the impact of the license here would be transitive to products built using LLVM-based compilers.

This is extremely undesirable, and as such this change will need to replace the xxhash with an alternative hash (ideally under the LLVM license) in order to be integrated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, I've uploaded a new change that replaces the hash with SipHash.

Unfortunately SipHash is subtantially slower than xxhash and led to a ~3.5x slowdown (vs native PAC instructions) on the benchmark cited in my original RFC (and BLAKE3 isn't header only which would make it substantially harder to integrate here). Hopefully the performance hit doesn't turn out to be a problem here.

@@ -0,0 +1,115 @@
#include <stdint.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This new file needs a license header, I guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, I forgot to add one. Will add.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

uint64_t __emupac_autda_impl(uint64_t ptr, uint64_t disc) {
if (pac_supported()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth writing a lazy ifunc-like resolver for this so it doesn't have to be checked on every call. Something like:

typedef BOOL (*fptr_ty)(void);

static BOOL resolver(void);

// Should be signed with address diversity
fptr_ty __emupac_autda_impl = resolver;

static BOOL resolver(void) {
  if (pac_supported())
    __emupac_autda_impl = __emupac_autda_pac;
  else
    __emupac_autda_impl = __emupac_autda_nopac;

  return __emupac_autda_impl();
}

If we had FMV support for PAC I'd suggest __attribute__((target_version("pac"))). cc @labrinea

You might also want to consider giving __emupac_autda a preserves-none calling convention, so you don't have to save/restore around them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with using ifuncs here is that this function will itself be called from ifunc resolvers (see this), so that could lead to order of initialization issues.

Since performance is not that important for this function's use case I didn't try to avoid checking every time or experiment with other calling conventions. Also, if compiler-rt needs to be buildable with non-Clang compilers, I'm not sure that we can use the other calling conventions anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

The choice of calling convention to use will probably affect register allocation significantly in the caller functions. I wonder if it could hide some subtle codegen issues. On the other hand, it may uncover other ones :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We haven't noticed codegen issues like this in our internal testing (other than the performance hit which is expected).

Copy link
Collaborator

@llvm-beanz llvm-beanz left a comment

Choose a reason for hiding this comment

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

We cannot introduce BSD-licensed code to compiler-rt. Please see my comment for more details.

Created using spr 1.3.6-beta.1
Copy link

github-actions bot commented Apr 3, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@pcc
Copy link
Contributor Author

pcc commented Apr 3, 2025

We cannot introduce BSD-licensed code to compiler-rt. Please see my comment for more details.

Thanks, resolved by switching to SipHash. #134197 extracts the existing SipHash implementation into a header that can be included here.

pcc added a commit to pcc/llvm-project that referenced this pull request Apr 3, 2025
The emulated PAC runtime functions emulate the ARMv8.3a pointer
authentication instructions and are intended for use in heterogeneous
testing environments. For more information, see the associated RFC:
https://discourse.llvm.org/t/rfc-emulated-pac/85557

TODO:
- Add tests.
- Figure out how to get emupac.cpp to build with CMake. For some reason
  any .cpp files added to the builtins library are ignored. So for now
  only the gn build works.

Pull Request: llvm#133530
Created using spr 1.3.6-beta.1
pcc added a commit to pcc/llvm-project that referenced this pull request Apr 4, 2025
The emulated PAC runtime functions emulate the ARMv8.3a pointer
authentication instructions and are intended for use in heterogeneous
testing environments. For more information, see the associated RFC:
https://discourse.llvm.org/t/rfc-emulated-pac/85557

TODO:
- Add tests.

Pull Request: llvm#133530
pcc added 3 commits April 7, 2025 20:45
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
@llvm-beanz llvm-beanz self-requested a review April 28, 2025 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

6 participants