[PATCH] D72167: Add support for __declspec(guard(nocf))

[Andrew Paverd via Phabricator via llvm-commits]

ajpaverd added a comment.

Thanks for the feedback @aaron.ballman and @dmajor!



================
Comment at: clang/include/clang/Basic/Attr.td:2914
+  let Spellings = [Declspec<"guard">];
+  let Subjects = SubjectList<[Function]>;
+  let Args = [EnumArgument<"Guard", "GuardArg", ["nocf"], ["nocf"]>];
----------------
aaron.ballman wrote:
> Should we also support ObjC methods? What about things like lambdas or blocks?
> 
> (Note, these could all be handled in a follow-up patch, I just wasn't certain if this attribute was specific to functions or anything that executes code.)
Good point! At the moment this attribute is only documented for functions, so I'll have to investigate the expected behaviour for lambdas, blocks, and ObjC methods and provide a follow-up patch. 


================
Comment at: clang/lib/CodeGen/CGCall.cpp:4422-4427
+    if (FD->hasAttr<CFGuardAttr>() &&
+        FD->getAttr<CFGuardAttr>()->getGuard() == CFGuardAttr::GuardArg::nocf) {
+      if (!CI->getCalledFunction()) {
+        Attrs = Attrs.addAttribute(
+            getLLVMContext(), llvm::AttributeList::FunctionIndex, "guard_nocf");
+      }
----------------
aaron.ballman wrote:
> ```
> if (const auto *A = FD->getAttr<CFGuardAttr>()) {
>   if (A->getGuard() == CFGuardAttr::GuardArg::nocf && !CI->getCalledFunction())
>     Attrs = ...
> }
> ```
> (This way you don't have to call `hasAttr<>` followed by `getAttr<>`.)
That's much nicer - thanks!


================
Comment at: clang/test/Sema/attr-guard_nocf.cpp:1
+// RUN: %clang_cc1 -triple %ms_abi_triple -fms-extensions -verify -std=c++11 -fsyntax-only %s
+
----------------
aaron.ballman wrote:
> Since this test file is identical to the C test, I would remove the .cpp file and add its RUN line to the .c test instead. You can do this with:
> ```
> // RUN: %clang_cc1 -triple %ms_abi_triple -fms-extensions -verify -std=c++11 -fsyntax-only -x c++ %s
> ```
That saves a lot of repetition - thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72167/new/

https://reviews.llvm.org/D72167