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

[Oren Ben Simhon via Phabricator via cfe-commits]

oren_ben_simhon marked 7 inline comments as done.
oren_ben_simhon added inline comments.


================
Comment at: include/clang/CodeGen/CGFunctionInfo.h:519
+  /// Whether this function has nocf_check attribute.
+  unsigned NoCfCheck : 1;
+
----------------
aaron.ballman wrote:
> This is unfortunate -- it bumps the bit-field over 32 bits. Can the bit be stolen from elsewhere?
The field is orthogonal to the other fields moreover i think that double meaning of the same field will lead to future bugs. The class is not a compact packed structure, so i don't feel it worth the confusion.


================
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
----------------
aaron.ballman wrote:
> Can you use the `LangOpts` field to specify this in Attr.td? Then you can go back to the simple handler.
When using LangOpts field in Attr.td, the diagnostic warning will not be descriptive as i use here (use -fcf-protection flag...).


================
Comment at: test/Sema/attr-nocf_check.cpp:1
+// RUN: %clang_cc1 -verify -fcf-protection=branch -target-feature +ibt -fsyntax-only %s
+
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > For better test coverage, you can switch to the `[[gnu::nocf_check]]` spelling in this file and pass `-std=c++11`
> This one also likely needs an explicit triple.
Since it doesn't test the functionality of this specific attribute, I believe it is an overkill to switch to [[gnu::nocf_check]] spelling.


Repository:
  rL LLVM

https://reviews.llvm.org/D41880