[PATCH] D40478: Added Instrument Control Flow Flag

Wed Dec 13 09:55:38 PST 2017

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