This is an archive of the discontinued LLVM Phabricator instance.

[clang][JumpDiagnostics] ignore non-asm goto target scopes
ClosedPublic

Authored by nickdesaulniers on Jul 14 2023, 3:16 PM.

Details

Summary

The current behavior of JumpScopeChecker::VerifyIndirectOrAsmJumps was
to cross validate the scope of every jumping statement (goto, asm goto)
against the scope of every label (even if the label was not even a
possible target of the asm goto).

When we have multiple asm goto's with unique targets, we could trigger
false positive build errors complaining that labels that weren't even in
the asm goto's label list could not be jumped to. Example:

error: cannot jump from this asm goto statement to one of its possible targets
asm goto(""::::foo);
note: possible target of asm goto statement
bar:
^

Fixes: https://github.com/ClangBuiltLinux/linux/issues/1886

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2023, 3:16 PM
nickdesaulniers requested review of this revision.Jul 14 2023, 3:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2023, 3:16 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nickdesaulniers edited the summary of this revision. (Show Details)
  • add link to CBL
Harbormaster completed remote builds in B245500: Diff 540574.
nickdesaulniers edited the summary of this revision. (Show Details)
  • reword description
nickdesaulniers edited the summary of this revision. (Show Details)
  • reword commit msg one more time
void accepted this revision.Jul 14 2023, 4:04 PM
This revision is now accepted and ready to land.Jul 14 2023, 4:04 PM
jyu2 accepted this revision.Jul 14 2023, 4:06 PM

LGTM.

jyu2 added inline comments.Jul 14 2023, 4:09 PM
clang/lib/Sema/JumpDiagnostics.cpp
794

Just wonder why not just use TargetLabel instead?

Thanks a lot for working on this.
This probably needs a release note entry

clang/lib/Sema/JumpDiagnostics.cpp
790

We don't need to make 3 copies!

791–800

I don't think the pointer adds that much, you can init capture by copy which simplifies a bit

clang/test/Sema/asm-goto.cpp
71

can you add more tests? Maybe something like that

cpp
void f() {
  try{
    __label__ label;
    asm goto("" : : : : label);
    label:;
  }
  catch(...){
    __label__ label;
    asm goto("" : : : : label);
    label:;
  };
  if constexpr(false) {
    __label__ label;
    asm goto("" : : : : label);
    label:;
  }
}
cor3ntin added inline comments.Jul 15 2023, 5:55 AM
clang/lib/Sema/JumpDiagnostics.cpp
791–800

I don't think the pointer adds that much, you can init capture by copy which simplifies a bit

s/pointer/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 are we building a map of all the labels and then filtering out the ones that aren't listed? We should just be checking the listed destinations with the stricter triviality rule that indirect-goto uses.

nickdesaulniers planned changes to this revision.Jul 17 2023, 9:58 AM

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 are we building a map of all the labels and then filtering out the ones that aren't listed? We should just be checking the listed destinations with the stricter triviality rule that indirect-goto uses.

Great point. Looks like I should probably be calling CheckJump from JumpScopeChecker::VerifyJumps instead. Let me work on that refactoring instead.

nickdesaulniers marked 5 inline comments as done.
  • test just asm goto targets, as per @rjmccall (this lead to many cleanups)
  • add release note and test case as per @cor3ntin
  • remove changes to JumpScopeChecker::BuildScopeInformation
This revision is now accepted and ready to land.Jul 17 2023, 10:53 AM
clang/test/Sema/asm-goto.cpp
71

Done, but I have to put it somewhere else because:

  1. exceptions are disabled in clang/test/Sema/*. This needs to go in clang/test/SemaCXX/
  2. if constexpr is a C++17 addition.
nickdesaulniers planned changes to this revision.Jul 17 2023, 11:53 AM
nickdesaulniers added inline comments.
clang/lib/Sema/JumpDiagnostics.cpp
75

AsmJumps can now be removed, too.

  • remove AsmJumps
This revision is now accepted and ready to land.Jul 17 2023, 11:54 AM
nickdesaulniers planned changes to this revision.Jul 17 2023, 11:56 AM
nickdesaulniers added inline comments.
clang/lib/Sema/JumpDiagnostics.cpp
730

should remove this reference to asm goto as well.

  • remove stale asm goto comment
This revision is now accepted and ready to land.Jul 17 2023, 11:56 AM

Thanks, that's way better. Just a couple minor requests.

clang/docs/ReleaseNotes.rst
629 ↗(On Diff #541169)
clang/lib/Sema/JumpDiagnostics.cpp
363

You can pull the GCCAsmStmtClass case right above this, make the cast unconditional (if (!cast<GCCAsmStmt>(S)->isAsmGoto()) break;), and then fall through into the GotoStmt case.

  • fix typo in release doc
clang/lib/Sema/JumpDiagnostics.cpp
363

I could hoist + use [[fallthrough]] but note that the case above Stmt::SwitchStmtClass also currently uses [[fallthrough]] here as well; so I would still need the dyn_cast.

With that in mind, do you still prefer the hoisting? I don't have a preference, but wanted to triple check that with you.

nickdesaulniers marked an inline comment as done.Jul 17 2023, 12:08 PM
rjmccall added inline comments.Jul 17 2023, 2:14 PM
clang/lib/Sema/JumpDiagnostics.cpp
363

Ah, right. Personally, I would probably put this common path in a helper function, but you could also just duplicate it since it's so small. This also seems like an acceptable use-case for goto if you can stomach that — with a good label, it should still be very readable.

clang/lib/Sema/JumpDiagnostics.cpp
363

Is https://reviews.llvm.org/D155522 what you had in mind? If so, then I'll squash it into this, otherwise can you state differently what you had in mind?

clang/lib/Sema/JumpDiagnostics.cpp
363

oh, rereading what you wrote, I think I understand what you mean with gotos. Let me post a diff of that, for clarification.

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.

nickdesaulniers added inline comments.
clang/lib/Sema/JumpDiagnostics.cpp
363
nickdesaulniers marked 2 inline comments as done.Jul 18 2023, 10:16 AM

Thanks for the reviews and advice. Any parting thoughts @rjmccall ?

rjmccall added inline comments.Jul 18 2023, 10:51 AM
clang/lib/Sema/JumpDiagnostics.cpp
669

I think it would be good to leave a comment here like this:

We know the possible destinations of an asm goto, but we don't have the
ability to add code along those control flow edges, so we have to diagnose
jumps both in and out of scopes, just like we do with an indirect goto.

nickdesaulniers marked an inline comment as done.
clang/lib/Sema/JumpDiagnostics.cpp
669

Depending on your definition of "we" (clang vs. llvm), llvm does have the ability to add code along those edges.

See llvm/lib/CodeGen/CallBrPrepare.cpp. I'll clarify in your comment that "clang does not split critical edges during code generation of llvm ... "

rjmccall added inline comments.Jul 18 2023, 11:12 AM
clang/lib/Sema/JumpDiagnostics.cpp
669

Okay, so, I don't really know much about this feature. I was thinking that the branch might go directly into some other assembly block, which would not be splittable. If the branch just goes to an arbitrary basic block in IR, then it would be straightforward for IRGen to just resolve the destination blocks for asm goto labels to some new block that does a normal goto to that label. If we did that, we wouldn't need extra restrictions here at all and could just check this like any other direct branch.

We don't need to do that work right away, but the comment should probably reflect the full state of affairs — "but clang's IR generation does not currently know how to add code along these control flow edges, so we have to diagnose jumps both in and out of scopes, like we do with indirect goto. If we ever add that ability to IRGen, this code could check these jumps just like ordinary gotos."

clang/lib/Sema/JumpDiagnostics.cpp
669

Okay, so, I don't really know much about this feature.

"Run this block of asm, then continue to either the next statement or one of the explicit labels in the label list."


That comment still doesn't seem quite right to me.

asm goto is more like a direct goto or a switch in the sense that the cases or possible destination are known at compile time; that's not like indirect goto where you're jumping to literally anywhere.

We need to check the scopes like we would for direct goto, because we don't want to bypass non-trivial destructors.


Interestingly, it looks like some of the cases inclang/test/Sema/asm-goto.cpp, g++ permits, if you use the -fpermissive flag. Clang doesn't error that it doesn't recognize that flag, but it doesn't seem to do anything in clang, FWICT playing with it in godbolt.


That said, I would have thought

void test4cleanup(int*);
// No errors expected.
void test4(void) {
l0:;
    int x __attribute__((cleanup(test4cleanup)));
    asm goto("# %l0"::::l0);
}

To work with this change, but we still produce:

x.c:6:5: error: cannot jump from this asm goto statement to one of its possible targets
    6 |     asm goto("# %l0"::::l0);
      |     ^
x.c:4:1: note: possible target of asm goto statement
    4 | l0:;
      | ^
x.c:5:9: note: jump exits scope of variable with __attribute__((cleanup))
    5 |     int x __attribute__((cleanup(test4cleanup)));
      |         ^

Aren't those in the same scope though? I would have expected that to work just as if we had a direct goto l0 rather than the asm goto.

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 direct branch to the target block if there are no destructors along the path.

Indirect goto is not allowed to jump out of scopes because, in general, the labels could be in different scopes with different sets of destructors required, and so the only way we could run the right set of destructors along the way would be to dynamically compare the label value with specific labels, which would defeat the point of the feature. (This is unnecessary if all the labels are in the same scope, which is typical, but nobody has felt like extending the feature to recognize that pattern and be more general.)

efriedma added inline comments.Jul 18 2023, 12:09 PM
clang/lib/Sema/JumpDiagnostics.cpp
669

(There's some history here that the original implementation of asm goto treated it semantically more like an indirect goto, including the use of address-of-labels; for a variety of reasons, we changed it so it's more like a switch statement.)

Suppose we have:

void test4cleanup(int*);
void test4(void) {
    asm goto("# %l0"::::l0);
l0:;
    int x __attribute__((cleanup(test4cleanup)));
    asm goto("# %l0"::::l0);
}

To make this work correctly, the first goto needs to branch directly to the destination, but the second needs to branch to a call to test4cleanup(). It's probably not that hard to implement: instead of branching directly to the destination bb, each edge out of the callbr needs to branch to a newly created block, and that block needs to EmitBranchThroughCleanup() to the final destination. (We create such blocks anyway to handle output values, but the newly created blocks branch directly to the destination BasicBlock instead of using EmitBranchThroughCleanup().)

But until we implement that, we need the error message so we don't miscompile.

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 direct branch to the target block if there are no destructors along the path.

Indirect goto is not allowed to jump out of scopes because, in general, the labels could be in different scopes with different sets of destructors required

Ah, got it.

clang/lib/Sema/JumpDiagnostics.cpp
669

but the second needs to branch to a call to test4cleanup().

GCC does not behave that way (i.e. if the branch is taken from the asm goto to l0, test4cleanup is not run). In fact, if I remove the call to DiagnoseIndirectOrAsmJump below, we generate the same control flow that GCC 12 does. https://godbolt.org/z/Y6en3YsY1

Perhaps one could argue "that's surprising" or "not correct" but if we were to have such a difference then that would probably preclude the use of the unholy combination of asm goto and __attribute__((cleanup())) (famous last words).

Let me try again with the comment based on feedback thus far.

efriedma added inline comments.Jul 18 2023, 1:43 PM
clang/lib/Sema/JumpDiagnostics.cpp
669

I guess that's an argument for keeping around this diagnostic, at least for now.

Can you file a bug against gcc?

jyknight added inline comments.Jul 18 2023, 1:49 PM
clang/lib/Sema/JumpDiagnostics.cpp
669

That looks like a bug in GCC to me. Can you open a bug report against GCC with your test-case, and refer to the bug here?

I think we should not modify Clang to implement any non-error behavior until the GCC bug is addressed.

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 a destructor is well-formed, but it's still arguably* a miscompile and must be diagnosed.

(*) It's a miscompile unless there's a rule saying it's U.B. to jump out of a scope. I don't see such a rule in any of the docs about this feature.

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 longer present when that change is re-applied on top of this one.

shafik added a subscriber: shafik.Jul 20 2023, 3:26 PM
shafik added inline comments.
clang/lib/Sema/JumpDiagnostics.cpp
680

Do we have a test that checks this diagnostic?

nickdesaulniers marked an inline comment as done.Jul 20 2023, 8:04 PM
nickdesaulniers added inline comments.
clang/lib/Sema/JumpDiagnostics.cpp
680

Yes, all of the existing tests in clang/test/Sema/asm-goto.cpp are a result of this statement.

nickdesaulniers marked an inline comment as done.Jul 20 2023, 8:04 PM
This revision was landed with ongoing or failed builds.Jul 20 2023, 8:05 PM
This revision was automatically updated to reflect the committed changes.