Skip to content
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

Add clang/docs/FunctionEffectAnalysis.rst. #109855

Merged
merged 7 commits into from
Oct 28, 2024
Merged

Conversation

dougsonos
Copy link
Contributor

Follow-on from #99656, which introduces 2nd pass caller/callee analysis for function effects.

Wrote a new documentation page, derived directly from the RFC posted to LLVM Discourse earlier this year.

@llvmbot llvmbot added the clang Clang issues not falling into any other category label Sep 24, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 24, 2024

@llvm/pr-subscribers-clang

Author: Doug Wyatt (dougsonos)

Changes

Follow-on from #99656, which introduces 2nd pass caller/callee analysis for function effects.

Wrote a new documentation page, derived directly from the RFC posted to LLVM Discourse earlier this year.


Patch is 21.94 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/109855.diff

2 Files Affected:

  • (added) clang/docs/FunctionEffectAnalysis.rst (+503)
  • (modified) clang/docs/index.rst (+1)
diff --git a/clang/docs/FunctionEffectAnalysis.rst b/clang/docs/FunctionEffectAnalysis.rst
new file mode 100644
index 00000000000000..8c1cf8a483c5f1
--- /dev/null
+++ b/clang/docs/FunctionEffectAnalysis.rst
@@ -0,0 +1,503 @@
+========================
+Function Effect Analysis
+========================
+
+Introduction
+============
+
+Clang Function Effect Analysis is a C++ language extension which can warn about "unsafe"
+constructs. The feature is currently tailored for the Performance Constraint attributes,
+``nonblocking`` and ``nonallocating``; functions with these attributes are verified as not
+containing any language constructs or calls to other functions which violate the constraint.
+(See :doc:`AttributeReference`.)
+
+
+The ``nonblocking`` and ``nonallocating`` attributes
+====================================================
+
+Attribute syntax
+----------------
+
+The ``nonblocking`` and ``nonallocating`` attributes apply to function types, allowing them to be
+attached to functions, blocks, function pointers, lambdas, and member functions.
+
+.. code-block:: c++
+
+  // Functions
+  void nonblockingFunction() [[clang::nonblocking]];
+  void nonallocatingFunction() [[clang::nonallocating]];
+
+  // Function pointers
+  void (*nonblockingFunctionPtr)() [[clang::nonblocking]];
+
+  // Typedefs, type aliases.
+  typedef void (*NBFunctionPtrTypedef)() [[clang::nonblocking]];
+  using NBFunctionPtrTypeAlias_gnu = __attribute__((nonblocking)) void (*)();
+  using NBFunctionPtrTypeAlias_std = void (*)() [[clang::nonblocking]];
+
+  // C++ methods
+  struct Struct {
+    void NBMethod() [[clang::nonblocking]];
+  };
+
+  // C++ lambdas
+  auto nbLambda = []() [[clang::nonblocking]] {};
+
+  // Blocks
+  void (^nbBlock)() = ^() [[clang::nonblocking]] {};
+
+The attribute applies only to the function itself. In particular, it does not apply to any nested
+functions or declarations, such as blocks, lambdas, and local classes.
+
+This document uses the C++/C23 syntax ``[[clang::nonblocking]]``, since it parallels the placement 
+of the ``noexcept`` specifier, and the attributes have other similarities to ``noexcept``. The GNU
+``__attribute__((nonblocking))`` syntax is also supported. Note that it requires a different 
+placement on a C++ type alias.
+
+Like ``noexcept``, ``nonblocking`` and ``nonallocating`` have an optional argument, a compile-time
+constant boolean expression. By default, the argument is true, so ``[[clang::nonblocking(true)]]``
+is equivalent to ``[[clang::nonblocking]]``, and declares the function type as never locking.
+
+
+Attribute semantics
+-------------------
+
+Together with ``noexcept``, the ``nonallocating`` and ``nonblocking`` attributes define an ordered
+series of performance constraints. From weakest to strongest:
+
+- ``noexcept`` (as per the C++ standard): The function type will never throw an exception.
+- ``nonallocating``: The function type will never allocate memory on the heap, and never throw an
+  exception.
+- ``nonblocking``: The function type will never block on a lock, never allocate memory on the heap,
+  and never throw an exception.
+
+``nonblocking`` includes the ``nonallocating`` guarantee. 
+
+``nonblocking`` and ``nonallocating`` include the ``noexcept`` guarantee, but the presence of either
+attribute does not implicitly specify ``noexcept``. (It would be inappropriate for a Clang 
+attribute, ignored by non-Clang compilers, to imply a standard language feature.)
+
+``nonblocking(true)`` and ``nonallocating(true)`` apply to function *types*, and by extension, to
+function-like declarations. When applied to a declaration with a body, the compiler verifies the
+function, as described in the section "Analysis and warnings", below. Functions without an explicit
+performance constraint are not verified.
+
+``nonblocking(false)`` and ``nonallocating(false)`` are synonyms for the attributes ``blocking`` and
+``allocating``. They can be used on a function-like declaration to explicitly disable any potential
+inference of ``nonblocking`` or ``nonallocating`` during verification. (Inference is described later
+in this document). ``nonblocking(false)`` and ``nonallocating(false)`` are legal, but superfluous 
+when applied to a function *type*. ``float (int) [[nonblocking(false)]]`` and ``float (int)`` are
+identical types.
+
+For all functions with no explicit performance constraint, the worst is assumed, that the function
+allocates memory and potentially blocks, unless it can be inferred otherwise, as described in the
+discussion of verification.
+
+The following list describes the meanings of all permutations of the two attributes and arguments:
+
+- ``nonblocking(true)`` + ``nonallocating(true)``: valid; ``nonallocating(true)`` is superfluous but
+  does not contradict the guarantee.
+- ``nonblocking(true)`` + ``nonallocating(false)``: error, contradictory.
+- ``nonblocking(false)`` + ``nonallocating(true)``: valid; the function does not allocate memory,
+  but may lock for other reasons.
+- ``nonblocking(false)`` + ``nonallocating(false)``: valid.
+
+Type conversions
+----------------
+
+A performance constraint can be removed or weakened via an implicit conversion. An attempt to add
+or strengthen a performance constraint is unsafe and results in a warning.
+
+.. code-block:: c++
+
+  void unannotated();
+  void nonblocking() [[clang::nonblocking]];
+  void nonallocating() [[clang::nonallocating]];
+
+  void example()
+  {
+    // It's fine to remove a performance constraint.
+    void (*fp_plain)();
+    fp_plain = unannotated;
+    fp_plain = nonblocking;
+    fp_plain = nonallocating;
+
+    // Adding/spoofing nonblocking is unsafe.
+    void (*fp_nonblocking)() [[clang::nonblocking]];
+    fp_nonblocking = nullptr;
+    fp_nonblocking = nonblocking;
+    fp_nonblocking = unannotated;
+    // ^ warning: attribute 'nonblocking' should not be added via type conversion
+    fp_nonblocking = nonallocating;
+    // ^ warning: attribute 'nonblocking' should not be added via type conversion
+
+    // Adding/spoofing nonallocating is unsafe.
+    void (*fp_nonallocating)() [[clang::nonallocating]];
+    fp_nonallocating = nullptr;
+    fp_nonallocating = nonallocating;
+    fp_nonallocating = nonblocking; // no warning because nonblocking includes nonallocating 
+    fp_nonallocating = unannotated;
+    // ^ warning: attribute 'nonallocating' should not be added via type conversion
+  }
+
+Virtual methods
+---------------
+
+In C++, when a base class's virtual method has a performance constraint, overriding methods in
+subclasses inherit the attribute.
+
+.. code-block:: c++
+
+  struct Base {
+    virtual void unsafe();
+    virtual void safe() noexcept [[clang::nonblocking]];
+  };
+
+  struct Derived : public Base {
+    void unsafe() [[clang::nonblocking]] override;
+    // It's okay for an overridden method to be more constrained
+
+    void safe() noexcept override;
+    // This method is implicitly declared `nonblocking`, inherited from Base.
+  };
+
+Redeclarations, overloads, and name mangling
+--------------------------------------------
+
+The ``nonblocking`` and ``nonallocating`` attributes, like ``noexcept``, do not factor into
+argument-dependent lookup and overloaded functions/methods.
+
+First, consider that ``noexcept`` is integral to a function's type:
+
+.. code-block:: c++
+
+  void f1(int);
+  void f1(int) noexcept;
+  // error: exception specification in declaration does not match previous
+  //   declaration
+
+Unlike ``noexcept``, a redeclaration of `f2` with an added or stronger performance constraint is
+legal, and propagates the attribute to the previous declaration:
+
+.. code-block:: c++
+
+  int f2();
+  int f2() [[clang::nonblocking]]; // redeclaration with stronger constraint is OK.
+
+This greatly eases adoption, by making it possible to annotate functions in external libraries
+without modifying library headers.
+
+A redeclaration with a removed or weaker performance constraint produces a warning, in order to
+parallel the behavior of ``noexcept``:
+
+.. code-block:: c++
+
+  int f2() { return 42; }
+  // warning: attribute 'nonblocking' on function does not match previous declaration
+
+In C++14, the following two declarations of `f3` are identical (a single function). In C++17 they
+are separate overloads:
+
+.. code-block:: c++
+
+  void f3(void (*)());
+  void f3(void (*)() noexcept);
+
+Similarly, the following two declarations of `f4` are separate overloads. This pattern may pose
+difficulties due to ambiguity:
+
+.. code-block:: c++
+
+  void f4(void (*)());
+  void f4(void (*)() [[clang::nonblocking]]);
+
+The attributes have no effect on the mangling of function and method names.
+
+``noexcept``
+------------
+
+``nonblocking`` and ``nonallocating`` are conceptually similar to a stronger form of C++'s
+``noexcept``, but with further diagnostics, as described later in this document. Therefore, in C++,
+a ``nonblocking`` or ``nonallocating`` function, method, block or lambda should also be declared
+``noexcept``.[^6] If ``noexcept`` is missing, a warning is issued. In Clang, this diagnostic is
+controlled by ``-Wperf-constraint-implies-noexcept``.
+
+Objective-C
+-----------
+
+The attributes are currently unsupported on Objective-C methods.
+
+Analysis and warnings
+=====================
+
+Constraints
+-----------
+
+Functions declared ``nonallocating`` or ``nonblocking``, when defined, are verified according to the
+following rules. Such functions:
+
+1. May not allocate or deallocate memory on the heap. The analysis follows the calls to
+   ``operator new`` and ``operator delete`` generated by the ``new`` and ``delete`` keywords, and
+   treats them like any other function call. The global ``operator new`` and ``operator delete``
+   aren't declared ``nonblocking`` or ``nonallocating`` and so they are considered unsafe. (This
+   is correct because most memory allocators are not lock-free. Note that the placement form of
+   ``operator new`` is implemented inline in libc++'s ``<new>`` header, and is verifiably
+   ``nonblocking``, since it merely casts the supplied pointer to the result type.)
+
+2. May not throw or catch exceptions. To throw, the compiler must allocate the exception on the
+   heap. (Also, many subclasses of ``std::exception`` allocate a ``std::string``). Exceptions are
+   deallocated when caught.
+
+3. May not make any indirect function call, via a virtual method, function pointer, or
+   pointer-to-member function, unless the target is explicitly declared with the same
+   ``nonblocking`` or ``nonallocating`` attribute (or stronger).
+
+4. May not make direct calls to any other function, with the following exceptions:
+
+  a. The callee is also explicitly declared with the same ``nonblocking`` or ``nonallocating``
+     attribute (or stronger).
+  b. The callee is defined in the same translation unit as the caller, does not have the ``false``
+     form of the required attribute, and can be verified to be have the same attribute or stronger,
+     according to these same rules.
+  c. The callee is a built-in function (other than builtins which are known to block or allocate).
+  d. The callee is declared ``noreturn`` and, if compiling C++, the callee is also declared
+     ``noexcept``. This exception excludes functions such as ``abort()`` and ``std::terminate()``
+     from the analysis.
+
+5. May not invoke or access an Objective-C method or property, since ``objc_msgSend()`` calls into 
+   the Objective-C runtime, which may allocate memory or otherwise block.
+
+Functions declared ``nonblocking`` have an additional constraint:
+
+6. May not declare static local variables (e.g. Meyers singletons). The compiler generates a lock
+   protecting the initialization of the variable.
+
+Violations of any of these rules result in warnings:
+
+.. code-block:: c++
+
+  void notInline();
+
+  void example() [[clang::nonblocking]]
+  {
+    auto* x = new int;
+    // warning: function with 'nonblocking' attribute must not allocate or deallocate
+    //   memory
+
+    if (x == nullptr) {
+      static Logger* logger = createLogger();
+      // warning: function with 'nonblocking' attribute must not have static locals
+
+      throw std::runtime_warning{ "null" };
+      // warning: 'nonblocking" function 'example' must not throw exceptions
+    }
+    notInline();
+    // warning: 'function with 'nonblocking' attribute must not call non-'nonblocking' function
+    //   'notInline'
+    // note (on notInline()): declaration cannot be inferred 'nonblocking' because it has no
+    //   definition in this translation unit
+  }
+
+Inferring ``nonblocking`` or ``nonallocating``
+----------------------------------------------
+
+In the absence of a ``nonblocking`` or ``nonallocating`` attribute (whether ``true`` or ``false``),
+a function, when found to be called from a performance-constrained function, can be analyzed to
+infer whether it has a desired attribute. This analysis happens when the function is not a virtual
+method, and it has a visible definition within the current translation unit (i.e. its body can be
+traversed).
+
+.. code-block:: c++
+
+  void notInline();
+  int implicitlySafe() { return 42; }
+  void implicitlyUnsafe() { notInline(); }
+
+  void example() [[clang::nonblocking]]
+  {
+    int x = implicitlySafe(); // OK
+    implicitlyUnsafe();
+    // warning: function with 'nonblocking' attribute must not call non-'nonblocking' function
+    //   'implicitlyUnsafe'
+    // note (on implicitlyUnsafe): function cannot be inferred 'nonblocking' because it calls
+    //   non-'nonblocking' function 'notInline'
+    // note (on notInline()): declaration cannot be inferred 'nonblocking' because it has no
+    //   definition in this translation unit
+  }
+
+Lambdas and blocks
+------------------
+
+As mentioned earlier, the performance constraint attributes apply only to a single function and not
+to any code nested inside it, including blocks, lambdas, and local classes. It is possible for a
+lock-free function to schedule the execution of a blocking lambda on another thread. Similarly, a
+blocking function may create a ``nonblocking`` lambda for use in a realtime context.
+
+Operations which create, destroy, copy, and move lambdas and blocks are analyzed in terms of the
+underlying function calls. For example, the creation of a lambda with captures generates a function
+call to an anonymous struct's constructor, passing the captures as parameters.
+
+Implicit function calls in the AST
+----------------------------------
+
+The ``nonblocking`` / ``nonallocating`` analysis occurs at the Sema phase of analysis in Clang.
+During Sema, there are some constructs which will eventually become function calls, but do not
+appear as function calls in the AST. For example, ``auto* foo = new Foo;`` becomes a declaration
+containing a ``CXXNewExpr`` which is understood as a function call to the global ``operator new``
+(in this example), and a ``CXXConstructExpr``, which, for analysis purposes, is a function call to
+``Foo``'s constructor. Most gaps in the analysis would be due to incomplete knowledge of AST
+constructs which become function calls.
+
+Disabling diagnostics
+---------------------
+
+Function effect diagnostics are controlled by ``-Wfunction-effects``.
+
+A construct like this can be used to exempt code from the checks described here:
+
+.. code-block:: c++
+
+  #define NONBLOCKING_UNSAFE(...)                                         \
+    _Pragma("clang diagnostic push")                                 \
+    _Pragma("clang diagnostic ignored \"-Wunknown-warning-option\"") \
+    _Pragma("clang diagnostic ignored \"-Wfunction-effects\"")       \
+    __VA_ARGS__                                                      \
+    _Pragma("clang diagnostic pop")
+
+Disabling the diagnostic allows for:
+
+- constructs which do block, but which in practice are used in ways to avoid unbounded blocking,
+  e.g. a thread pool with semaphores to coordinate multiple realtime threads.
+- using libraries which are safe but not yet annotated.
+- incremental adoption in a large codebase.
+
+Adoption
+========
+
+There are a few common issues that arise when adopting the ``nonblocking`` and ``nonallocating``
+attributes.
+
+C++ exceptions
+--------------
+
+Exceptions pose a challenge to the adoption of the performance constraints. Common library functions
+which throw exceptions include:
+
++----------------------------------+-----------------------------------------------------------------------+
+| Method                           | Alternative                                                           |
++==================================+=======================================================================+
+| ``std::vector<T>::at()``         | ``operator[](size_t)``, after verifying that the index is in range.   |
++----------------------------------+-----------------------------------------------------------------------+
+| ``std::optional<T>::value()``    | ``operator*``, after checking ``has_value()`` or ``operator bool()``. |
++----------------------------------+-----------------------------------------------------------------------+
+| ``std::expected<T, E>::value()`` | Same as for ``std::optional<T>::value()``.                            |
++----------------------------------+-----------------------------------------------------------------------+
+
+Interactions with type-erasure techniques
+-----------------------------------------
+
+``std::function<R(Args...)>`` illustrates a common C++ type-erasure technique. Using template
+argument deduction, it decomposes a function type into its return and parameter types. Additional
+components of the function type, including ``noexcept``, ``nonblocking``, ``nonallocating``, and any
+other attributes, are discarded.
+
+Standard library support for these components of a function type is not immediately forthcoming.
+
+Code can work around this limitation in either of two ways:
+
+1. Avoid abstractions like ``std::function`` and instead work directly with the original lambda type.
+
+2. Create a specialized alternative, e.g. ``nonblocking_function<R(Args...)>`` where all function
+   pointers used in the implementation and its interface are ``nonblocking``.
+
+As an example of the first approach, when using a lambda as a *Callable* template parameter, the
+attribute is preserved:
+
+.. code-block:: c++
+
+  std::sort(vec.begin(), vec.end(),
+    [](const Elem& a, const Elem& b) [[clang::nonblocking]] { return a.mem < b.mem; });
+
+Here, the type of the ``Compare`` template parameter is an anonymous class generated from the
+lambda, with an ``operator()`` method holding the ``nonblocking`` attribute.
+
+A complication arises when a *Callable* template parameter, instead of being a lambda or class
+implementing ``operator()``, is a function pointer:
+
+.. code-block:: c++
+
+  static bool compare_elems(const Elem& a, const Elem& b) [[clang::nonblocking]] {
+    return a.mem < b.mem; };
+
+  std::sort(vec.begin(), vec.end(), compare_elems);
+
+Here, the type of ``compare_elems`` is decomposed to ``bool(const Elem&, const Elem&)``, without
+``nonblocking``, when forming the template parameter. This can be solved using the second approach,
+creating a specialized alternative which explicitly requires the attribute. In this case, it's
+possible to use a small wrapper to transform the function pointer into a functor:
+
+.. code-block:: c++
+
+  template <typename>
+  class nonblocking_fp;
+
+  template <typename R, typename... Args>
+  class nonblocking_fp<R(Args...)> {
+  public:
+    using impl_t = R (*)(Args...) [[clang::nonblocking]];
+
+  private:
+    impl_t mImpl{ nullptr_t };
+  public:
+    nonblocking_fp() = default;
+    nonblocking_fp(impl_t f) : mImpl{ f } {}
+
+    R operator()(Args... args) const
+    {
+      return mImpl(std::forward<Args>(args)...);
+    }
+  };
+
+  // deduction guide (like std::function's)
+  template< class R, class... ArgTypes >
+  nonblocking_fp( R(*)(ArgTypes...) ) -> nonblocking_fp<R(ArgTypes...)>;
+
+  // --
+
+  // Wrap the function pointer in a functor which preserves ``nonblocking``.
+  std::sort(vec.begin(), vec.end(), nonblocking_fp{ compare_elems });
+
+Now, the ``nonblocking`` attribute of ``compare_elems`` is verified when it is converted to a
+``nonblocking`` function pointer, as the argument ...
[truncated]

