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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits