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

[Aaron Ballman via Phabricator via cfe-commits]

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