bruno added a comment.
Thanks for working on upstreaming this @ab. Overall looks good to me, I see
clang-format issues, are those legit? One more comment inline.
================
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">;
+}
----------------
ab wrote:
> 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)
I'd vote for keeping as is: `-fptrauth-intrinsics` allows a nice limited use of
the pauth feature pack. Has actually been useful for us (non-Apple targets) in
code that needs signing capabilities but not want itself to be codegen'd using
pauth - e.g. a dynamic linker for a system that is migrating codebase to pauth
in small steps.
`ptrauth_intrinsics` gates have been equally useful. By doing this downstream
we would need to duplicate this logic as well, so I don't really it benefiting
the community as much.
Enabling the feature when `+pauth` sounds like overall goodness regardless imo.
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
