ned14 / llfio

P1031 low level file i/o and filesystem library for the C++ standard
https://ned14.github.io/llfio/
Other
874 stars 45 forks source link

MSVC compiler regression #83

Closed BurningEnlightenment closed 2 years ago

BurningEnlightenment commented 3 years ago

Since Visual Studio 16.11.4 (verified for MSVC [19.29.30133; 19.29.30136]) compiling the sl target fails:

C:\PROGRA~2\MICROS~1\2019\ENTERP~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\cl.exe
    /TP

    -DLLFIO_DYNAMIC_THREAD_POOL_GROUP_USING_GCD=0
    -DLLFIO_ENABLE_TEST_IO_MULTIPLEXERS=1
    -DLLFIO_EXPERIMENTAL_STATUS_CODE=1
    -DLLFIO_INCLUDE_STORAGE_PROFILE=1
    -DLLFIO_SOURCE=1
    -DLLFIO_STATIC_LINK=1
    -DQUICKCPPLIB_ENABLE_VALGRIND=1
    -D_WIN32_WINNT=0x601

    /nologo
    /DWIN32
    /D_WINDOWS
    /W3
    /utf-8
    /MP
    /utf-8
    /Zc:__cplusplus
    /std:c++latest
    -ID:/devel/source/vcpkg/installed/x64-windows-ltcg-static/include
    /D_DEBUG
    /MTd
    /Z7
    /Ob0
    /Od
    /RTC1
    /EHsc
    /MDd
    /W4
    /showIncludes
    /FoCMakeFiles\llfio_sl.dir\src\llfio.cpp.obj
    /FdCMakeFiles\llfio_sl.dir\llfio_sl.pdb
    /FS
    -c
    D:\devel\source\vcpkg\buildtrees\llfio\src\d3799d14b8-90055f6d12.clean\src\llfio.cpp

with the following error message

cl : Command line warning D9025 : overriding '/MTd' with '/MDd'
cl : Command line warning D9025 : overriding '/W3' with '/W4'
D:\devel\source\vcpkg\buildtrees\llfio\src\d3799d14b8-90055f6d12.clean\include\llfio\v2.0\algorithm\handle_adapter\../../map_handle.hpp(954): error C2440: 'return': cannot convert from 'std::shared_ptr<llfio_v2_7238591f::detail::map_handle_allocate_registered_buffer::<lambda_1>::<lambda_invoker_cdecl>::registered_buffer_type_indirect> (__cdecl *)(llfio_v2_7238591f::map_handle)' to 'std::shared_ptr<llfio_v2_7238591f::detail::map_handle_allocate_registered_buffer::<lambda_1>::()::registered_buffer_type_indirect> (__cdecl *)(llfio_v2_7238591f::map_handle)'
D:\devel\source\vcpkg\buildtrees\llfio\src\d3799d14b8-90055f6d12.clean\include\llfio\v2.0\algorithm\handle_adapter\../../map_handle.hpp(954): note: This conversion requires a reinterpret_cast, a C-style cast or function-style cast
D:\devel\source\vcpkg\buildtrees\llfio\src\d3799d14b8-90055f6d12.clean\include\llfio\v2.0\algorithm\handle_adapter\../../map_handle.hpp(954): error C2440: 'return': cannot convert from 'std::shared_ptr<llfio_v2_7238591f::detail::map_handle_allocate_registered_buffer::<lambda_1>::<lambda_invoker_vectorcall>::registered_buffer_type_indirect> (__vectorcall *)(llfio_v2_7238591f::map_handle)' to 'std::shared_ptr<llfio_v2_7238591f::detail::map_handle_allocate_registered_buffer::<lambda_1>::()::registered_buffer_type_indirect> (__vectorcall *)(llfio_v2_7238591f::map_handle)'
D:\devel\source\vcpkg\buildtrees\llfio\src\d3799d14b8-90055f6d12.clean\include\llfio\v2.0\algorithm\handle_adapter\../../map_handle.hpp(954): note: This conversion requires a reinterpret_cast, a C-style cast or function-style cast

