Skip to content

Conversation

@MillePlateaux
Copy link
Contributor

@MillePlateaux MillePlateaux commented Apr 5, 2025

The error is emitted when a musttail call is made to a function marked with the not_tail_called attribute.Closes #133509

@github-actions
Copy link

github-actions bot commented Apr 5, 2025

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 5, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 5, 2025

@llvm/pr-subscribers-clang

Author: None (MillePlateaux)

Changes

Added judgment statements and accurately reported errors.Closes #133509


Full diff: https://github.com/llvm/llvm-project/pull/134465.diff

1 Files Affected:

  • (modified) clang/lib/Sema/SemaStmt.cpp (+8)
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index e1b9ccc693bd5..c8354198a5614 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -717,6 +717,14 @@ bool Sema::checkMustTailAttr(const Stmt *St, const Attr &MTA) {
     return false;
   }
 
+  if (const FunctionDecl *CalleeDecl = CE->getDirectCallee()) {
+    if (CalleeDecl->hasAttr<NotTailCalledAttr>()) {
+      Diag(St->getBeginLoc(), diag::err_musttail_conflicts_with_not_tail_called)
+          << &MTA;
+      return false;
+    }
+  }
+
   if (const auto *EWC = dyn_cast<ExprWithCleanups>(E)) {
     if (EWC->cleanupsHaveSideEffects()) {
       Diag(St->getBeginLoc(), diag::err_musttail_needs_trivial_args) << &MTA;

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First:
The title of this PR has llvm and stmt in it, neither of which should be used here, so don't bother with a tag.

Second:
The PR message (which we use as a commit message) needs a ton more detail.

Third:
Please add a release note to this.

Four:
This needs tests to show that what it is doing is correct.

Five:
I don't think this is really a good idea to make this an error, not a warning under IgnoredAttributes. It seems more reasonable to just warn here.

Six:
The diagnostic I think needs rewording. It isn't clear that it is referring to either the musttail attribute nor the not_tail_called attribute.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this! Please be sure to add a release note to clang/docs/ReleaseNotes.rst so user know about the fix.

"use '%0' on statements">,
InGroup<IgnoredAttributes>;

def err_musttail_conflicts_with_not_tail_called : Error<
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reuse err_musttail_mismatch instead?

Comment on lines 720 to 726
if (const FunctionDecl *CalleeDecl = CE->getDirectCallee()) {
if (CalleeDecl->hasAttr<NotTailCalledAttr>()) {
Diag(St->getBeginLoc(), diag::err_musttail_conflicts_with_not_tail_called)
<< &MTA;
return false;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (const FunctionDecl *CalleeDecl = CE->getDirectCallee()) {
if (CalleeDecl->hasAttr<NotTailCalledAttr>()) {
Diag(St->getBeginLoc(), diag::err_musttail_conflicts_with_not_tail_called)
<< &MTA;
return false;
}
}
if (const FunctionDecl *CalleeDecl = CE->getDirectCallee();
CalleeDecl && CalleeDecl->hasAttr<NotTailCalledAttr>()) {
Diag(St->getBeginLoc(), diag::err_musttail_conflicts_with_not_tail_called)
<< &MTA;
return false;
}

@MillePlateaux
Copy link
Contributor Author

@AaronBallman @erichkeane Thank you for your comments. There's a question here about whether to report an error or a warning here, and you're in a divided opinion. My opinion is that it is better to use the error report here. Because these are two mutually exclusive conditions, it is semantically unreasonable to set tail calls or non-tail calls even if the program is still run after a warning is reported. I still want to hear your opinions. Regarding the PR title, PR information, test cases, I will fill in the next submission. Thank you both again.

@MillePlateaux MillePlateaux changed the title [llvm][Stmt]:Fix musttail attribute on a function with not_tail_called attribute has no warning/error [Clang][Sema]:Fix musttail attribute on a function with not_tail_called attribute has no warning/error Apr 9, 2025
@AaronBallman
Copy link
Collaborator

@AaronBallman @erichkeane Thank you for your comments. There's a question here about whether to report an error or a warning here, and you're in a divided opinion. My opinion is that it is better to use the error report here. Because these are two mutually exclusive conditions, it is semantically unreasonable to set tail calls or non-tail calls even if the program is still run after a warning is reported. I still want to hear your opinions. Regarding the PR title, PR information, test cases, I will fill in the next submission. Thank you both again.

I favor an error in this case because the whole point to musttail is to ensure that a tail call happens or the code is rejected: https://clang.llvm.org/docs/AttributeReference.html#musttail

@MillePlateaux
Copy link
Contributor Author

@AaronBallman @erichkeane Thank you for your comments. There's a question here about whether to report an error or a warning here, and you're in a divided opinion. My opinion is that it is better to use the error report here. Because these are two mutually exclusive conditions, it is semantically unreasonable to set tail calls or non-tail calls even if the program is still run after a warning is reported. I still want to hear your opinions. Regarding the PR title, PR information, test cases, I will fill in the next submission. Thank you both again.

I favor an error in this case because the whole point to musttail is to ensure that a tail call happens or the code is rejected: https://clang.llvm.org/docs/AttributeReference.html#musttail

@AaronBallman Thank you for your guidance. By the way, which file should the test case be added to?

@erichkeane
Copy link
Collaborator

@AaronBallman @erichkeane Thank you for your comments. There's a question here about whether to report an error or a warning here, and you're in a divided opinion. My opinion is that it is better to use the error report here. Because these are two mutually exclusive conditions, it is semantically unreasonable to set tail calls or non-tail calls even if the program is still run after a warning is reported. I still want to hear your opinions. Regarding the PR title, PR information, test cases, I will fill in the next submission. Thank you both again.

I favor an error in this case because the whole point to musttail is to ensure that a tail call happens or the code is rejected: https://clang.llvm.org/docs/AttributeReference.html#musttail

I lean towards warning, only because the behavior is 'ignored attribute', and those are a specific warning group.

@AaronBallman
Copy link
Collaborator

@AaronBallman @erichkeane Thank you for your comments. There's a question here about whether to report an error or a warning here, and you're in a divided opinion. My opinion is that it is better to use the error report here. Because these are two mutually exclusive conditions, it is semantically unreasonable to set tail calls or non-tail calls even if the program is still run after a warning is reported. I still want to hear your opinions. Regarding the PR title, PR information, test cases, I will fill in the next submission. Thank you both again.

I favor an error in this case because the whole point to musttail is to ensure that a tail call happens or the code is rejected: https://clang.llvm.org/docs/AttributeReference.html#musttail

I lean towards warning, only because the behavior is 'ignored attribute', and those are a specific warning group.

CC @haberman as the original author of musttail in Clang, but 8344675 was the original commit for this feature and every misuse of musttail is an error there. What I recall of the design discussion is that guarantees are the entire point to using the attribute. In fact, the original commit message said "The attribute is applied to a return statement (not a function declaration), and an error is emitted if a tail call cannot be guaranteed, for example if the function signatures of caller and callee are not compatible. Guaranteed tail calls enable a class of algorithms that would otherwise use an arbitrary amount of stack space."

@mizvekov
Copy link
Contributor

mizvekov commented Apr 9, 2025

I agree that if we can't guarantee it will be tail-called, this must be an error.

must_tail is not an optimization.

In fact, LLVM by itself will already perform tail call optimizations, and remove them when it would not be profitable in terms of execution cost.

The point of must_tail is to support cases where the tail call is required for correctness, otherwise by ignoring it will lead to UB.

@MillePlateaux
Copy link
Contributor Author

Thank you for this! Please be sure to add a release note to clang/docs/ReleaseNotes.rst so user know about the fix.

@AaronBallman I have modified the code and release note and completed the test cases according to your suggestions,could you review it?

- Now correctly diagnose a tentative definition of an array with static
storage duration in pedantic mode in C. (#GH50661)

- The ``-err-musttail-mismatch`` error is emitted when a musttail call is made to a function marked with the not_tail_called attribute.(#133509).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- The ``-err-musttail-mismatch`` error is emitted when a musttail call is made to a function marked with the not_tail_called attribute.(#133509).
- An error is now emitted when a ``musttail`` call is made to a function marked with the ``not_tail_called`` attribute. (#GH133509).

"use '%0' on statements">,
InGroup<IgnoredAttributes>;


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can back out this whitespace change.

int main() {
int result = foo2(10);
return 0;
} No newline at end of file
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the newline to the end of the file?


int foo2(int a) {
[[clang::musttail]]
return foo1(a); // expected-error{{cannot perform a tail call to function'musttail' because its signature is incompatible with the calling function}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diagnostic seems odd to me because of function'musttail' Is there really no whitespace there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diagnostic seems odd to me because of function'musttail' Is there really no whitespace there?

@AaronBallman Since we reuse err_musttail_mismatch, and the function itself does not have spaces, I tried to add spaces , but the test case did not pass.I've pasted the code below.

def err_musttail_mismatch : Error<
  "cannot perform a tail call to function%select{| %1}0 "
  "because its signature is incompatible with the calling function">;

def note_tail_call_required : Note<"tail call required by %0 attribute here">;
def err_musttail_mismatch : Error<
"cannot perform a tail call to function%select{| %1}0 because its signature "
"cannot perform a tail call to function %select{| %1}0 because its signature "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original code here was correct and should not lead to any missing whitespace. You'd either get: ... to function because its signature... or you'd get to function 'foo' because. With your changes, you'd get ...to function 'foo' because (note the extra whitespace before 'foo').


if (const FunctionDecl *CalleeDecl = CE->getDirectCallee();
CalleeDecl && CalleeDecl->hasAttr<NotTailCalledAttr>()) {
Diag(St->getBeginLoc(), diag::err_musttail_mismatch) << &MTA;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be passing &MTA as that's naming the attribute. It should be passing CalleeDecl instead.

@MillePlateaux
Copy link
Contributor Author

@AaronBallman Thank you for your selfless guidance. I have modified the code now. Please review it.

@MillePlateaux
Copy link
Contributor Author

Since adding this modification, I passed the local test, and there was no response when using commandclang -cc1 -fsyntax-only -verify -Wno-error , but I don’t know why there was an error after submitting.


if (const FunctionDecl *CalleeDecl = CE->getDirectCallee();
CalleeDecl && CalleeDecl->hasAttr<NotTailCalledAttr>()) {
Diag(St->getBeginLoc(), diag::err_musttail_mismatch) << CalleeDecl;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Diag(St->getBeginLoc(), diag::err_musttail_mismatch) << CalleeDecl;
Diag(St->getBeginLoc(), diag::err_musttail_mismatch) << true << CalleeDecl;

This should resolve the crash we're seeing.

@MillePlateaux
Copy link
Contributor Author

@AaronBallman I have revised my submission, please review it. I overlooked %select{| %1}0, and I didn't catch the hint you gave me before. Thank you very much for your guidance and I hope to learn more from you in the future.

@MillePlateaux
Copy link
Contributor Author

Hi @erichkeane @AaronBallman, I've updated the PR based on the feedback. Let me know if there's anything else I should improve. Thanks for your time!

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, are you happy with it @erichkeane?


- An error is now emitted when a ``musttail`` call is made to a function marked with the ``not_tail_called`` attribute. (#GH133509).


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should only be a single -new-line at the end of this list.


if (const FunctionDecl *CalleeDecl = CE->getDirectCallee();
CalleeDecl && CalleeDecl->hasAttr<NotTailCalledAttr>()) {
Diag(St->getBeginLoc(), diag::err_musttail_mismatch) << true << CalleeDecl;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Diag(St->getBeginLoc(), diag::err_musttail_mismatch) << true << CalleeDecl;
Diag(St->getBeginLoc(), diag::err_musttail_mismatch) << /*show-function-callee=*/true << CalleeDecl;

Not exactly our policy, but makes it way easier to understand what is going on here.


int foo2(int a) {
[[clang::musttail]]
return foo1(a); // expected-error {{cannot perform a tail call to function 'foo1' because its signature is incompatible with the calling function}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooof... I know @AaronBallman suggested reusing the diagnostic... can we do at least a note (perhaps pointing to the attribute not_tail_called) to show WHY we are emitting this error?

Something like: `note: 'not_tail_called' attribute here prevents being called as a tail call'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erichkeane Where do you want this note to be added?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, a 'note' diagnostic. See a bunch of our diagnostics starting with note. Then you just do a second Diag call afterwards. I would think the source-location for it cna be the attribute on the callee.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erichkeane I still don't understand what you mean. Do I need to report cannot perform a tail call to function 'foo1' because its signature is incompatible with the calling function note: 'not_tail_called' attribute here prevents being called as a tail call' after the function is run?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In SemaStmt.cpp right after the line:
Diag(St->getBeginLoc(), diag::err_musttail_mismatch) << true << CalleeDecl;

You can do:
Diag(<get location of not_tail_called_attribute>, diag::note_<your new diag>);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea Erich! I don't think we need a new diagnostic though, we can reuse note_conflicting_attribute, so I think this will suffice:

Diag(CalleeDecl->getAttr<NotTailCalledAttr>()->getLoc(), diag::note_conflicting_attribute);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that attribute is imperfect (and requires reading tea leaves a bit), but probably good enough. I think that's a good suggestion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it'd help if the note mentioned the other attribute that was in conflict with, but I think it probably suffices. It at least gets folks' eyeballs closer to the conflict.


int foo2(int a) {
[[clang::musttail]]
return foo1(a); // expected-error {{cannot perform a tail call to function 'foo1' because its signature is incompatible with the calling function}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In SemaStmt.cpp right after the line:
Diag(St->getBeginLoc(), diag::err_musttail_mismatch) << true << CalleeDecl;

You can do:
Diag(<get location of not_tail_called_attribute>, diag::note_<your new diag>);

@MillePlateaux
Copy link
Contributor Author

@erichkeane @AaronBallman This is my first time participating in an open source project. I would like to ask if you have the same opinion so far. I have revised many versions over and over again. I just hope that the requirements can be clarified.

@AaronBallman
Copy link
Collaborator

@erichkeane @AaronBallman This is my first time participating in an open source project.

Welcome!

I would like to ask if you have the same opinion so far. I have revised many versions over and over again. I just hope that the requirements can be clarified.

FWIW, our process is to keep iterating on the review until all the active reviewers (people leaving comments) are happy with it it. That sometimes takes a number of iterations, especially as we "polish" a review up to make it production-quality. So this kind of back-and-forth is pretty typical, even if it can be a bit frustrating.

"%diff{ (expected $ but has $)|}1,2"
"|has different return type%diff{ ($ expected but has $)|}1,2}0">;
def note_musttail_disabled_by_not_tail_called : Note<
"'not_tail_called' attribute here prevents being called as a tail call">;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"'not_tail_called' attribute here prevents being called as a tail call">;
"'not_tail_called' attribute prevents being called as a tail call">;


int foo2(int a) {
[[clang::musttail]]
return foo1(a); // expected-error {{cannot perform a tail call to function 'foo1' because its signature is incompatible with the calling function}} `note: 'not_tail_called' attribute here prevents being called as a tail call'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return foo1(a); // expected-error {{cannot perform a tail call to function 'foo1' because its signature is incompatible with the calling function}} `note: 'not_tail_called' attribute here prevents being called as a tail call'
return foo1(a); // expected-error {{cannot perform a tail call to function 'foo1' because its signature is incompatible with the calling function}}

if (const FunctionDecl *CalleeDecl = CE->getDirectCallee();
CalleeDecl && CalleeDecl->hasAttr<NotTailCalledAttr>()) {
Diag(St->getBeginLoc(), diag::err_musttail_mismatch) << /*show-function-callee=*/true << CalleeDecl;
Diag(CalleeDecl->getLocation(), diag::note_musttail_disabled_by_not_tail_called) << CalleeDecl;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Diag(CalleeDecl->getLocation(), diag::note_musttail_disabled_by_not_tail_called) << CalleeDecl;
Diag(CalleeDecl->getLocation(), diag::note_musttail_disabled_by_not_tail_called);

You don't need the << CalleeDecl since that is how we fill in the %0 type things in the diagnostic kinds message. Since the added one doesn't have one,

Aaron suggested instead of this diagnostic to reuse an existing one in a previous comment which I'm OK with (though I'm also ok with this one). I'll let Aaron decide if he feels strongly enough to revert to the other one.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly about reusing the existing diagnostic, this new note is fine by me.

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Aaron is happy with the new note, looks like we're good. One of us will merge it when it passes CI.

@MillePlateaux
Copy link
Contributor Author

@AaronBallman @erichkeane It was not easy for me,thank you both! I hope to keep learning from you in the future.

@erichkeane erichkeane merged commit 061f87f into llvm:main Apr 14, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

musttail attribute on a function with not_tail_called attribute has no warning/error that musttail is ignored

5 participants