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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:2914
+  let Spellings = [Declspec<"guard">];
+  let Subjects = SubjectList<[Function]>;
+  let Args = [EnumArgument<"Guard", "GuardArg", ["nocf"], ["nocf"]>];
----------------
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.)


================
Comment at: clang/lib/CodeGen/CGCall.cpp:4421
+  // Control Flow Guard checks should not be added, even if the call is inlined.
+  if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(CurFuncDecl)) {
+    if (FD->hasAttr<CFGuardAttr>() &&
----------------
`const auto *` because the type is spelled out in the initialization anyway.


================
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");
+      }
----------------
```
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<>`.)


================
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
+
----------------
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
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72167