bruno 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">;
+}
----------------
kristof.beyls wrote:
> bruno wrote:
> > 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.
> Thanks for sharing your experience @bruno .
> I have to confess I do not fully understand "code that needs signing
> capabilities but not want itself to be codegen'd using pauth." I'm assuming
> this means "the ptrauth intrinsics must be available, but the compiler must
> not automatically insert pointer signing/authenticating instructions"?
>
> If I understand that correctly, I'm still wondering if it's useful to have a
> command line switch that removes ptrauth intrinsics, rather than relying on
> ptrauth.h having the appropriate ifdefs to remove intrinsics when targeting
> something where they cannot work?
>
> With the above, I guess "-f(no)ptrauth-intrinsics" then actually means "let
> the compiler automatically insert signing/authenticating instructions as
> defined by the default signing scheme for the target triple you're targeting"?
>
> Maybe my confusion would be less if this patch also adds documentation for
> the command line switch.
> I'm not sure where that documentation would best live. Maybe at
> https://clang.llvm.org/docs/UsersManual.html#command-line-options?
> I have to confess I do not fully understand "code that needs signing
> capabilities but not want itself to be codegen'd using pauth." I'm assuming
> this means "the ptrauth intrinsics must be available, but the compiler must
> not automatically insert pointer signing/authenticating instructions"?
Exactly, I'd like to see such instructions only being emitted via intrinsic
usage.
> If I understand that correctly, I'm still wondering if it's useful to have a
> command line switch that removes ptrauth intrinsics, rather than relying on
> ptrauth.h having the appropriate ifdefs to remove intrinsics when targeting
> something where they cannot work?
I think the `-fnoptrauth-intrinsics` is useful in practice, it provides quick
workarounds when trying to untangle bugs in production, the "cancel previous
added flag" behavior. I do not see a semantic meaning of its existence though.
> With the above, I guess "-f(no)ptrauth-intrinsics" then actually means "let
> the compiler automatically insert signing/authenticating instructions as
> defined by the default signing scheme for the target triple you're targeting"?
We should also be able to use the intrinsics regardless of the target triple
setup (and error out at driver time in case the instructions aren't supported
by the backend).
The problem is that I might still need to disable automatically codegen of
those instructions, but be able to use intrinsics to control their usage. Would
there be another combination of driver flags to that effect?
> Maybe my confusion would be less if this patch also adds documentation for
> the command line switch. I'm not sure where that documentation would best
> live. Maybe at
> https://clang.llvm.org/docs/UsersManual.html#command-line-options?
Agreed!
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
