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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Looks solid to me overall; just a few nits.

I don't think I have actual ownership over any of this code, so I'll defer to either Aaron or Richard for the final LGTM

Thanks again!



================
Comment at: clang/lib/Sema/SemaChecking.cpp:317
+  // buffer size would be.
+  auto computeObjectSize = [&](unsigned ObjIdx) {
+    // If the parameter has a pass_object_size attribute, then we should use
----------------
nit: I think the prevailing preference is to name lambdas as you'd name variables, rather than as you'd name methods/functions


================
Comment at: clang/lib/Sema/SemaChecking.cpp:321
+    // assume type 0.
+    int BOSType = 0;
+    if (auto *POS = FD->getParamDecl(ObjIdx)->getAttr<PassObjectSizeAttr>())
----------------
Should this also try to consider `fortify_stdlib`?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:367
+    DiagID = diag::warn_memcpy_chk_overflow;
+    if (!evaluateObjectSize(TheCall->getNumArgs()-1) ||
+        !evaluateSizeArg(TheCall->getNumArgs()-2))
----------------
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.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:5929
 
+    checkFortifiedBuiltinMemoryFunction(FDecl, TheCall);
+
----------------
Why isn't this in CheckBuiltinFunctionCall?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797