[ADT] function_ref captures function pointers by value

[sammccall]

Summary

Currently function_ref always stores a pointer to its callee, and
expects the callee to stay alive.
This has two surprising consequences.

First, function_ref(&someFreeFunction) is often incorrect: it stores a pointer
to a *temporary* function pointer, and the outer pointer will often dangle.
function_ref(someFreeFunction) does the right thing, stores a plain pointer.
This is surprising because referring to a free function as x or &x often
has the same effect (or produces a diagnostic).

Second, function_ref(someFreeFunction) is miscompiled by GCC 5: it
behaves like function_ref(&someFreeFunction).
https://godbolt.org/z/G8Tj4zWWW

Regarding compatibility with upcoming std::function_ref, the
function_ref(&someFreeFunction) case is mentioned in
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0792r5.html
but I can't really understand how the resolution relates to it.

[sammccall]

I suppose this "breaks" using function_ref to capture by reference a function pointer that later gets reassigned. This doesn't seem very likely to be an issue?

[dblaikie]

What advantage would this narrow use case have over using a raw function pointer (a function pointer member, in the case of the "S" example, for instance)? implicit parameter/return type conversions? Usually I find those to be more hazardous than intentional.

[chandlerc]

An alternative would be to delete this overload and insist callers pass a lambda that itself calls the desired function. This makes the user of function_ref more verbose, but it avoids having two dynamic dispatches (I think).

[sammccall]

In D106784#2905600, @dblaikie wrote:

What advantage would this narrow use case have over using a raw function pointer (a function pointer member, in the case of the "S" example, for instance)?

Typically you accept function_ref in an interface, and don't want to constrain callers to only passing things that can be expressed as function pointers.

In the case where we found the S bug, this wasn't the case (it was just a function pointer type with better syntax). But AFAIK that's just a coincidence.

In D106784#2905762, @chandlerc wrote:

An alternative would be to delete this overload and insist callers pass a lambda that itself calls the desired function. This makes the user of function_ref more verbose, but it avoids having two dynamic dispatches (I think).

Yeah, the double indirection is a fair point, using function pointers to represent functions we know statically is wasteful.

But the ergonomics are pretty bad for functions with complicated (matching) signatures. And it will involve uglifying lots of places where there's no *correctness* problem even on buggy GCC. (If the function_ref is passed to a function and invoked synchronously then the pointer lives long enough).

[chandlerc]

llvm/include/llvm/ADT/STLExtras.h

168–170

The more I think about this the more I feel like we shouldn't try to make this promise... it seems too narrow to be useful. I think maybe if we want to do something here, causing things to break immediately rather than kinda-sorta working is better.

For the code where this came up, I think just moving away from function_ref is a much better approach than relying on the special behavior in the case of a pointer.

But happy to defer to dblaikie here ultimately.

[sammccall]

Seems like there's 3 alternatives at this point:

• support function pointers by value (this patch)
• disallow function pointers/references entirely (breaks existing code)
• function pointers are UB if the pointer dies, work around the GCC bug somehow (std::function_ref takes this path)

llvm/include/llvm/ADT/STLExtras.h

168–170

it seems too narrow to be useful

I think it's useful - it fixes broken code people expect to write e.g. this looks like an existing bug. And I'm not sure that making it broader would actually be more useful! Nobody expects to be able to *store* e.g. a temporary lambda in a function_ref, but I think that expectation exists for function pointers.

However FWIW on the standard track it's not going to be supported for consistency reasons: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0792r5.html#lifetime-of-pointers-to-function (missed this on my first scan).

For the code where this came up, I think just moving away from function_ref is a much better approach

Agreed, I've committed such a fix to make sure we have something in time for the 13 branch.

[dblaikie]

In D106784#2905919, @sammccall wrote:

In D106784#2905600, @dblaikie wrote:

What advantage would this narrow use case have over using a raw function pointer (a function pointer member, in the case of the "S" example, for instance)?

Typically you accept function_ref in an interface, and don't want to constrain callers to only passing things that can be expressed as function pointers.

In the case where we found the S bug, this wasn't the case (it was just a function pointer type with better syntax). But AFAIK that's just a coincidence.

I'm not sure I follow how "that's just a coincidence" - it looks to me like the only place this could come up is a place where lambdas and other things that aren't function pointers would not be usable. So this feature could only be used in cases where the constraint is "only use function pointers, if you use anything else it'll break subtly/invoke UB". That doesn't seem like a good API design to carve out this special case while still leaving most of the uses of function_ref in the same context, UB.

I guess it comes down to: Are there situations where this patch fixes code that would've been invalid, but where the same code could use a lambda and be correct?

llvm/include/llvm/ADT/STLExtras.h

168–170

Yeah, that's basically where I am too.

[dblaikie]

(a temporary lambda, specifically)

llvm/include/llvm/ADT/STLExtras.h

 };
 
 /// An efficient, type-erasing, non-owning reference to a callable. This is
 /// intended for use as the type of a function parameter that is not used
 /// after the function in question returns.
 ///
 /// This class does not own the callable, so it is not in general safe to store
 /// a function_ref.
+///
+/// (As an exception, function_ref(&someFreeFunction) is safe, even though the
+/// callable is technically a temporary function pointer).
 template<typename Fn> class function_ref;
 
 template<typename Ret, typename ...Params>
 class function_ref<Ret(Params...)> {
   Ret (*callback)(intptr_t callable, Params ...params) = nullptr;
   intptr_t callable;
 
   template<typename Callable>
function_ref(
       // Functor must be callable and return a suitable type.
       std::enable_if_t<std::is_void<Ret>::value ||
                        std::is_convertible<decltype(std::declval<Callable>()(
                                                std::declval<Params>()...)),
                                            Ret>::value> * = nullptr)
       : callback(callback_fn<typename std::remove_reference<Callable>::type>),
         callable(reinterpret_cast<intptr_t>(&callable)) {}
 
+  // Overload for function pointers.
+  // We store the function pointer itself in callback, therefore we don't rely
+  // on the caller keeping the pointer (which may be a temporary) alive.
+  template <typename PRet, typename... PParams>
+  function_ref(
+      PRet (*fptr)(PParams...),
+      // Pointer must be callable and return a suitable type.
+      std::enable_if_t<
+          std::is_void<Ret>::value ||
+          std::is_convertible<decltype(std::declval<PRet (*)(PParams...)>()(
+                                  std::declval<Params>()...)),
+                              Ret>::value> * = nullptr)
+      : callback(callback_fn<PRet(PParams...)>),
+        callable(reinterpret_cast<intptr_t>(fptr)) {}
+
   Ret operator()(Params ...params) const {
     return callback(callable, std::forward<Params>(params)...);
   }
 
   explicit operator bool() const { return callback; }
 };
 
 //===----------------------------------------------------------------------===//

llvm/unittests/ADT/FunctionRefTest.cpp

TEST(FunctionRefTest, BadCopy) {
   function_ref<int()> X;
   function_ref<int()> Y = A;
   function_ref<int()> Z = static_cast<const function_ref<int()> &&>(Y);
   X = Z;
   Y = nullptr;
   ASSERT_EQ(1, X());
 }
 
+// Ensure that function_ref captures a function pointer by value.
+// This means we don't have to keep the original pointer alive.
+TEST(FunctionRefTest, FunctionPointer) {
+  static int X;
+  void (*Inc)(void) = [] { ++X; };
+  void (*Dec)(void) = [] { ++X; };
+  function_ref<void()> IncRef = Inc;
+
+  X = 0;
+  IncRef();
+  EXPECT_EQ(X, 1);
+  Inc = Dec; // not nullptr as UB may be optimized.
+  IncRef();
+  EXPECT_EQ(X, 2);
+}
+
+static int square(int X) { return X * X; }
+
+// Ensure function_ref binds properly to free functions.
+// This uses the function-pointer-by-value constructor.
+// (Using the "normal" constructor with a function ref caused issues in GCC 5
+// due to too-eager decay: See https://github.com/clangd/clangd/issues/800)
+TEST(FunctionRefTest, FreeFunction) {
+  struct S {
+    function_ref<int(int)> Call;
+  };
+  S Wrapper = S{square};
+  EXPECT_EQ(4, Wrapper.Call(2));
+}
+
 // Test that overloads on function_refs are resolved as expected.
 std::string returns(StringRef) { return "not a function"; }
 std::string returns(function_ref<double()> F) { return "number"; }
 std::string returns(function_ref<StringRef()> F) { return "string"; }
 
 TEST(FunctionRefTest, SFINAE) {
   EXPECT_EQ("not a function", returns("boo!"));
   EXPECT_EQ("number", returns([] { return 42; }));
   EXPECT_EQ("string", returns([] { return "hello"; }));
 }
 
 } // namespace