Copy link
Contributor

@cjappl cjappl left a comment

Choose a reason for hiding this comment

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

(partial review, will do another pass later today -- overall looks great so far)

Introduction
============

Clang Function Effect Analysis is a C++ language extension which can warn about "unsafe"
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not only C++, but also works in C I think?

Copy link
Contributor

Choose a reason for hiding this comment

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

May be worth documenting how to use in C, I know the syntax is a little different with the attrs

Copy link
Member

Choose a reason for hiding this comment

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

I don’t think we need to explain the different attribute syntaxes in too much detail here (you mention them below, and what’s there is enough imo), but that should just say ‘language extension’ w/o the ‘C++’, yeah.

identical types.

For all functions with no explicit performance constraint, the worst is assumed, that the function
allocates memory and potentially blocks, unless it can be inferred otherwise, as described in the
Copy link
Contributor

Choose a reason for hiding this comment

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

nit/style (take it or leave it) - some commas could be removed

Copy link
Member

Choose a reason for hiding this comment

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

I mean, if we’re talking about style, I’d replace some of them w/ colons and en/em dashes, but I don’t think this has to be stylistically perfect (I could point out all the places I noticed if @dougsonos wants me to; I quite like proofreading as someone w/ a background in linguistics, but I try not to annoy people w/ that too much ;Þ).

