[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-28 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin added a comment. It seems to have done the trick! https://lab.llvm.org/buildbot/#/builders/245/builds/11766 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155955/new/ https://reviews.llvm.org/D155955 __

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-28 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin added a comment. In D155955#4542531 , @antmo wrote: > Hi, clang-armv8-quick bot now fails on > Clang::cxx2a-constexpr-dynalloc-limits.cpp > > https://lab.llvm.org/buildbot/#/builders/245/builds/11764 > > Could you please look at this ? I land

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-28 Thread antoine moynault via Phabricator via cfe-commits
antmo added a comment. Hi, clang-armv8-quick bot now fails on Clang::cxx2a-constexpr-dynalloc-limits.cpp https://lab.llvm.org/buildbot/#/builders/245/builds/11764 Could you please look at this ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-28 Thread Corentin Jabot via Phabricator via cfe-commits
This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG45ab2b48bd55: [Clang] Improve the handling of large arrays evaluation. (authored by cor3ntin). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-28 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin updated this revision to Diff 545099. cor3ntin edited the summary of this revision. cor3ntin added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155955/new/ https://reviews.llvm.org/D155955 Files: clang/docs/ReleaseNot

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-28 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin added a subscriber: rsmith. cor3ntin added a comment. @efriedma I sent a mail to core last night, some discussion is happening. @rsmith made some good point so hopefully we will be able to produce some hard error in that case, But given this is preexisting, I'm going to land that and h

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-27 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin added a comment. In D155955#4539698 , @efriedma wrote: > That's scary: it means we can silently behave differently from other > compilers in a way that's hard to understand. Is there some way we can > provide a diagnostic? > > That said, it lo

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-27 Thread Eli Friedman via Phabricator via cfe-commits
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. That's scary: it means we can silently behave differently from other compilers in a way that's hard to understand. Is there some way we can provide a diagnostic? That said, it looks like

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-27 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin added a comment. In D155955#4537145 , @efriedma wrote: > The general approach seems fine. The multiplier for constexpr vs. constant > folding can be left for a followup, and we can continue to consider other > possible improvements elsewhere.

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-26 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added a comment. The general approach seems fine. The multiplier for constexpr vs. constant folding can be left for a followup, and we can continue to consider other possible improvements elsewhere. I guess I have one remaining question here: how does this interact with SFINAE? In o

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-26 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin added a comment. @efriedma You still have objection to landing that now with the goal to backport later in the release process? Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155955/new/ https://reviews.llvm.org/D155955 __

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-25 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In D155955#4532433 , @cor3ntin wrote: > In D155955#4532385 , @aaron.ballman > wrote: > >> In D155955#4528792 , @efriedma >> wrote: >>

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-25 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin added a comment. In D155955#4532385 , @aaron.ballman wrote: > In D155955#4528792 , @efriedma > wrote: > >>> Note sure what you mean. Making sure we use size_t for all array extents is >>> not something

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-25 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In D155955#4528792 , @efriedma wrote: >> Note sure what you mean. Making sure we use size_t for all array extents is >> not something that can be fixed overnight but more importantly, it does not >> help: Would it not over

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-24 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added a comment. > Note sure what you mean. Making sure we use size_t for all array extents is > not something that can be fixed overnight but more importantly, it does not > help: Would it not overflow, the allocation would still fail. Oh, right, it would be sizeof(APValue) * UINT_MAX

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-24 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin updated this revision to Diff 543606. cor3ntin added a comment. Update release notes and commit message Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155955/new/ https://reviews.llvm.org/D155955 Files: clang/docs/ReleaseNotes.rst clan

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-24 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin added a comment. In D155955#4528571 , @efriedma wrote: > The UINT_MAX thing seems like a straightforward bug; if we have time to fix > it properly, I'd prefer not to add weird workarounds. Note sure what you mean. Making sure we use `size_t` fo

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-24 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added a comment. The UINT_MAX thing seems like a straightforward bug; if we have time to fix it properly, I'd prefer not to add weird workarounds. The release note says "unless they are part of a constant expression", but I don't see any code in the implementation that distinguishes fo

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-24 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added subscribers: thieta, tstellar, clang-vendors. aaron.ballman added a comment. The changes generally seem reasonable to me, but I'm not 100% sure of the impacts of tying this to constexpr steps. That's a vague measure that was used mostly as an escape hatch for recursive conste

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-24 Thread Danila Malyutin via Phabricator via cfe-commits
danilaml added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:1026 + // APValue stores array extents as unsigned, + // so anything that is greater that unsigned would overflow when + // constructing the array, we catch this here. ==

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-24 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin updated this revision to Diff 543530. cor3ntin added a comment. Add more Vla tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155955/new/ https://reviews.llvm.org/D155955 Files: clang/docs/ReleaseNotes.rst clang/docs/UsersManual.rs

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-24 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin updated this revision to Diff 543504. cor3ntin added a comment. Add some VLA tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155955/new/ https://reviews.llvm.org/D155955 Files: clang/docs/ReleaseNotes.rst clang/docs/UsersManual.rs

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-24 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin updated this revision to Diff 543490. cor3ntin added a comment. Address Aaron's comments and add documentation Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155955/new/ https://reviews.llvm.org/D155955 Files: clang/docs/ReleaseNotes.rst

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-24 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:356 + "cannot allocate array; evaluated array bound %0 exceeds the limit (%1); " + "use -fconstexpr-steps=%0 to increase this limit">; def note_constexpr_new_too_small : Note<

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-24 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin marked 2 inline comments as done. cor3ntin added inline comments. Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:356 + "cannot allocate array; evaluated array bound %0 exceeds the limit (%1); " + "use -fconstexpr-steps=%0 to increase this limit">; def not

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-24 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:356 + "cannot allocate array; evaluated array bound %0 exceeds the limit (%1); " + "use -fconstexpr-steps=%0 to increase this limit">; def note_constexpr_new_too_small : Note<

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-24 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:6754-6758 bool IsNothrow = false; for (unsigned I = 1, N = E->getNumArgs(); I != N; ++I) { EvaluateIgnoredValue(Info, E->getArg(I)); IsNothrow |= E->getType()->isNothrowT(); } --

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-24 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin updated this revision to Diff 543472. cor3ntin added a comment. - Use -fconstexpr-steps to limit the size of the array. - Aply the logic to both constant evaluation and constant folding as is constant evaluation fail the limit can now be increased. Repository: rG LLVM Github Monorepo

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-21 Thread Shafik Yaghmour via Phabricator via cfe-commits
shafik added a comment. Did you evaluate trying to use `StepsLeft` at all to limit the allocation? Should the limit be setable via the command line like what we do with steps via `-fconstexpr-steps`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D155955: [Clang] Improve the handling of large arrays evaluation.

2023-07-21 Thread Corentin Jabot via Phabricator via cfe-commits
cor3ntin created this revision. Herald added a project: All. cor3ntin requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This is a temporary fix (for clang 17) that caps the size of any array we try to constant evaluate: There are 2 limits: