[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf023f5cdb2e6: [clang][JumpDiagnostics] ignore non-asm goto target scopes (authored by nickdesaulniers). Repository: rG LLVM Github Monorepo

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers marked an inline comment as done. nickdesaulniers added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:669 +if (JumpScope != TargetScope) + DiagnoseIndirectOrAsmJump(G, JumpScope, LD, TargetScope); + }

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

shafik added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:669 +if (JumpScope != TargetScope) + DiagnoseIndirectOrAsmJump(G, JumpScope, LD, TargetScope); + } Do we have a test that checks this diagnostic? Repository:

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nathanchance added a comment. Thanks, I can confirm that the `__attribute__((__cleanup__(...)))` errors that we observed with Peter's work-in-progress guards series are resolved and that all the errors I observed when building the kernel after https://reviews.llvm.org/D154696 was merged are no

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

rjmccall accepted this revision. rjmccall added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155342/new/ https://reviews.llvm.org/D155342 ___ cfe-commits mailing list

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers updated this revision to Diff 542081. nickdesaulniers marked 4 inline comments as done. nickdesaulniers added a comment. - add link to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110728 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

rjmccall added a comment. Yes, this diagnostic implements a core semantic rule and must be emitted. There are some situations where Clang will actually generate malformed IR (violating dominance rules) if you fail to diagnose jump violations correctly. The IR you get when there's a jump over

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