Generally, my approach to comments/documentation is, if it’s intelligible and not horribly wrong, then it’s fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Sirraide, despite my being a former English major who dropped out to hack on computers and play in bands, I don't mind having my writing picked apart at all! Thanks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Though, I am in the middle of incorporating this round of feedback + fixing whatever else looks wrong...)


Functions declared ``nonblocking`` have an additional constraint:

6. May not declare static local variables (e.g. Meyers singletons). The compiler generates a lock
Copy link
Contributor

Choose a reason for hiding this comment

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

I think more appropriately "the compiler may generate a lock" -- I'm not sure it is always guaranteed to generate a lock, but will in some circumstances. Someone with more experience than me can confirm or deny

Copy link
Member

Choose a reason for hiding this comment

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

[stmt.decl]p3:

Dynamic initialization of a block variable with static storage duration or thread storage duration is performed the first time control passes through its declaration; [...] If control enters the declaration concurrently while the variable is being initialized, the concurrent execution shall wait for completion of the initialization.

According to the standard, we ought to emit a lock; however, for things that are trivial, such as static int x = 3; (https://godbolt.org/z/Y9qqzj5fe), we just do the initialisation at compile-time, but I’m pretty sure that’s just an optimisation by the as-if rule.

So yes, ‘the compiler may generate a lock’ is probably more appropriate.

Side note: We could try and be clever and detect situations where we don’t generate a lock and allow such static locals, but 1. I don’t know what part of Clang knows about this off the top of my head, and it might be non-trivial to get a hold of this information, and 2. other compilers might have different criteria for this than Clang (GCC often tries really hard to evaluate everything at compile-time, for instance), so this might end up making code non-portable, so I don’t think that would be a good idea overall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, when I looked at this a long time ago, I found that the decision in the back-end to emit a lock (or not) didn't seem trivially (ha) replicable in Sema.

Copy link
Contributor

@cjappl cjappl left a comment

Choose a reason for hiding this comment

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

Overall, very thorough. I would wait for more eyes but this LGTM


Like ``noexcept``, ``nonblocking`` and ``nonallocating`` have an optional argument, a compile-time
constant boolean expression. By default, the argument is true, so ``[[clang::nonblocking(true)]]``
is equivalent to ``[[clang::nonblocking]]``, and declares the function type as never locking.
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be good to mention these also control RealtimeSanitizer and do a link to our documentation page.

After this goes in I can submit a PR to cross link from that page here, as I think it's beneficial to the user to see the connection seeing as the attrs are shared

- ``nonblocking(true)`` + ``nonallocating(false)``: error, contradictory.
- ``nonblocking(false)`` + ``nonallocating(true)``: valid; the function does not allocate memory,
but may lock for other reasons.
- ``nonblocking(false)`` + ``nonallocating(false)``: valid.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit (take it or leave it): This may possibly be clearer to show in a code block with the warnings emitted? That matches the style in the rest of the doc and also gives good search strings for the user wondering what's going wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had this as a table in the RFC but translating it to reStructuredText threatened to take up the whole afternoon... I agree a code block would probably be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just discovered this table format when updating our docs, which is way way faster and more sustainable to write:

.. list-table:: Runtime Flags
   :widths: 20 10 70
   :header-rows: 1

   * - Flag name
     - Default value
     - Short description
   * - halt_on_error
     - true
     - Exit after first reported error.
   * - print_stats_on_exit
     - false
     - Exit after first reported error. If false, deduplicates error stacks so errors only appear once.
   * - color
     - auto
     - Colorize reports: (always|never|auto).
   * - fast_unwind_on_fatal
     - false
     - If available, use the fast frame-pointer-based unwinder on detected errors.

Saved me a lot of adjusting/readjusting the "cells" by hand.

clang/docs/FunctionEffectAnalysis.rst Show resolved Hide resolved
clang/docs/FunctionEffectAnalysis.rst Outdated Show resolved Hide resolved
clang/docs/FunctionEffectAnalysis.rst Show resolved Hide resolved
clang/docs/FunctionEffectAnalysis.rst Show resolved Hide resolved
clang/docs/FunctionEffectAnalysis.rst Show resolved Hide resolved
Introduction
============

Clang Function Effect Analysis is a C++ language extension which can warn about "unsafe"
Copy link
Member

Choose a reason for hiding this comment

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

I don’t think we need to explain the different attribute syntaxes in too much detail here (you mention them below, and what’s there is enough imo), but that should just say ‘language extension’ w/o the ‘C++’, yeah.

clang/docs/FunctionEffectAnalysis.rst Outdated Show resolved Hide resolved
Comment on lines 77 to 78
attribute does not implicitly specify ``noexcept``. (It would be inappropriate for a Clang
attribute, ignored by non-Clang compilers, to imply a standard language feature.)
Copy link
Member

Choose a reason for hiding this comment

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

I’d mention that, despite that, there is a warning to tell users to add noexcept if either attribute is specified.

Copy link
Member

Choose a reason for hiding this comment

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

See my comment on the noexcept section below.

clang/docs/FunctionEffectAnalysis.rst Outdated Show resolved Hide resolved
identical types.

For all functions with no explicit performance constraint, the worst is assumed, that the function
allocates memory and potentially blocks, unless it can be inferred otherwise, as described in the
Copy link
Member

Choose a reason for hiding this comment

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

I mean, if we’re talking about style, I’d replace some of them w/ colons and en/em dashes, but I don’t think this has to be stylistically perfect (I could point out all the places I noticed if @dougsonos wants me to; I quite like proofreading as someone w/ a background in linguistics, but I try not to annoy people w/ that too much ;Þ).

Generally, my approach to comments/documentation is, if it’s intelligible and not horribly wrong, then it’s fine.


Functions declared ``nonblocking`` have an additional constraint:

6. May not declare static local variables (e.g. Meyers singletons). The compiler generates a lock
Copy link
Member

Choose a reason for hiding this comment

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

[stmt.decl]p3:

Dynamic initialization of a block variable with static storage duration or thread storage duration is performed the first time control passes through its declaration; [...] If control enters the declaration concurrently while the variable is being initialized, the concurrent execution shall wait for completion of the initialization.

According to the standard, we ought to emit a lock; however, for things that are trivial, such as static int x = 3; (https://godbolt.org/z/Y9qqzj5fe), we just do the initialisation at compile-time, but I’m pretty sure that’s just an optimisation by the as-if rule.

So yes, ‘the compiler may generate a lock’ is probably more appropriate.

Side note: We could try and be clever and detect situations where we don’t generate a lock and allow such static locals, but 1. I don’t know what part of Clang knows about this off the top of my head, and it might be non-trivial to get a hold of this information, and 2. other compilers might have different criteria for this than Clang (GCC often tries really hard to evaluate everything at compile-time, for instance), so this might end up making code non-portable, so I don’t think that would be a good idea overall.

6. May not declare static local variables (e.g. Meyers singletons). The compiler generates a lock
protecting the initialization of the variable.

Violations of any of these rules result in warnings:
Copy link
Member

Choose a reason for hiding this comment

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

I’d also mention the warning category here if you don’t already do that somewhere.

clang/docs/FunctionEffectAnalysis.rst Show resolved Hide resolved
clang/docs/FunctionEffectAnalysis.rst Show resolved Hide resolved
This may help in adopting the feature incrementally.

It also helps that for many of the functions in ``<math.h>``, Clang generates calls to built-in
functions, which the diagnosis understands to be safe.
Copy link
Member

Choose a reason for hiding this comment

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

I think we generally just treat most C standard library functions as builtins, not just <cmath>, don’t we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, math.h is just the first cluster of examples I ran into in adoption...

@dougsonos
Copy link
Contributor Author

Ping

@dougsonos
Copy link
Contributor Author

Ping

Copy link
Member

@Sirraide Sirraide left a comment

Choose a reason for hiding this comment

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

Just gave the entire thing another read, and I have a few nits and comments, but other than that, I’m personally fine w/ it.

clang/docs/FunctionEffectAnalysis.rst Outdated Show resolved Hide resolved
clang/docs/FunctionEffectAnalysis.rst Outdated Show resolved Hide resolved
clang/docs/FunctionEffectAnalysis.rst Outdated Show resolved Hide resolved
clang/docs/FunctionEffectAnalysis.rst Outdated Show resolved Hide resolved
clang/docs/FunctionEffectAnalysis.rst Outdated Show resolved Hide resolved
clang/docs/FunctionEffectAnalysis.rst Outdated Show resolved Hide resolved
clang/docs/FunctionEffectAnalysis.rst Outdated Show resolved Hide resolved
clang/docs/FunctionEffectAnalysis.rst Outdated Show resolved Hide resolved
clang/docs/FunctionEffectAnalysis.rst Outdated Show resolved Hide resolved
clang/docs/FunctionEffectAnalysis.rst Outdated Show resolved Hide resolved
dougsonos and others added 2 commits October 28, 2024 10:38
@dougsonos
Copy link
Contributor Author

@Sirraide, thanks for the excellent feedback. I've incorporated all of your suggestions.

@cjappl
Copy link
Contributor

cjappl commented Oct 28, 2024

@dougsonos let me know if you want to merge this, happy to push the button when you are OK with it

@dougsonos
Copy link
Contributor Author

@cjappl please merge at will. Thank you!

@cjappl
Copy link
Contributor

cjappl commented Oct 28, 2024

Happy to help. I'll merge this afternoon after the checks are green.

Excited to have these docs up, had many conversations where I've wanted to refer people to them!

@cjappl cjappl merged commit 034cae4 into llvm:main Oct 28, 2024
9 checks passed
@Sirraide
Copy link
Member

@dougsonos Now that this document exists, we should also update the function effects release note that the part 2 pr added to include a link to it. 👀

NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
Follow-on from llvm#99656, which introduces 2nd pass caller/callee analysis
for function effects.

Wrote a new documentation page, derived directly from the RFC posted to
LLVM Discourse earlier this year.

---------

Co-authored-by: Doug Wyatt <dwyatt@apple.com>
Co-authored-by: Sirraide <aeternalmail@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants