Reapply [ADT] function_ref's constructor is unavailable if the argument is not callable.

[sammccall]

Summary

This reverts commit 281703e67ffaee8e26efef86e0df3e145477f4cb.

GCC 5.4 bugs are worked around by avoiding use of variable templates.

[kadircet]

If you have some pointers to gcc bug being discussed in some place, it might be nice to include.

llvm/include/llvm/ADT/STLExtras.h

201

nit: I would propbably merge the two into a single parameter, but up to you.

std::enable_if_t<
// This is not the copy-constructor.
!std::is_same<std::remove_cv_t<std::remove_reference_t<Callable>>,
                        function_ref>::value &&
// Functor must be callable and return a suitable type.
(std::is_void<Ret>::value || std::is_convertible<
                           std::result_of_t<Callable(Params...)>, Ret>::value)> * = nullptr

[sammccall]

In D88977#2317022, @kadircet wrote:

If you have some pointers to gcc bug being discussed in some place, it might be nice to include.

I didn't find them, and they're fixed as since gcc 7 so it doesn't really make sense to file a bug :-(

llvm/include/llvm/ADT/STLExtras.h

201

Clang (and maybe GCC?) has special support for diagnosing substitution failure with enable_if: it says "requirement failed" and dumps the expression. So the fewer/larger the expressions are, the less useful it is.

[JamesNagurne]

I believe our teams' downstream Darwin (cross compiler to ARM) builds are failing due to this commit:

Apple LLVM version 9.1.0 (clang-902.0.39.2)
Target: x86_64-apple-darwin17.7.0
Thread model: posix

Cutdown: https://godbolt.org/z/dePeze

In file included from bla.cpp:2:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:4039:19: error: 
      invalid application of 'sizeof' to a function type
    static_assert(sizeof(_Tp) > 0, "Type must be complete.");
                  ^~~~~~~~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:4044:15: note: 
      in instantiation of template class 'std::__1::__check_complete<void ()
      __attribute__((noreturn))>' requested here
    : private __check_complete<_Tp>
              ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:4337:15: note: 
      in instantiation of template class 'std::__1::__check_complete<void (&)()
      __attribute__((noreturn))>' requested here
    : private __check_complete<_Fp>
              ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:4392:9: note: 
      in instantiation of template class 'std::__1::__invokable_r<void, void (&)()
      __attribute__((noreturn))>' requested here
        __invokable<_Fp, _Args...>::value,
        ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:4401:14: note: 
      in instantiation of template class 'std::__1::__invoke_of<void (&)()
      __attribute__((noreturn))>' requested here
    : public __invoke_of<_Fp, _Args...>
             ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:4406:22: note: 
      in instantiation of template class 'std::__1::result_of<void (&())()
      __attribute__((noreturn))>' requested here
template <class _Tp> using result_of_t = typename result_of<_Tp>::type;
                     ^
bla.cpp:33:33: note: in instantiation of template type alias 'result_of_t' requested here
                           std::result_of_t<Callable(Params...)>, Ret>::value>
                                ^
bla.cpp:51:13: note: while substituting deduced template arguments into function template
      'function_ref' [with Callable = void (&)() __attribute__((noreturn))]
    myY.foo(abort);
            ^
1 error generated.

> cat /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__libcpp_version
5000

This error seems to be caused by using a libcxx implementation from before monorepo commit 11a18a79 (2018-05-10), which removes the implementation of "check_complete". The problem is that in the implementation of "check_complete", which is used to determine if a type is invokable, which in turn is used to define result_of, there sits a sizeof check, which is illegal on function types.
Hopefully someone more knowledgeable on the support model of the llvm source can chime in. Do we need to upgrade our libc++ implementation, or is this commit intended to work with this version?

[sammccall]

Sorry about the delay.
This is a supported configuration: apple clang toolchain (including libc++) is supposed to work back to 6.0. I'll get this fixed today.

https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library

[sammccall]

375f7a416031b3a011a5a88ba5f80f5879d775ca should fix this.

[stella.stamenova]

The new test fails on Windows when we compile Debug (instead of Release). Is that expected?

[sammccall]

No, not expected... can you provide more details?
Is this on a public LLVM buildbot I missed somehow?

If not, toolchain information, logs... I guess this is x64?
There are a few main differences between Debug and Release as I understand: optimization level, -DNDEBUG, debug info. Do you know which it is (e.g. does Release + -DLLVM_ENABLE_ASSERTIONS=On also fail?)

[stella.stamenova]

In D88977#2355654, @sammccall wrote:

No, not expected... can you provide more details?
Is this on a public LLVM buildbot I missed somehow?

It is not a public buildbot - I don't think there is a public buildbot that builds Debug on Windows.

If not, toolchain information, logs... I guess this is x64?
There are a few main differences between Debug and Release as I understand: optimization level, -DNDEBUG, debug info. Do you know which it is (e.g. does Release + -DLLVM_ENABLE_ASSERTIONS=On also fail?)

