[PATCH] D104887: [clang] Evaluate strlen of strcpy argument for -Wfortify-source.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

thanks for this! mostly just nits from me



================
Comment at: clang/lib/AST/ExprConstant.cpp:15755
+
+bool Expr::tryEvaluateStrLen(uint64_t &Result, ASTContext &Ctx) const {
+  Expr::EvalStatus Status;
----------------
Looks like this is the second "try to evaluate the call to this builtin function" API endpoint we have here (the other being for `__builtin_object_size`). IMO this isn't an issue, but if we need many more of these, it might be worth considering exposing a more general `Expr::tryEvaluateBuiltinFunctionCall(APValue &, ASTContext &, BuiltinID, ArrayRef<Expr>)` or similar.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:604
 
+  auto ComputeCheckVariantSize = [&](unsigned Index) -> Optional<llvm::APSInt> {
+    Expr::EvalResult Result;
----------------
nit: i'd rename this `ComputeExplicitObjectSizeArgument`


================
Comment at: clang/lib/Sema/SemaChecking.cpp:621
+
+    Expr *ObjArg = TheCall->getArg(Index);
+    uint64_t Result;
----------------
nit: would `const Expr *` work here? clang prefers to have `const` where possible


================
Comment at: clang/lib/Sema/SemaChecking.cpp:739
     DiagID = diag::warn_fortify_source_size_mismatch;
-    SizeIndex = TheCall->getNumArgs() - 1;
-    ObjectIndex = 0;
+    SourceSize = ComputeCheckVariantSize(TheCall->getNumArgs() - 1);
+    DestinationSize = ComputeSizeArgument(0);
----------------
i expected `ComputeCheckVariantSize` to imply that the argument was to a `_chk` function, but these `case`s don't reference `_chk` functions (nor do we set `IsChkVariant = true;`). should this be calling `ComputeSizeArgument` instead?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:753
     DiagID = diag::warn_fortify_source_overflow;
-    SizeIndex = TheCall->getNumArgs() - 1;
-    ObjectIndex = 0;
+    SourceSize = ComputeCheckVariantSize(TheCall->getNumArgs() - 1);
+    DestinationSize = ComputeSizeArgument(0);
----------------
same "shouldn't this be `ComputeSizeArgument`?' question


================
Comment at: clang/lib/Sema/SemaChecking.cpp:762
     DiagID = diag::warn_fortify_source_size_mismatch;
-    SizeIndex = 1;
-    ObjectIndex = 0;
+    SourceSize = ComputeCheckVariantSize(1);
+    DestinationSize = ComputeSizeArgument(0);
----------------
same question


================
Comment at: clang/test/Sema/warn-fortify-source.c:64
+  char dst[4];
+  __builtin_strcpy(dst, src); // expected-warning {{'strcpy' will always overflow; destination buffer has size 4, but the source string has length 7 (including null byte)}}
+}
----------------
for completeness and consistency, please include a case where this warning doesn't fire.

at the same time, it'd be nice to test for an off-by-one (which i believe is handled correctly by this patch already); maybe shorten `src` to `"abcd"` and have a test on `char dst2[5];` that doesn't fire?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104887