[PATCH] D41880: Adding nocf_check attribute for cf-protection fine tuning

[Oren Ben Simhon via Phabricator via cfe-commits]

oren_ben_simhon marked 2 inline comments as done.
oren_ben_simhon added a comment.

I added a comment ignoring nocf_check attribute in case -fcf-protection is not set.
Now LLVM is identical to GCC.



================
Comment at: test/Sema/attr-nocf_check.c:18-20
+  FuncPointerWithNoCfCheck fNoCfCheck = f; // no-warning
+  (*fNoCfCheck)();                       // no-warning
+  f = fNoCfCheck;                        // no-warning
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > oren_ben_simhon wrote:
> > > aaron.ballman wrote:
> > > > oren_ben_simhon wrote:
> > > > > aaron.ballman wrote:
> > > > > > oren_ben_simhon wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > These are an error in GCC and I think we should match that behavior. https://godbolt.org/g/r3pf4X
> > > > > > > I will create a warning however in LLVM we don't create an error upon incompatible pointer due to function attribute types.
> > > > > > It should be an error -- Clang does error on this sort of thing when appropriate (which I believe it is, here). For instance, calling convention attributes do this: https://godbolt.org/g/mkTGLg
> > > > > In Clang there is Sema::IncompatiblePointer in case to pointers are not compatible. This flag emits warning message. In the time i check for pointer incompatibility (checkPointerTypesForAssignment()), i don;t have a handle to the attributes. Any suggestion how to implement the exception for nocf_check attribute?
> > > > I believe this is handled in `ASTContext::mergeFunctionType()`. See:
> > > > ```
> > > >   // Compatible functions must have compatible calling conventions
> > > >   if (lbaseInfo.getCC() != rbaseInfo.getCC())
> > > >     return QualType();
> > > > ```
> > > > Somewhere around there is likely where you should be.
> > > I already added there getnocfcheck.
> > > 
> > > After double checking, I see that nocf_check behavior is identical to other function attributes.
> > > For some reason in the clang tests they give warning but in godbolt it gives an error.
> > > I am not sure what is the difference between the flags in godbolt and in my test but this is what causing the warning/error message difference.
> > > 
> > > So basically my behavior is identical to other function type attributes (e.g. no_caller_saved_registers). I believe it is also identical to GCC but i can't prove it because i don't know the flags that godbolt is using.
> > > 
> > You can see the flags being passed in godbolt by passing -v on the command line. FWIW, I get the same error behavior elsewhere as well:
> > 
> > http://coliru.stacked-crooked.com/a/d28234385fa68374
> > https://wandbox.org/permlink/SRLM82l2uJ8q3o1Q
> > 
> > I think you should do some more investigation into what's going on there. Ultimately, I want to avoid clang accepting the `nocf_check` attribute (even with a warning) in cases where GCC doesn't accept it, because that leads to incompatibilities when switching between the two compilers. We should accept what GCC accepts and reject what GCC rejects unless there's a good reason to deviate.
> Ah, I think the distinction here is C++ vs C code. In C, this code should warn, in C++ this code should err. I'm guessing that if you add a C++ test case, the behavior will be to err on this without any other code changes.
Indeed the difference is C++ Vs C. I added a new test that checks for C++ as well.


Repository:
  rL LLVM

https://reviews.llvm.org/D41880