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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM aside from some very minor nits.



================
Comment at: clang/include/clang/Basic/AttrDocs.td:987-989
+The first argument to the attribute is the type passed to
+`__builtin_object_size`, and the second is the flag that the fortified format
+functions accept.
----------------
erik.pilkington wrote:
> aaron.ballman wrote:
> > Maybe I'm being dense, but I still have no idea what I'd pass for either of these arguments. When I hear "type", I immediately assume I should pass in something like 'int', but that seems wrong given that this is an integer argument. Is there some public documentation we could link to with a "for more information, see <blah>" snippet? Similar for the fortified format function flag.
> > 
> > However, I'm also starting to wonder why this attribute is interesting to users. Why not infer this attribute automatically, since there's only a specified set of standard library functions that it can be applied to anyway? Is it a bad idea to try to infer this for some reason?
> Yeah, I guess we could link to GCC's docs for `__builtin_object_size`, I'll update this. I think the flag argument has something to do with enabling checking `%n` output parameters, but I'm not totally sure and don't want to spread any misinformation in the clang docs. On our implementation, the value is just ignored. 
> 
> This attribute would never really be used by users that aren't C library implementers. The problem with doing this automatically is that library users need to be able to customize the checking mode by defining the `_FORTIFY_SOURCE` macro to a level in their `.c` files. We can't do this based on the presence of that macro for a couple reasons, firstly, I'm not sure we can assume that all `*_chk` variants are present just because `_FORTIFY_SOURCE` is defined (whether the `_chk` variants are present seems like a decision best left to the library). And secondly because that would mean that `clang -E t.c -o - | clang -xc -` would generate different code from `clang t.c`. Given that, it seems like an attribute is the best customization point.
Thank you for the explanation -- I agree that an attribute probably makes sense then. I'd appreciate a note in the docs saying something along the lines of "This attribute is intended for use within standard C library implementations and should not generally be used for user applications." or some such. WDYT?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:994-1012
+  - memcpy
+  - memmove
+  - memset
+  - stpcpy
+  - strcat
+  - strcpy
+  - strlcat
----------------
Rather than enumerate a long list, can you shorten it vertically by grouping the functions? e.g., strpcpy, strcpy, et. al. on the same line


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1491
+  llvm::Value *FlagVal = llvm::ConstantInt::get(IntTy, Flag);
+  auto emitObjSize = [&]() -> llvm::Value * {
+    return evaluateOrEmitBuiltinObjectSize(CE->getArg(0), BOSType, SizeTy,
----------------
Is the explicit trailing return here needed? I would have assumed this could be inferred.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1548
+  assert(Err == ASTContext::GE_None && "Should not codegen an error");
+  llvm::FunctionType *LLVMVariantTy =
+      cast<llvm::FunctionType>(ConvertType(VariantTy));
----------------
`auto *`


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1574
 
+  if (auto *Attr = FD->getAttr<FortifyStdLibAttr>())
+    return emitFortifiedStdLibCall(*this, E, BuiltinID, Attr->getType(),
----------------
`const auto *` and some other identifier than `Attr` (so it won't conflict with the type name).


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6437
+    S.Diag(AL.getArgAsExpr(0)->getBeginLoc(),
+           diag::err_attribute_argument_outof_range)
+        << AL << 0 << 3;
----------------
lol, I'll fix that typo after you land your patch.


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

https://reviews.llvm.org/D57918