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

erik.pilkington added a comment. In D58797#1447148 , @phosek wrote: > This is triggering a Clang assertion failure in Fuchsia build: > > clang/lib/AST/ExprConstant.cpp:5032: bool (anonymous > namespace)::ExprEvaluatorBase<(anonymous > namespace)::Point

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

erik.pilkington added a comment. Ah, looks like the problem is we're sending a dependent expression to the constant evaluator, we should just bail out in that case. I'll fix this tomorrow morning, thanks for tracking this down! Repository: rC Clang CHANGES SINCE LAST ACTION https://review

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

phosek added a comment. This is triggering a Clang assertion failure in Fuchsia build: clang/lib/AST/ExprConstant.cpp:5032: bool (anonymous namespace)::ExprEvaluatorBase<(anonymous namespace)::PointerExprEvaluator>::VisitMemberExpr(const clang::MemberExpr *) [Derived = (anonymous namespace):

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

nickdesaulniers added a comment. In D58797#1443856 , @erik.pilkington wrote: > I added it in r357041. LGTM, thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58797/new/ https://reviews.llvm.org/D58797 __

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

erik.pilkington added a comment. In D58797#1443411 , @efriedma wrote: > For that particular case, I think we could suppress the false positive using > DiagRuntimeBehavior. How many total false positives are we talking about, > and how many can the compi

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

george.burgess.iv added a comment. (Forgot to refresh before pressing 'Submit', so maybe efriedma's comment answers all of the questions in mine ;) ) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58797/new/ https://reviews.llvm.org/D58797

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

george.burgess.iv added a comment. We have warnings like -Wdivision-by-zero that take reachability into account: https://godbolt.org/z/3PD-eF. I don't immediately know how it's all done (e.g. in batch because CFG construction is expensive? ...), but looking there for inspiration may be useful.

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

efriedma added a comment. For that particular case, I think we could suppress the false positive using DiagRuntimeBehavior. How many total false positives are we talking about, and how many can the compiler statically prove are unreachable? Repository: rC Clang CHANGES SINCE LAST ACTION

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

aaron.ballman added a comment. In D58797#1439888 , @erik.pilkington wrote: > Ah, I didn't consider that case. Presumably `st` is configured to have > different sizes based on the target? I agree that this is a false-positive, > but it seems like a prett

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

nickdesaulniers added a comment. In D58797#1439888 , @erik.pilkington wrote: > Ah, I didn't consider that case. Presumably `st` is configured to have > different sizes based on the target? Yes; sorry I was not clear about that in my example. > I agree

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

erik.pilkington marked 4 inline comments as done. erik.pilkington added a comment. In D58797#1438975 , @nickdesaulniers wrote: > This is causing false positive warnings for the Linux kernel: > https://github.com/ClangBuiltLinux/linux/issues/423 > :( > >

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

nickdesaulniers added a comment. This is causing false positive warnings for the Linux kernel: https://github.com/ClangBuiltLinux/linux/issues/423 :( https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/statfs.c#n128 Consider this untested case: if (sizeof(buf) == s

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

This revision was automatically updated to reflect the committed changes. Closed by commit rC356397: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics (authored by epilk, committed by ). Changed prior to commit: https://reviews.llvm.org/D58797?vs=190549&id=191142#toc Repository: rC Cl

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

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from some minor nits. Comment at: clang/lib/Sema/SemaChecking.cpp:338 + case Builtin::BI__builtin___vsnprintf_chk: { +DiagID = diag::warn_memcpy

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

george.burgess.iv added a comment. This LGTM; feel free to submit after Aaron stamps this. Thanks again! Comment at: clang/lib/Sema/SemaExpr.cpp:5929 +checkFortifiedBuiltinMemoryFunction(FDecl, TheCall); + erik.pilkington wrote: > george.burgess.iv wrote

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

erik.pilkington updated this revision to Diff 190549. erik.pilkington marked 16 inline comments as done. erik.pilkington added a comment. Address review comments. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58797/new/ https://reviews.llvm.org/D58797 Files: clang/include/cla

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

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.

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

aaron.ballman added inline comments. Comment at: clang/lib/AST/Decl.cpp:2994 +/// +/// \param ConsiderWrapperFunctions If we should consider wrapper functions as +/// their wrapped builtins. This shouldn't be done in general, but its useful in If we should -> If

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

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

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

george.burgess.iv added a comment. Thanks for working on this! I hope to take an in-depth look at this patch next week (if someone else doesn't beat me to it...), but wanted to note that I think enabling clang to emit these warnings on its own is a good thing. `diagnose_if` is great for potent

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

erik.pilkington created this revision. erik.pilkington added reviewers: george.burgess.iv, rsmith, aaron.ballman. Herald added subscribers: jdoerfert, dexonsmith, jkorous. Herald added a project: clang. These diagnose overflowing calls to subset of fortifiable functions. Some functions, like `spr