[PATCH] D106784: [ADT] function_ref captures function pointers by value

[Sam McCall via Phabricator via llvm-commits]

sammccall created this revision.
sammccall added reviewers: chandlerc, rsmith.
Herald added a subscriber: dexonsmith.
sammccall requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106784

Files:
  llvm/include/llvm/ADT/STLExtras.h
  llvm/unittests/ADT/FunctionRefTest.cpp


Index: llvm/unittests/ADT/FunctionRefTest.cpp
===================================================================
--- llvm/unittests/ADT/FunctionRefTest.cpp
+++ llvm/unittests/ADT/FunctionRefTest.cpp
@@ -48,6 +48,36 @@
   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"; }
Index: llvm/include/llvm/ADT/STLExtras.h
===================================================================
--- llvm/include/llvm/ADT/STLExtras.h
+++ llvm/include/llvm/ADT/STLExtras.h
@@ -165,6 +165,9 @@
 ///
 /// 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>
@@ -196,6 +199,21 @@
       : 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)...);
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D106784.361604.patch
Type: text/x-patch
Size: 2901 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210726/b4f6fbf5/attachment.bin>