-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[LangRef] Clarify to exclude norecurse attribute when a function could occur in a cycle in dynamic call-graph #157087
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-llvm-ir Author: Usha Gupta (usha1830) ChangesUpdate the definition of the As per my understanding, this scenario only arises when norecurse is forced through a llvm user option There are a few examples in #157081 which shows that the function attribute inference incorrectly infers norecurse when the behavior (as per new definition) is not enforced. Full diff: https://github.com/llvm/llvm-project/pull/157087.diff 1 Files Affected:
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index e64b9343b7622..787592a85971e 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -2256,9 +2256,12 @@ For example:
behavior at runtime if the function ever does dynamically return. Annotated
functions may still raise an exception, i.a., ``nounwind`` is not implied.
``norecurse``
- This function attribute indicates that the function does not call itself
- either directly or indirectly down any possible call path. This produces
- undefined behavior at runtime if the function ever does recurse.
+ This function attribute indicates that the function does not participate in
+ recursion, either directly or through mutual recursion. At runtime it is
+ undefined behavior if any dynamic call stack contains this function more
+ than once at the same time. A function must not be marked ``norecurse`` if,
+ along any call path starting from its body, control may reach an external
+ function whose definition is not available.
.. _langref_willreturn:
|
d3396f2
to
d193013
Compare
through external functions is present.
d193013
to
6df0f1d
Compare
Just a friendly ping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, but I'd like to have a second opinion on this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
.. code-block:: llvm | ||
|
||
fn -> other_fn -> fn ; fn is not norecurse | ||
other_fn -> fn -> other_fn ; fn is not norecurse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old definition of norecurse arguably allows other_fn -> fn -> other_fn
. For some uses of norecurse, the uniqueness of the stack frame itself is sufficient. But I can see how it would cause difficulties for inference. If we can't actually infer norecurse, it's pretty useless; frontends will rarely add norecurse markings.
@nikic @efriedma-quic Thank you for taking the time to review this! |
Update the definition of the
norecurse
attribute to forbid marking a function asnorecurse
if any call path from its body may reach it (possibly through an external function without a visible definition). This makes it clear thatnorecurse
excludes both direct and mutual recursion, even when recursion could arise through callees in separate modules.As per my understanding, this scenario only arises when norecurse is forced through a llvm user option
-mllvm -force-attribute=<fname>:norecurse
.There are a few examples in #157081 which shows that the function attribute inference incorrectly infers norecurse when the behavior (as per new definition) is not enforced.