pcc added inline comments.
================
Comment at: include/clang/Driver/Options.td:1035
HelpText<"Instrument function entry only, after inlining, without arguments
to the instrumentation call">;
-
+def finstrument_control_flow : Flag<["-"], "finstrument-control-flow">,
+ Group<f_Group>, Flags<[CC1Option]>,
----------------
oren_ben_simhon wrote:
> pcc wrote:
> > oren_ben_simhon wrote:
> > > pcc wrote:
> > > > oren_ben_simhon wrote:
> > > > > pcc wrote:
> > > > > > My comments about the attribute name in D40482 also apply to this
> > > > > > flag name. GCC appears to have chosen a better name for this flag,
> > > > > > `-mcet`. Can we use that name instead?
> > > > > Again, I am sorry for the late response, I was off for about a week.
> > > > > GCC is only using -mcet as a super set for -mibt and -mshstk. I am
> > > > > planning to add it in a different review.
> > > > > -mcet/-mibt/-mshstk only enables the HW technology (ISA/Intrinsics).
> > > > > GCC is using -cf-protection for actually instrumenting the control
> > > > > flow.
> > > > > I think it will be best to stay coherent with GCC (and ICC and soon
> > > > > MS) and use -cf-protection.
> > > > > What do you think?
> > > > I see. As far as I can tell, there are no released versions of GCC and
> > > > ICC with the new flags, so I think there is still a possibility to
> > > > change them. (MS is a different story, their flags are incompatible
> > > > anyway.)
> > > >
> > > > I took a closer look at what GCC does, and here are my thoughts:
> > > > - As far as I can tell, IBT does not introduce any new instructions
> > > > other than ENDBRANCH, and it doesn't seem to make sense for ENDBRANCH
> > > > to be exposed via intrinsics (but rather via attributes, as you have
> > > > done). That means that `-mibt` basically does nothing except for
> > > > allowing the use of `-fcf-protection`.
> > > > - SHSTK introduces ISA extensions that seem reasonable to expose via
> > > > intrinsics, so it would probably be fine to have a `-mshstk` flag that
> > > > enables the intrinsics.
> > > > - In GCC, `-fcf-protection` means "emit code that is compatible with
> > > > CET" and requires that the user pass `-mibt` or `-mshstk`. But the code
> > > > that it emits is also compatible with non-CET processors, so it doesn't
> > > > make sense to me to require the use of flags such as `-mshstk` which
> > > > imply that SHSTK is required.
> > > >
> > > > As for the name of the flag: there is nothing about the name
> > > > `-fcf-protection` that implies hardware-based protection, so the
> > > > presence of this flag could confuse users who want software-based
> > > > protection. The hardware-based protection is sort of implied by the
> > > > requirement to pass `-mibt` or `-mshstk`, but that is a little awkward,
> > > > and I don't think it makes sense either for the reasons I mentioned.
> > > > What we want is a clear way to say "I want to generate code that is
> > > > compatible with (but does not require) this hardware feature", but
> > > > there doesn't seem to be any precedent for how to express that. The
> > > > most straightforward way to express it is to mention the name of the
> > > > hardware feature in the flag.
> > > >
> > > > So here are the changes that I would propose across compilers:
> > > > - Remove the `-mibt` flag.
> > > > - Change the flag that emits ENDBRANCH to something like
> > > > `-mcet-compatible` and stop requiring the use of other flags.
> > > > Please let me know what you think.
> > > It is true that -mibt doesn't enable intrinsics however it is required
> > > for the case of inline asm.
> > > The code that -cf-protection produce is not always compatible with
> > > non-CET processors.
> > > For example, consider the required fix to the shadow stack in the case of
> > > setJmp/longJmp.
> > > In that case, we need to use SHSTK instructions (like incssp) that are
> > > not compatible.
> > > Would the flag -fcf-arch-protection make more sense (as it implies on HW
> > > support)?
> > > It is true that -mibt doesn't enable intrinsics however it is required
> > > for the case of inline asm.
> >
> > For enabling ENDBRANCH in inline asm? Would there be any harm in allowing
> > ENDBRANCH unconditionally?
> >
> > > For example, consider the required fix to the shadow stack in the case of
> > > setJmp/longJmp.
> > > In that case, we need to use SHSTK instructions (like incssp) that are
> > > not compatible.
> >
> > I don't think that is true. I looked at the code that GCC emits for
> > `setjmp` and `longjmp`, and it appears to quite cleverly avoid the need to
> > execute SHSTK-required instructions on older processors by taking advantage
> > of the fact that RDSSP is a NOP on older processors.
> >
> > With this program:
> > ```
> > #include <setjmp.h>
> >
> > jmp_buf env;
> > void f() {
> > __builtin_setjmp(env);
> > }
> >
> > void g() {
> > __builtin_longjmp(env, 1);
> > }
> > ```
> > I get this asm for saving the SSP:
> > ```
> > movl $0, %eax
> > rdsspq %rax
> > movq %rax, env+24(%rip)
> > ```
> > and this asm for restoring it:
> > ```
> > movl $0, %eax
> > rdsspq %rax
> > subq env+24(%rip), %rax
> > je .L5
> > negq %rax
> > shrq $3, %rax
> > cmpq $255, %rax
> > jbe .L6
> > .L7:
> > incsspq %rax
> > subq $255, %rax
> > cmpq $255, %rax
> > ja .L7
> > .L6:
> > incsspq %rax
> > .L5:
> > ```
> > So on a non-SHSTK processor, we will store 0 to the slot reserved for SSP
> > during `setjmp`, and then compare 0 to 0 during `longjmp` and bypass the
> > entire loop.
> >
> > > Would the flag -fcf-arch-protection make more sense (as it implies on HW
> > > support)?
> >
> > It isn't ideal to me, but it does at least seem better than
> > `-fcf-protection`.
> You are correct, -mshstk flag should be used only for enabling the intrinsics
> that are not rdssp.
> It shouldn't be a requirement for generating shadow stack fixes. Is that what
> you meant?
Yes, exactly.
Repository:
rL LLVM
https://reviews.llvm.org/D40478
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
