[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

rjmccall added a comment. There's no reliable way to report this with UBSan because in general we can't ever know that a loop will not terminate. That said, we could report *obviously* trivial loops, either with UBSan or just with a diagnostic. If the body of your loop really is empty, and

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

xbolva00 added a comment. In D96418#2783541 , @leonardchan wrote: >> Well, no, I'm afraid it is actually clear that that code does have UB >> according to the C++ standard. Perhaps you mean that it *shouldn't* have >> UB, or that Clang should define

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

jdoerfert added a comment. In D96418#2783541 , @leonardchan wrote: >> Well, no, I'm afraid it is actually clear that that code does have UB >> according to the C++ standard. Perhaps you mean that it *shouldn't* have >> UB, or that Clang should define

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

leonardchan added a comment. > Well, no, I'm afraid it is actually clear that that code does have UB > according to the C++ standard. Perhaps you mean that it *shouldn't* have UB, > or that Clang should define its behavior despite the standard. > > I might agree with you that I don't see the

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

rjmccall added a comment. In D96418#2780687 , @leonardchan wrote: > Hi all, it looks like this commit led to some unexpected behavior in our > build. When compiling something like: > > // clang++ -c -S -o - /tmp/test.cc -std=c++17 -O1 > static int

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

xbolva00 added a comment. >> Would it be fine to revert this for now to work out the kinks? I dont think. This is a known problem, not caused by this patch, just exposed. You can search bugzilla for it, simply, if there is an UB, llvm should emit a “ret”. Repository: rG LLVM Github

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

leonardchan added a comment. Hi all, it looks like this commit led to some unexpected behavior in our build. When compiling something like: // clang++ -c -S -o - /tmp/test.cc -std=c++17 -O1 static int do_nothing(void* unused) { for (;;) { } return 0; } typedef int

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG6c3129549374: [clang] Refactor mustprogress handling, add it to all loops in c++11+. (authored by fhahn). Repository: rG LLVM Github Monorepo

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

fhahn marked an inline comment as done. fhahn added inline comments. Comment at: clang/lib/CodeGen/CGStmt.cpp:796 + bool EmitBoolCondBranch = !C || !C->isOne(); + bool CondIsConst = C; const SourceRange = S.getSourceRange(); rjmccall wrote: > fhahn wrote:

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

fhahn updated this revision to Diff 341863. fhahn added a comment. Thanks everyone! I rebased the patch and addressed John's comments. I'll commit it shortly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96418/new/

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

rjmccall added a comment. Seems conceptually okay, given that we have an option to control it. Comment at: clang/lib/CodeGen/CGStmt.cpp:796 + bool EmitBoolCondBranch = !C || !C->isOne(); + bool CondIsConst = C; const SourceRange = S.getSourceRange();

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

aaron.ballman accepted this revision. aaron.ballman added a comment. I only found one tiny nit, and otherwise it LGTM, but this is a bit out of my wheelhouse. Comment at: clang/lib/CodeGen/CodeGenFunction.h:529 +// following (6.9.2.3.1 in C++11): +// - terminate, +

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

fhahn added a comment. In D96418#2676338 , @lebedev.ri wrote: > Seems fine to me. > Might be good for someone else (@aaron.ballman ?) to also take a look, not > sure. ping. @aaron.ballman any chance you could take another quick look? Repository: rG

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

lebedev.ri accepted this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. Seems fine to me. Might be good for someone else (@aaron.ballman ?) to also take a look, not sure. Thank you. Comment at: clang/lib/CodeGen/CGStmt.cpp:801 +

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

fhahn added inline comments. Comment at: clang/lib/CodeGen/CGStmt.cpp:801 + SourceLocToDebugLoc(R.getEnd()), + loopMustProgress(CondIsConst)); lebedev.ri wrote: > fhahn wrote: > > lebedev.ri wrote: > > > Now this doesn't make

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

