aaron.ballman added a comment. One thing I notice is that GCC trunk requires passing the `-fcf-protection` flag in order to enable the attribute. Do we wish to do the same?
================ Comment at: include/clang/Basic/Attr.td:2089 +def AnyX86NoCfCheck : InheritableAttr, TargetSpecificAttr<TargetAnyX86>{ + let Spellings = [GCC<"nocf_check">]; + let Documentation = [AnyX86NoCfCheckDocs]; ---------------- oren_ben_simhon wrote: > aaron.ballman wrote: > > This attribute doesn't appear to be supported by GCC. Did you mean to use > > the Clang spelling instead? > Attribute documentation can be found here: > https://gcc.gnu.org/onlinedocs/gcc/x86-Function-Attributes.html#x86-Function-Attributes > Ah, it was hiding in there -- my apologies! ================ Comment at: lib/Sema/SemaDeclAttr.cpp:1990 -bool Sema::CheckNoReturnAttr(const AttributeList &Attrs) { - if (!checkAttributeNumArgs(*this, Attrs, 0)) { - Attrs.setInvalid(); +static void handleNoCfCheckAttr(Sema &S, Decl *D, const AttributeList &attr) { + if (S.CheckAttrTarget(attr) || S.CheckAttrNoArgs(attr)) ---------------- aaron.ballman wrote: > `attr` doesn't follow the proper naming conventions. Please don't name the parameter variable after a type -- that can confuse some editors. ================ Comment at: lib/Sema/SemaDeclAttr.cpp:2007 + +bool Sema::CheckAttrNoArgs(const AttributeList &Attr) { + if (!checkAttributeNumArgs(*this, Attr, 0)) { ---------------- oren_ben_simhon wrote: > craig.topper wrote: > > Wy did this get renamed? > To reuse existing code. The same check is performed in several attributes so > instead of writing this block for every attribute i only call this function. I think this may be the wrong approach -- this code seems like it should be handled automatically in `handleCommonAttributeFeatures()`. We already check whether the attribute appertains to its subjects and other table-gennable predicate checks, and this seems like it would be another instance of that kind of situation. I think you need to retain this function (because it's called for type attributes, which don't have automated checking yet), but call it from `handleCommonAttributeFeatures()` rather than call it manually. ================ Comment at: lib/Sema/SemaDeclAttr.cpp:1968-1969 - if (S.CheckNoReturnAttr(Attrs)) + if (S.CheckAttrNoArgs(Attrs)) return; ---------------- I think this can be removed entirely -- it should be handled automatically by the common handler (double-check that there's test coverage for it). ================ Comment at: lib/Sema/SemaDeclAttr.cpp:1983 const AttributeList &Attr) { - if (S.CheckNoCallerSavedRegsAttr(Attr)) + if (S.CheckAttrTarget(Attr) || S.CheckAttrNoArgs(Attr)) return; ---------------- I think you can drop the `CheckAttrNoArgs()` call from here as well. And once `CheckAttrTarget()` is moved to the common handler, this function can also be replaced by a call to `handleSimpleAttribute<>()` ================ Comment at: lib/Sema/SemaDeclAttr.cpp:1990 -bool Sema::CheckNoReturnAttr(const AttributeList &Attrs) { - if (!checkAttributeNumArgs(*this, Attrs, 0)) { - Attrs.setInvalid(); +static void handleNoCfCheckAttr(Sema &S, Decl *D, const AttributeList &Attr) { + if (S.CheckAttrTarget(Attr)) ---------------- Once you move the target check into the common predicate function, this function can go away and you can use `handleSimpleAttribute<>()` instead. ================ Comment at: test/Sema/attr-nocf_check.c:18-20 + FuncPointerWithNoCfCheck fNoCfCheck = f; // no-warning + (*fNoCfCheck)(); // no-warning + f = fNoCfCheck; // no-warning ---------------- These are an error in GCC and I think we should match that behavior. https://godbolt.org/g/r3pf4X 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