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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2691
+def warn_nocf_check_attribute_ignored :
+  Warning<"nocf_check attribute ignored. Use -fcf-prtection flag to enable it">,
+  InGroup<IgnoredAttributes>;
----------------
Diagnostics are not complete sentences, so the punctuation and capitalization should go away. Typo in the flag spelling. The attribute name should be quoted. I'd go with: `"'nocf_check' attribute ignored; use -fcf-protection to enable the attribute"`


================
Comment at: include/clang/CodeGen/CGFunctionInfo.h:519
+  /// Whether this function has nocf_check attribute.
+  unsigned NoCfCheck : 1;
+
----------------
This is unfortunate -- it bumps the bit-field over 32 bits. Can the bit be stolen from elsewhere?


================
Comment at: include/clang/Sema/Sema.h:3331-3332
                             const FunctionDecl *FD = nullptr);
-  bool CheckNoReturnAttr(const AttributeList &attr);
-  bool CheckNoCallerSavedRegsAttr(const AttributeList &attr);
+  bool CheckAttrTarget(const AttributeList &Attr);
+  bool CheckAttrNoArgs(const AttributeList &Attr);
   bool checkStringLiteralArgumentAttr(const AttributeList &Attr,
----------------
Please don't name the parameter after a type.


================
Comment at: lib/Frontend/CompilerInvocation.cpp:1999
+    StringRef Name = A->getValue();
+    if ((Name == "full") || (Name == "branch")) {
+      Opts.CFProtectionBranch = 1;
----------------
Can elide some parens.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:1979-1980
+static void handleNoCfCheckAttr(Sema &S, Decl *D, const AttributeList &Attrs) {
+  if (!S.getLangOpts().CFProtectionBranch)
+    S.Diag(Attrs.getLoc(), diag::warn_nocf_check_attribute_ignored);
+  else
----------------
Can you use the `LangOpts` field to specify this in Attr.td? Then you can go back to the simple handler.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:1985
 
-bool Sema::CheckNoReturnAttr(const AttributeList &Attrs) {
-  if (!checkAttributeNumArgs(*this, Attrs, 0)) {
-    Attrs.setInvalid();
+bool Sema::CheckAttrNoArgs(const AttributeList &Attr) {
+  if (!checkAttributeNumArgs(*this, Attr, 0)) {
----------------
Don't name the parameter after a type.


================
Comment at: test/Sema/attr-nocf_check.cpp:1
+// RUN: %clang_cc1 -verify -fcf-protection=branch -target-feature +ibt -fsyntax-only %s
+
----------------
For better test coverage, you can switch to the `[[gnu::nocf_check]]` spelling in this file and pass `-std=c++11`


Repository:
  rL LLVM

https://reviews.llvm.org/D41880