fhahn updated this revision to Diff 336058. fhahn added a comment. Change names of loopMustProgress to checkIfLoopMustProgress. Same for functionMustProgress. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96418/new/

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

lebedev.ri added a comment. Thank you! Some more thoughts. Comment at: clang/lib/CodeGen/CGStmt.cpp:801 + SourceLocToDebugLoc(R.getEnd()), + loopMustProgress(CondIsConst)); fhahn wrote: > lebedev.ri wrote: > > Now this doesn't

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

fhahn updated this revision to Diff 335786. fhahn added a comment. rebased after pre-committing additional RUN lines in 7ca4dd82175c . Also adjusts a variable name Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

fhahn added inline comments. Comment at: clang/lib/CodeGen/CGStmt.cpp:796 + bool EmitBoolCondBranch = !C || !C->isOne(); + bool CondIsConst = C; const SourceRange = S.getSourceRange(); lebedev.ri wrote: > I think, if we really want to give this a name,

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

lebedev.ri requested changes to this revision. lebedev.ri added a comment. This revision now requires changes to proceed. @fhahn Are you still at this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96418/new/ https://reviews.llvm.org/D96418

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

lebedev.ri added a comment. Some thoughts Comment at: clang/lib/CodeGen/CGStmt.cpp:796 + bool EmitBoolCondBranch = !C || !C->isOne(); + bool CondIsConst = C; const SourceRange = S.getSourceRange(); I think, if we really want to give this a name, perhaps

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

fhahn added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96418/new/ https://reviews.llvm.org/D96418 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

fhahn added a comment. ping :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96418/new/ https://reviews.llvm.org/D96418 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

fhahn updated this revision to Diff 323568. fhahn added a comment. Rebased on top of 51bf4c0e6d4c , which added -ffinite-loops. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

fhahn updated this revision to Diff 323003. fhahn added a comment. Add reference to standards. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96418/new/ https://reviews.llvm.org/D96418 Files: clang/lib/CodeGen/CGStmt.cpp

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

aaron.ballman added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.h:523 return getLangOpts().C11 || getLangOpts().C17 || getLangOpts().C2x; } fhahn wrote: > aaron.ballman wrote: > > jdoerfert wrote: > > > fhahn wrote: > > > > jdoerfert

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

fhahn added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.h:523 return getLangOpts().C11 || getLangOpts().C17 || getLangOpts().C2x; } aaron.ballman wrote: > jdoerfert wrote: > > fhahn wrote: > > > jdoerfert wrote: > > > > Can you

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

aaron.ballman added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.h:523 return getLangOpts().C11 || getLangOpts().C17 || getLangOpts().C2x; } jdoerfert wrote: > fhahn wrote: > > jdoerfert wrote: > > > Can you modify the documentation

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

jdoerfert added a comment. Seems reasonable to me and makes things simpler. @atmnpatel Any thoughts? Comment at: clang/lib/CodeGen/CodeGenFunction.h:523 return getLangOpts().C11 || getLangOpts().C17 || getLangOpts().C2x; } fhahn wrote: > jdoerfert

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

fhahn added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.h:523 return getLangOpts().C11 || getLangOpts().C17 || getLangOpts().C2x; } jdoerfert wrote: > Can you modify the documentation to talk about what loops must make progress, >

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

fhahn updated this revision to Diff 322791. fhahn added a comment. Herald added a subscriber: jfb. Document reasoning. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96418/new/ https://reviews.llvm.org/D96418 Files: clang/lib/CodeGen/CGStmt.cpp

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.h:523 return getLangOpts().C11 || getLangOpts().C17 || getLangOpts().C2x; } Can you modify the documentation to talk about what loops must make progress, this is the code

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

fhahn created this revision. fhahn added reviewers: jdoerfert, rjmccall, xbolva00, atmnpatel, aaron.ballman, rsmith. fhahn requested review of this revision. Herald added a project: clang. Currently Clang does not add mustprogress to inifinite loops with a known constant condition, matching C11