[PATCH] D86233: [LangRef] Define mustprogress attribute

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

Thanks for the clarification wrt callees, this LGTM now.



================
Comment at: llvm/docs/LangRef.rst:1963
+    Standard <http://eel.is/c++draft/intro.progress>`_. This attribute does
+    not apply transitively to callees. This is implied by `willreturn`.
 
----------------
jdoerfert wrote:
> nikic wrote:
> > jdoerfert wrote:
> > > ```
> > > This attribute indicates that the function is required to return, unwind,
> > > or interact with the environment in an observable way e.g. via a
> > > volatile 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 <http://eel.is/c++draft/intro.progress>`_. This attribute does
> > > not apply transitively to callees, that means a callee might not make progress.
> > > Note that `willreturn` implies `mustprogress`.
> > > ```
> > Some notes from my side:
> >  * I'm not sure how to interpret the "does not apply to callees" part. If mustprogress function A calls not-mustprogress function B and B goes into an infinite loop, then A will not return, unwind or interact with the environment, thus violating the mustprogress contract. Now, the callee does not necessarily have to be mustprogress in general, but I believe it needs to progress for any invocation from a mustprogress function -- or not?
> >  * I would explicitly state that violating the progress requirement results in undefined behavior.
> >  * It may be worthwhile to explicitly mention that a non-interacting loop inside a mustprogress function can thus be assumed to terminate and may be removed. That would help clarify the practical effect setting this attribute has.
> > I'm not sure how to interpret the "does not apply to callees" part. If mustprogress function A calls not-mustprogress function B and B goes into an infinite loop, then A will not return, unwind or interact with the environment, thus violating the mustprogress contract. Now, the callee does not necessarily have to be mustprogress in general, but I believe it needs to progress for any invocation from a mustprogress function -- or not?
> 
> Yes. That is the intention. If we come from the perspective that C++ has the mustprogress requirement, it has to hold for any statement, including a call to a function that may not progress. However, the statement (=call) has to make progress from the C++ side. This is the same as other attributes, let's say `readonly` (aka. pure). A function can have that attribute and call an arbitrary function. This is all good as long as the arbitrary function is readonly for this call site.
> 
> Do you have an idea how to clarify this in the wording? (Even though other attributes implicitly make this assumption as well.)
> 
> 
> > I would explicitly state that violating the progress requirement results in undefined behavior.
> 
> Sure. @atmnpatel 
> 
> > It may be worthwhile to explicitly mention that a non-interacting loop inside a mustprogress function can thus be assumed to terminate and may be removed. That would help clarify the practical effect setting this attribute has.
> 
> Sounds good to me. @atmnpatel 
Nit: "it is undefined behavior" -> "the behavior is undefined" (seems to be the phrasing used for other attributes).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86233/new/

https://reviews.llvm.org/D86233