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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