The code in question: https://github.com/ned14/llfio/blob/707493a81a42aaab273b2e5af37e6d968c615abd/include/llfio/v2.0/map_handle.hpp#L957-L969

So it looks like they changed something related to their lambda name mangling, lambda type representation or type deduction. I worked around this by explicitly specifying the return type to be a shared_ptr to the base class:

index 89e616c..d5d61e9 100644
--- a/include/llfio/v2.0/map_handle.hpp
+++ b/include/llfio/v2.0/map_handle.hpp
@@ -939,7 +939,7 @@ namespace detail
   {
     try
     {
-      auto make_shared = [](map_handle h) {
+      auto make_shared = [](map_handle h) -> io_handle::registered_buffer_type {
         struct registered_buffer_type_indirect : io_multiplexer::_registered_buffer_type
         {
           map_handle h;

I'm aware of the cl : Command line warning D9025 : overriding '/MTd' with '/MDd' which is caused by a bad interaction between vcpkg and quickcpplib. I'll open a seperate issue for that.

ned14 commented 2 years ago

Sorry it's been so long for me to get to this. I haven't been well last few months, and to be honest I had completely missed the email saying you'd submitted this. So, my apologies.

It's fixed in https://github.com/ned14/llfio/commit/bfa67375086cd63db1eb1bef64ccc9708b021eaa. Thanks for the BR!

BurningEnlightenment commented 2 years ago

Sorry it's been so long for me to get to this. I haven't been well last few months, and to be honest I had completely missed the email saying you'd submitted this. So, my apologies.

Don't worry. After all, it's OSS with no warranty, and I've also been stressed in the past and stopped replying, so you have my sympathy. (I still need to complete my reply regarding deleting immutable files).

Have you thought about "acquiring" a second trusted maintainer, such as someone from the committee or from your workplace? As an added benefit, this would also increase the project bus factor to two.

Would you mind if I updated the vcpkg ports and fixed their static variants? (This would require fixing https://github.com/ned14/quickcpplib/issues/21 first, for which I could also provide a patch set).

ned14 commented 2 years ago

Have you thought about "acquiring" a second trusted maintainer, such as someone from the committee or from your workplace? As an added benefit, this would also increase the project bus factor to two.

Are you volunteering? :)

Re: workplace, yes a large chunk of my current employer's revenue now depends heavily on LLFIO. But I am only allowed to spend work time on LLFIO if I am debugging a production showstopper. Feature work is not possible on work time.

Re: where LLFIO is going next, as you may know ASIO was not approved for entry into C++ 23, and now targets 26. I was asked informally by WG21 to come up with a proposed solution for portable secure sockets, and hopefully by the end of this Christmas break there will be a reference implementation.

LLFIO will be staying far away from the async model wars. Its byte_io_multiplexer doesn't care what async model gets eventually chosen by WG21. It can plug into ASIO just as easily as libunifex or even Qt. That said, if somebody wrote a high quality Windows RIO or Linux epoll/Linux io_uring or BSD kqueues and contributed it, I'd accept it.

Finally, after the networking PR gets merged LLFIO will probably get renamed to LLIO i.e. low level i/o. Unlike ASIO/libunifex/pretty much any C++ networking library, LLIO can do true zero copy portable networking. If you're a user who needs absolute bare metal performance, LLIO will be a game changer.

Would you mind if I updated the vcpkg ports and fixed their static variants? (This would require fixing ned14/quickcpplib#21 first, for which I could also provide a patch set).

I only just did that myself last week, but feel free to do so. If you can wait to the next Boost release in the Spring, any merged PRs would get pushed to vcpkg then at that point.

BurningEnlightenment commented 2 years ago

Are you volunteering? :)

Well, I am not opposed to that, but I am also unsure whether I have the right credentials to do that. I have little knowledge about IO kernel internals compared to you and I am still a CS student (albeit working part time for a while now). I'm mainly using llfio for the user space virtual encrypting and authenticating filesystem "driver" which I have designed and developed for my employer over the past years. (It's OSS pending publication, so I would be allowed to give you source access if you were interested.)

I was asked informally by WG21 to come up with a proposed solution for portable secure sockets, and hopefully by the end of this Christmas break there will be a reference implementation.

Does that include low level unsecure sockets? TBH I am skeptical w.r.t. standardizing a TLS API. That protocol has (as you surely know) way to many goddamn knobs which includes ever changing stuff like cipher suites. Combined with the current policy of (basically not) breaking the stdlib ABI and API I could imagine this leading to quite unfortunate situations in the future. However, it sounded like you already have a plan to address that.

Finally, after the networking PR gets merged LLFIO will probably get renamed to LLIO i.e. low level i/o. Unlike ASIO/libunifex/pretty much any C++ networking library, LLIO can do true zero copy portable networking. If you're a user who needs absolute bare metal performance, LLIO will be a game changer.

I highly appreciate efficient designs and will definitely take a look.

I only just did that myself last week, but feel free to do so. If you can wait to the next Boost release in the Spring, any merged PRs would get pushed to vcpkg then at that point.

Thank you, I missed that as I was on vacation last week. However, said vfs driver gets statically linked against the CRT, i.e. I absolutely need ned14/quickcpplib#22 to be part of the official ports before I can switch back. Which I would like to do before the whole thing goes into production next february.

ned14 commented 2 years ago

Well, I am not opposed to that, but I am also unsure whether I have the right credentials to do that. I have little knowledge about IO kernel internals compared to you and I am still a CS student (albeit working part time for a while now). I'm mainly using llfio for the user space virtual encrypting and authenticating filesystem "driver" which I have designed and developed for my employer over the past years. (It's OSS pending publication, so I would be allowed to give you source access if you were interested.)

I like how your mind works and your instincts. And I'd choose that any day over direct experience. I've invited you there for both repos. Just try to PR anything big please so it can be reviewed before merge.

I was asked informally by WG21 to come up with a proposed solution for portable secure sockets, and hopefully by the end of this Christmas break there will be a reference implementation.

Does that include low level unsecure sockets?

Indeed it does, and LLFIO now has a byte_socket_handle, and a poll().

Why must we have a low level unsecure socket? The reason is composure: OpenSSL/curl/whatever have the ability to use a third party low level socket implementation. If we want end users to use the same i/o multiplexer for secure sockets each with a different implementation, then they all must vector through the same low level socket implementation.

Also, our low level unsecure sockets can deliver true zero copy on all platforms, and as much as that isn't hugely useful when there is a crypto layer on top, it isn't zero benefit either. In particular, if you're doing a ton of memory traversals and maxing out a 40Gbit NIC over TLS, you'll gain a nice few percent plus markedly better > 99% tail latencies if you can have zero copy raw sockets at the bottom.

TBH I am skeptical w.r.t. standardizing a TLS API. That protocol has (as you surely know) way to many goddamn knobs which includes ever changing stuff like cipher suites. Combined with the current policy of (basically not) breaking the stdlib ABI and API I could imagine this leading to quite unfortunate situations in the future. However, it sounded like you already have a plan to address that.

My solution is not a good one - it hides completely from standard code what the TLS implementation is, including at ABI level. Basically you can ask for a list of secure socket providers and you can choose one. Sockets from it get delivered to you in a completely implementation erased form.

This completely detaches the crypto layer from standard C++, but it also exposes your standard C++ code to all the vagaries of weird crypto library interactions. For example, at work, we had an awful time for a month there when our S3 provider upgraded their TLS implementation and whatever the newer version did differently it caused random SSL hangs with our TLS implementation. Not easily repeatable - no - rather they'd appear randomly only at heavy load, and as soon as you looked at them the problem went away. It took a good few weeks to figure out the cause, and it was not fun at all.

That's literally what my solution will deliver to standard C++ - random unpredictable future unreliability where well tested binaries which work fine now will randomly very subtly break in the future. I did warn my WG21 colleagues of all this, but in the end the ecosystem really wants portable secure sockets, so this will be the least worst (though not good) way of delivering those, in my opinion.