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

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.


================
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.
----------------
aaron.ballman wrote:
> it's -> its
The opposite mistake as above, lol.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:321
+    // assume type 0.
+    int BOSType = 0;
+    if (auto *POS = FD->getParamDecl(ObjIdx)->getAttr<PassObjectSizeAttr>())
----------------
george.burgess.iv wrote:
> Should this also try to consider `fortify_stdlib`?
I reverted `fortify_stdlib` in r356103 (although you couldn't possible know this, since that was after you made this comment ;)). We're going to use your `pass_object_size` attribute instead.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:367
+    DiagID = diag::warn_memcpy_chk_overflow;
+    if (!evaluateObjectSize(TheCall->getNumArgs()-1) ||
+        !evaluateSizeArg(TheCall->getNumArgs()-2))
----------------
aaron.ballman wrote:
> 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?
Sure, I guess it's a bit cleaner to do that.


================
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
----------------
aaron.ballman wrote:
> Why don't we want to do this for the new fortify diagnostics?
No reason, just was trying to avoid changing the output of the `_chk` diagnostic. We might as as well though, it cleans up the diagnostic.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:5929
 
+    checkFortifiedBuiltinMemoryFunction(FDecl, TheCall);
+
----------------
george.burgess.iv wrote:
> Why isn't this in CheckBuiltinFunctionCall?
For the same reason I added the `bool` parameter to `getBuiltinCallee`, we don't usually consider calls to `__attribute__((overloadable))` be builtins, so we never reach `CheckBuiltinFunctionCall` for them. We're planning on using that attribute though:

```
int sprintf(__attribute__((pass_object_size(_FORTIFY_LEVEL))) char *buffer, const char *format, ...) 
          __attribute__((overloadable)) __asm__("___sprintf_pass_object_size_chk");
```


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

https://reviews.llvm.org/D58797