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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