jyknight added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658 +if (auto *G = dyn_cast(Jump)) { + for (AddrLabelExpr *L : G->labels()) { efriedma wrote: > nickdesaulniers wrote: > > efriedma wrote: > > > nickdesaulniers wrote: > >

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

efriedma added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658 +if (auto *G = dyn_cast(Jump)) { + for (AddrLabelExpr *L : G->labels()) { nickdesaulniers wrote: > efriedma wrote: > > nickdesaulniers wrote: > > > rjmccall wrote: > >

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers added a comment. In D155342#4511879 , @rjmccall wrote: >> We need to check the scopes like we would for direct goto, because we don't >> want to bypass non-trivial destructors. > > I think this is the basic misunderstanding here. Direct

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers updated this revision to Diff 541705. nickdesaulniers added a comment. - rewrite comment based on feedback from @rjmccall and @efriedma Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155342/new/ https://reviews.llvm.org/D155342

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

efriedma added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658 +if (auto *G = dyn_cast(Jump)) { + for (AddrLabelExpr *L : G->labels()) { nickdesaulniers wrote: > rjmccall wrote: > > nickdesaulniers wrote: > > > rjmccall wrote: > >

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

rjmccall added a comment. > We need to check the scopes like we would for direct goto, because we don't > want to bypass non-trivial destructors. I think this is the basic misunderstanding here. Direct `goto` is allowed to jump out of scopes; it just runs destructors on the way. It is only a

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658 +if (auto *G = dyn_cast(Jump)) { + for (AddrLabelExpr *L : G->labels()) { rjmccall wrote: > nickdesaulniers wrote: > > rjmccall wrote: > > > I think it would be

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

rjmccall added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658 +if (auto *G = dyn_cast(Jump)) { + for (AddrLabelExpr *L : G->labels()) { nickdesaulniers wrote: > rjmccall wrote: > > I think it would be good to leave a comment here

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658 +if (auto *G = dyn_cast(Jump)) { + for (AddrLabelExpr *L : G->labels()) { rjmccall wrote: > I think it would be good to leave a comment here like this: > > > We

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers updated this revision to Diff 541639. nickdesaulniers marked an inline comment as done. nickdesaulniers added a comment. - add comment as per @rjmccall Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155342/new/

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

rjmccall added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658 +if (auto *G = dyn_cast(Jump)) { + for (AddrLabelExpr *L : G->labels()) { I think it would be good to leave a comment here like this: > We know the possible

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers marked 2 inline comments as done. nickdesaulniers added a comment. Thanks for the reviews and advice. Any parting thoughts @rjmccall ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155342/new/ https://reviews.llvm.org/D155342

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers updated this revision to Diff 541619. nickdesaulniers added a comment. - squash D155525 as per @rjmccall Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155342/new/ https://reviews.llvm.org/D155342

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:361 + if (!GS->isAsmGoto()) +break; // Remember both what scope a goto is in as well as the fact that we have nickdesaulniers wrote: > nickdesaulniers wrote:

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

rjmccall added a comment. I do like the look of https://reviews.llvm.org/D155522, yeah, since we have so many of these that aren't top-level in a case anyway. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155342/new/

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:361 + if (!GS->isAsmGoto()) +break; // Remember both what scope a goto is in as well as the fact that we have nickdesaulniers wrote: > rjmccall wrote: > >

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:361 + if (!GS->isAsmGoto()) +break; // Remember both what scope a goto is in as well as the fact that we have rjmccall wrote: > nickdesaulniers wrote: > >

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

rjmccall added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:361 + if (!GS->isAsmGoto()) +break; // Remember both what scope a goto is in as well as the fact that we have nickdesaulniers wrote: > rjmccall wrote: > > You can

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:361 + if (!GS->isAsmGoto()) +break; // Remember both what scope a goto is in as well as the fact that we have rjmccall wrote: > You can pull the

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers updated this revision to Diff 541178. nickdesaulniers added a comment. - fix typo in release doc Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155342/new/ https://reviews.llvm.org/D155342 Files: clang/docs/ReleaseNotes.rst

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

rjmccall added a comment. Thanks, that's way better. Just a couple minor requests. Comment at: clang/docs/ReleaseNotes.rst:629 +- Fixed false positive error diagnostic observed from mixing ``asm goto`` with + ``__attribute__((cleanup()))`` variables falsly warning that jumps

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers updated this revision to Diff 541173. nickdesaulniers added a comment. This revision is now accepted and ready to land. - remove stale asm goto comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155342/new/

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers planned changes to this revision. nickdesaulniers added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:726 // Collect a single representative of every scope containing an // indirect or asm goto. For most code bases, this substantially

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers updated this revision to Diff 541169. nickdesaulniers added a comment. This revision is now accepted and ready to land. - remove AsmJumps Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155342/new/ https://reviews.llvm.org/D155342

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers planned changes to this revision. nickdesaulniers added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:75 SmallVector IndirectJumps; SmallVector AsmJumps; SmallVector MustTailStmts; `AsmJumps` can now be removed, too.

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers added inline comments. Comment at: clang/test/Sema/asm-goto.cpp:71 +l1:; +} cor3ntin wrote: > can you add more tests? Maybe something like that > > ```cpp > void f() { > try{ > __label__ label; > asm goto("" : : : : label); >

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers updated this revision to Diff 541139. nickdesaulniers marked 5 inline comments as done. nickdesaulniers added a comment. This revision is now accepted and ready to land. - test just asm goto targets, as per @rjmccall (this lead to many cleanups) - add release note and test case as

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers planned changes to this revision. nickdesaulniers added a comment. In D155342#4504812 , @rjmccall wrote: > Wait, the whole point of this algorithm is that we have to do this whole > complicated linear check against every label whose

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

rjmccall added a comment. Wait, the whole point of this algorithm is that we have to do this whole complicated linear check against every label whose address is taken because we don't know where it's going. If we have a list of all the possible labels that the `asm goto` might jump to, why

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

cor3ntin added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:791-800 + // This unnecessary copy is because: + // warning: captured structured bindings are a C++20 extension + // [-Wc++20-extensions] + LabelDecl *TL = TargetLabel; + //

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

cor3ntin added a comment. Thanks a lot for working on this. This probably needs a release note entry Comment at: clang/lib/Sema/JumpDiagnostics.cpp:790 // reach this label scope. for (auto [JumpScope, JumpStmt] : JumpScopes) { + // This unnecessary copy is

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

jyu2 added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:794 + // [-Wc++20-extensions] + LabelDecl *TL = TargetLabel; + // Is TargetLabel one of the targets of the JumpStmt? If not, then skip Just wonder why not just use

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

jyu2 accepted this revision. jyu2 added a comment. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155342/new/ https://reviews.llvm.org/D155342 ___ cfe-commits mailing list

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers updated this revision to Diff 540586. nickdesaulniers edited the summary of this revision. nickdesaulniers added a comment. - reword commit msg one more time Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155342/new/

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers updated this revision to Diff 540585. nickdesaulniers edited the summary of this revision. nickdesaulniers added a comment. - reword description Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155342/new/

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers updated this revision to Diff 540584. nickdesaulniers added a comment. - rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155342/new/ https://reviews.llvm.org/D155342 Files: clang/lib/Sema/JumpDiagnostics.cpp

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers updated this revision to Diff 540574. nickdesaulniers edited the summary of this revision. nickdesaulniers added a comment. - add link to CBL Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155342/new/

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

nickdesaulniers created this revision. Herald added a project: All. nickdesaulniers requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Otherwise we observe false positive warnings claiming that an asm goto cannot jump to another label in a