Skip to content

[LangRef] Try to clarify mustprogress wording. #90510

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

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

efriedma-quic
Copy link
Collaborator

Ensure it's clear that:

  • Infinite loops in non-mustprogress functions are well-defined, even if they're called by mustprogress functions.
  • Infinite recursion in mustprogress functions is not well-defined.

Looking at D86233, it's clear this was the intent, but the "transitive" wording is ambiguous. Instead, just explicitly state that infinite loops written in non-mustprogress functions count as progress.

Try to ensure it's clear that:

- Infinite loops in non-mustprogress functions are well-defined, even
  if they're called by mustprogress functions.
- Infinite recursion in mustprogress functions is not well-defined.

Looking at D86233, it's clear this was the intent, but the "transitive"
wording is ambiguous. Instead, just explicitly state that infinite loops
written in non-mustprogress functions count as progress.
@llvmbot
Copy link
Member

llvmbot commented Apr 29, 2024

@llvm/pr-subscribers-llvm-ir

Author: Eli Friedman (efriedma-quic)

Changes

Ensure it's clear that:

  • Infinite loops in non-mustprogress functions are well-defined, even if they're called by mustprogress functions.
  • Infinite recursion in mustprogress functions is not well-defined.

Looking at D86233, it's clear this was the intent, but the "transitive" wording is ambiguous. Instead, just explicitly state that infinite loops written in non-mustprogress functions count as progress.


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

1 Files Affected:

  • (modified) llvm/docs/LangRef.rst (+7-5)
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index 37662f79145d67..7441d76470d288 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -2447,12 +2447,14 @@ example:
     memory access, I/O, or other synchronization.  The ``mustprogress``
     attribute is intended to model the requirements of the first section of
     [intro.progress] of the C++ Standard. As a consequence, a loop in a
-    function with the `mustprogress` attribute can be assumed to terminate if
+    function with the ``mustprogress`` attribute can be assumed to terminate if
     it does not interact with the environment in an observable way, and
-    terminating loops without side-effects can be removed. If a `mustprogress`
-    function does not satisfy this contract, the behavior is undefined.  This
-    attribute does not apply transitively to callees, but does apply to call
-    sites within the function. Note that `willreturn` implies `mustprogress`.
+    terminating loops without side-effects can be removed. If a ``mustprogress``
+    function does not satisfy this contract, the behavior is undefined. If a
+    ``mustprogress`` function calls a function not marked ``mustprogress``,
+    and that function never returns, the program is well-defined even if there
+    isn't any other observable progress.  Note that ``willreturn`` implies
+    ``mustprogress``.
 ``"warn-stack-size"="<threshold>"``
     This attribute sets a threshold to emit diagnostics once the frame size is
     known should the frame size exceed the specified value.  It takes one

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM

Agree that this is what we want -- and I've definitely seen confusion over the current wording.

@efriedma-quic efriedma-quic merged commit 600cae7 into llvm:main Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants