Page MenuHomePhabricator

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

Authored by sammccall on Oct 7 2020, 9:10 AM.

Details

Summary

This reverts commit 281703e67ffaee8e26efef86e0df3e145477f4cb.

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

Diff Detail

Event Timeline

sammccall created this revision.Oct 7 2020, 9:10 AM
sammccall requested review of this revision.Oct 7 2020, 9:10 AM
kadircet accepted this revision.Oct 7 2020, 9:19 AM

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
This revision is now accepted and ready to land.Oct 7 2020, 9:19 AM

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.

This revision was landed with ongoing or failed builds.Oct 7 2020, 9:31 AM
This revision was automatically updated to reflect the committed changes.

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?

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

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

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?)

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)

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!

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.