ab added inline comments.
================
Comment at: clang/include/clang/Driver/Options.td:2865-2872
+let Group = f_Group in {
+ let Flags = [CC1Option] in {
+ def fptrauth_intrinsics : Flag<["-"], "fptrauth-intrinsics">,
+ HelpText<"Enable pointer-authentication intrinsics">;
+ }
+ def fno_ptrauth_intrinsics : Flag<["-"], "fno-ptrauth-intrinsics">;
+}
----------------
rjmccall wrote:
> kristof.beyls wrote:
> > My impression is that generally for `__builtin_XXX` intrinsics, there are
> > no compiler flags to make them available or remove their availability.
> > Is there a good reason why a command line option is needed for the
> > `__builtin_ptrauth` intrinsics, but not (IIUC) for most or any other
> > existing `__builtin_XXX` intrinsic?
> > If there is no good reason, it seems better to me to not have a command
> > line option so there is better consistency across all `__builtin_XXX`
> > intrinsics?
> >
> > (after having read more of the patch): my impression has changed now that
> > the f(no-)ptrauth-intrinsics flag rather selects whether the ptrauth
> > intrinsics get lowered to PAuth hardware instructions, or to "regular"
> > instructions emulating the behavior of authenticated pointers. If that is
> > correct (and assuming it's a useful option to have), I would guess a
> > different name for the command line option could be less misleading. As is,
> > it suggests this selects whether ptrauth_ intrinsics are available or not.
> > If instead, as I'm guessing above, this selects whether ptrauth_ intrinsics
> > get lowered to PAuth instructions or not, maybe something like
> > '-femulate-ptrauth' would describe the effect of the command line switch a
> > bit better?
> The ptrauth features were implemented gradually, beginning with the
> intrinsics. Originally we needed a way to enable the intrinsics feature
> without relying on target information. We do still need a way to enable them
> without necessarily enabling automatic type/qualifier-based pointer
> authentication. I don't know if we need to be able to *disable* them when
> the target supports them; I agree that that would be a little strange.
>
> If not, we could just enable the intrinsics whenever either the target says
> they're okay or software emulation (a separate, experimental feature) is
> enabled. The AArch64 target has a `+pauth` target feature. However, I don't
> know if `-arch arm64e` actually adds that feature on Apple targets. Also,
> the `HasPAuth` field in the clang `TargetInfo` does not appear to be properly
> initialized to `false` when `+pauth` *isn't* present; fortunately, that field
> never used.
>
> I'm not sure if it would actually be okay to remove the
> `-fptrauth-intrinsics` driver option if we just enabled the intrinsics based
> on the target feature. That does feel cleaner, but unfortunately, we at
> Apple probably have explicit uses of the option that we'd have to clean up
> before we could remove the option. We could treat that as an Apple problem
> and keep it out of the open source tree, though, and maybe remove the option
> altogether someday.
>
> Ahmed, thoughts?
Hmm, I agree it would be strange to need to disable the intrinsics, but we do
also gate the various higher-level qualifiers (and intrinsics) on
`ptrauth_intrinsics`. So, in `ptrauth.h` (and in various users) the feature
now really means "we're in a 'ptrauth-aware' environment". And it does make
more sense to keep that separate from "we're running on a CPU that
theoretically could support ptrauth". It comes down to what "ptrauth-aware"
really means, and that's probably also an Apple problem, and all current users
of `ptrauth_intrinsics` should use something like `__arm64e__` instead.
That still means there's no equivalent for other targets and/or software
emulation, but that seems okay: `ptrauth.h` already needs changes to be usable
from anywhere other than arm64e (cf. the discussion about keys), and we can
cross that bridge when we get there.
(One could argue that all the language-feature-specific qualifiers and
intrinsics should be gated on the appropriate ptrauth_whatever feature, but the
qualifiers are often used in precisely the glue/runtime code that doesn't build
in the appropriate mode, so doesn't have the feature enabled.)
So, concretely, we could:
- continue gating these plain intrinsics on `ptrauth_intrinsics` in ptrauth.h
(IIRC there's an ACLE feature macro but it's specific to return address signing
and BTI defaults; I'll check)
- enable the feature when `+pauth`
- replace all other uses of `ptrauth_intrinsics` with `__arm64e__`, in both
ptrauth.h (gating the ABI qualifiers) and gradually in our internal codebases
and keep `-fptrauth-intrinsics` downstream for the transition (or, depending on
how much the flag is really used, just keep the old behavior of enabling the
feature for arm64e only; but yeah, that's my downstream problem)
================
Comment at: clang/lib/Headers/ptrauth.h:19-37
+ /* A process-independent key which can be used to sign code pointers.
+ Signing and authenticating with this key is a no-op in processes
+ which disable ABI pointer authentication. */
+ ptrauth_key_process_independent_code = ptrauth_key_asia,
+
+ /* A process-specific key which can be used to sign code pointers.
+ Signing and authenticating with this key is enforced even in processes
----------------
kristof.beyls wrote:
> rjmccall wrote:
> > kristof.beyls wrote:
> > > I think, but am not sure, that the decision of which keys are process
> > > independent and which ones are process-dependent is a software platform
> > > choice?
> > > If so, maybe ptrauth_key_process_{in,}dependent_* should only get defined
> > > conditionally?
> > > I'm not sure if any decisions have been taken already for e.g. linux,
> > > Android, other platforms.
> > > If not, maybe this block of code should be surrounded by an ifdef that is
> > > enabled only when targeting Darwin?
> > Yes, in retrospect it was a bad idea to define these particular generic
> > names. I believe Apple platforms no longer have "process-independent"
> > keys. It should really just be (1) the concrete keys, (2) recommended
> > default keys for code and data pointers, and then (3) the specific keys
> > used in specific schemas. Beyond that, if people want a specific different
> > key for some purpose, they should ask for it.
> >
> > Unfortunately, there's already a fair amount of code using these names. We
> > could deprecate the old names and point people towards the new names,
> > though.
> Thanks for those background insights!
> I was thinking that maybe the keys that should be deprecated could be enabled
> only when targeting Apple platforms? I'm assuming here that most existing
> code using these only target Apple platforms; so making them available only
> when targeting Apple platforms could help with not letting the use of them
> spread further without impacting existing code much?
Or we could keep these downstream as well, and deprecate them there directly.
Worth mentioning we'll have a similar problem with the
"language-feature-specific" key enums and qualifiers (e.g.,
`ptrauth_key_function_pointer`): they're currently hardcoded in ptrauth.h with
the arm64e values. I was thinking maybe they should be defined by the frontend
and exposed to ptrauth.h through macros (and/or builtin types, or something
else), so that the single source of truth is the frontend logic that decides
which schemas are used where. But that's a problem for another day.
So, concretely:
- remove these 4 key aliases (and transition them downstream, yadda yadda)
- gate the later key aliases (that describe the language ABI schemas) on
`__arm64e__`
- figure out some more complicated way of defining these, as needed, for other
targets
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112941/new/
https://reviews.llvm.org/D112941
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
