[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/AST/Decl.cpp:2994
+///
+/// \param ConsiderWrapperFunctions If we should consider wrapper functions as
+/// their wrapped builtins. This shouldn't be done in general, but its useful in
----------------
If we should -> If true, we should


================
Comment at: clang/lib/AST/Decl.cpp:2995
+/// \param ConsiderWrapperFunctions If we should consider wrapper functions as
+/// their wrapped builtins. This shouldn't be done in general, but its useful in
+/// Sema to diagnose calls to wrappers based on their semantics.
----------------
its -> it's


================
Comment at: clang/lib/Sema/SemaChecking.cpp:319
+    // If the parameter has a pass_object_size attribute, then we should use
+    // it's (potentially) more strict checking mode. Otherwise, conservatively
+    // assume type 0.
----------------
it's -> its


================
Comment at: clang/lib/Sema/SemaChecking.cpp:367
+    DiagID = diag::warn_memcpy_chk_overflow;
+    if (!evaluateObjectSize(TheCall->getNumArgs()-1) ||
+        !evaluateSizeArg(TheCall->getNumArgs()-2))
----------------
george.burgess.iv wrote:
> nit: All of these cases (and the two lambdas above) look super similar. Might it be clearer to just set `SizeIndex` and `ObjectIndex` variables from here, and actually `evaluate` them before `UsedSize.ule(ComputedSize)`?
> 
> If not, I'm OK with this as-is.
Formatting looks off here -- run the patch through clang-format?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:388
+      return;
+    // Whether these functions overflow depend on the runtime strlen of the
+    // string, not just the buffer size, so emitting the "always overflow"
----------------
depend -> depends


================
Comment at: clang/lib/Sema/SemaChecking.cpp:423
+  StringRef FunctionName = getASTContext().BuiltinInfo.getName(BuiltinID);
+  if (DiagID == diag::warn_memcpy_chk_overflow) {
+    // __builtin___memcpy_chk -> memcpy
----------------
Why don't we want to do this for the new fortify diagnostics?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797