We run a few different permutations locally, including Release (+/- assertions) and Debug (+/- assertions). Of those, the test fails on Debug regardless of assertions and passes on Release regardless of assertions. The setup is essentially a copy of the Windows LLDB bot (http://lab.llvm.org:8011/#/workers/55), but building with VS 2017 directly rather than Ninja.

Here's the log from the failure:

         FAIL: LLVM-Unit :: ADT/Debug/ADTTests.exe/FunctionRefTest.SFINAE (518 of 40375)
         ******************** TEST 'LLVM-Unit :: ADT/Debug/ADTTests.exe/FunctionRefTest.SFINAE' FAILED ********************
         Note: Google Test filter = FunctionRefTest.SFINAE
         
         [==========] Running 1 test from 1 test case.
         
         [----------] Global test environment set-up.
         
         [----------] 1 test from FunctionRefTest
         
         [ RUN      ] FunctionRefTest.SFINAE
         
##[error]llvm\unittests\ADT\FunctionRefTest.cpp(57,0): Error : Expected: "not a function"
     1>E:\agent\_work\20\s\llvm\unittests\ADT\FunctionRefTest.cpp(57): error : Expected: "not a function" [e:\agent\_work\20\b\llvm\test\check-llvm.vcxproj]
         
               Which is: 00007FF66D93C920
         
         To be equal to: returns("boo!")
         
               Which is: 00007FF66D93C898
         
##[error]llvm\unittests\ADT\FunctionRefTest.cpp(58,0): Error : Expected: "number"
     1>E:\agent\_work\20\s\llvm\unittests\ADT\FunctionRefTest.cpp(58): error : Expected: "number" [e:\agent\_work\20\b\llvm\test\check-llvm.vcxproj]
         
               Which is: 00007FF66D93C994
         
         To be equal to: returns([] { return 42; })
         
               Which is: 00007FF66D93C8A8

##[error]llvm\unittests\ADT\FunctionRefTest.cpp(59,0): Error : Expected: "string"
     1>E:\agent\_work\20\s\llvm\unittests\ADT\FunctionRefTest.cpp(59): error : Expected: "string" [e:\agent\_work\20\b\llvm\test\check-llvm.vcxproj]
         
               Which is: 00007FF66D93CA0C
         
         To be equal to: returns([] { return "hello"; })
         
               Which is: 00007FF66D93C8B0
         
         [  FAILED  ] FunctionRefTest.SFINAE (1 ms)

[sammccall]

Oh wow, I think those are different pointers to the same string, and MSVC+Debug is the only configuration that doesn't fold them together.

So this is just a dumb bug in the test. I just pushed 1a1aad9156407bc891e2738e9877c03bd594e67f which should fix it, please let me know if it doesn't!

[stella.stamenova]

In D88977#2356948, @sammccall wrote:

Oh wow, I think those are different pointers to the same string, and MSVC+Debug is the only configuration that doesn't fold them together.

So this is just a dumb bug in the test. I just pushed 1a1aad9156407bc891e2738e9877c03bd594e67f which should fix it, please let me know if it doesn't!

Thanks! All good now.

llvm/include/llvm/ADT/STLExtras.h

 
 public:
   function_ref() = default;
   function_ref(std::nullptr_t) {}
 
   template <typename Callable>
   function_ref(
       Callable &&callable,
+      // This is not the copy-constructor.
       std::enable_if_t<
           !std::is_same<std::remove_cv_t<std::remove_reference_t<Callable>>,
-                        function_ref>::value> * = nullptr)
+                        function_ref>::value> * = nullptr,
+      // Functor must be callable and return a suitable type.
+      std::enable_if_t<std::is_void<Ret>::value ||
+                       std::is_convertible<
+                           std::result_of_t<Callable(Params...)>, Ret>::value>
+          * = nullptr)
       : callback(callback_fn<typename std::remove_reference<Callable>::type>),
         callable(reinterpret_cast<intptr_t>(&callable)) {}
 
   Ret operator()(Params ...params) const {
     return callback(callable, std::forward<Params>(params)...);
   }
 
   explicit operator bool() const { return callback; }

llvm/lib/AsmParser/LLParser.h

namespace llvm {
 
   public:
     LLParser(StringRef F, SourceMgr &SM, SMDiagnostic &Err, Module *M,
              ModuleSummaryIndex *Index, LLVMContext &Context,
              SlotMapping *Slots = nullptr)
         : Context(Context), Lex(F, SM, Err, Context), M(M), Index(Index),
           Slots(Slots), BlockAddressPFS(nullptr) {}
     bool Run(
-        bool UpgradeDebugInfo,
-        DataLayoutCallbackTy DataLayoutCallback = [](Module *) {});
+        bool UpgradeDebugInfo, DataLayoutCallbackTy DataLayoutCallback =
+                                   [](StringRef) { return None; });
 
     bool parseStandaloneConstantValue(Constant *&C, const SlotMapping *Slots);
 
     bool parseTypeAtBeginning(Type *&Ty, unsigned &Read,
                               const SlotMapping *Slots);
 
     LLVMContext &getContext() { return Context; }
 

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

public:
   Error materialize(GlobalValue *GV) override;
   Error materializeModule() override;
   std::vector<StructType *> getIdentifiedStructTypes() const override;
 
   /// Main interface to parsing a bitcode buffer.
   /// \returns true if an error occurred.
   Error parseBitcodeInto(
       Module *M, bool ShouldLazyLoadMetadata = false, bool IsImporting = false,
-      DataLayoutCallbackTy DataLayoutCallback = [](std::string) {
-        return None;
-      });
+      DataLayoutCallbackTy DataLayoutCallback = [](StringRef) { return None; });
 
   static uint64_t decodeSignRotatedValue(uint64_t V);
 
   /// Materialize any deferred Metadata block.
   Error materializeMetadata() override;
 
   void setStripDebugInfo() override;
 

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());
 }
 
+// Test that overloads on function_refs are resolved as expected.
+const char *returns(StringRef) { return "not a function"; }
+const char *returns(function_ref<double()> F) { return "number"; }
+const char *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