[PATCH] D57918: Add an attribute that causes clang to emit fortified calls to C stdlib functions

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:986
+  }
+  }];
+}
----------------
Probably worth clarifying somewhere in here that only a specific set of stdlib functions are supported.  I don't know that it's important to spell that set out.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1490
 
+  if (auto *Attr = FD->getAttr<FortifyStdLibAttr>()) {
+    SmallVector<llvm::Value *, 8> ArgVals;
----------------
Could you move the body of this into a helper function?  This function is really far too large as it is.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1516
+      // void *memset(void *b, int c, size_t len);
+      // void *__memset_chk(void *b, int c, size_t len, size_t);
+      ArgVals.push_back(emitObjSize());
----------------
I think this might be over-commented. :)

Could you just put smaller comments on groups of cases, like:
```
  // All of these take the destination object size as the final argument.
```
or
```
  // These take flags and the destination object size immediately before the format string.
```


================
Comment at: clang/test/Sema/fortify-std-lib.c:6
+__attribute__((fortify_stdlib(0, 0)))
+int not_anyting_special(); // expected-error {{'fortify_stdlib' attribute applied to an unknown function}}
+
----------------
Silly comment, but: you typo'd "anything" here.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57918