WG21-SG14 / SG14

A library for Study Group 14 of Working Group 21 (C++)
501 stars 53 forks source link

[inplace_function] usage at shared library boundaries #170

Open kamrann opened 4 years ago

kamrann commented 4 years ago

Seeing as inplace_function is standard layout and cannot allocate, I assumed it would be safe (dependent on what is put in it, of course) to be used across shared library interfaces. Apparently this is not the case.

The mechanism used to identify an empty inplace_function relies on taking the address of an inline constexpr global. Perhaps this is specific to MSVC (obviously this is not specified by the standard), but in practice I'm seeing a default constructed function converting to false in one module, but to true (callable) in another module due to differing addresses of empty_vtable.

Is usage across shared libraries a bad idea in general and intentionally not supported? If not, is this something that can be easily addressed?

Quuxplusone commented 4 years ago

Hi @kamrann, As you already know, "standard C++" doesn't say anything about shared libraries, so the short answer is that anything involving shared libraries in general won't be "supported," especially by a header-only library like this one.

I'd be very sympathetic if you could work up a specific test case that reproduces the issue on TravisCI; then we could discuss whether that specific test case is worth supporting, and how to support it, and so on. (And we could run the test case on TravisCI to prove that we'd fixed it, forever.)

Off the top of my head, I think the only ways to fix the kind of issue you're probably seeing would be either

kamrann commented 4 years ago

Thanks for the quick response @Quuxplusone. Yep that makes sense, I guess 'supported' was the wrong word. Essentially, not having much experience with shared libraries, I'm open to being told that trying to use the class in this way is just a bad idea for more general reasons. But if that's not necessarily the case, then obviously it would be nice if the library was flexible enough to make it feasible in practice, without any guarantees.

I'd be happy to try to create a reproduction test case, though haven't used TravisCI before so any quick pointers would be helpful - would the idea be to fork the SG14 repo, then add to the tests? If so, I guess the nature of the issue would mean it would require some Cmake changes and not really drop nicely into the existing unit test setup. Happy to have a go, just want to check so I'm not starting down the wrong path.

Quuxplusone commented 4 years ago

would the idea be to fork the SG14 repo, then add to the tests?

Yes, the GitHub model is fork-based:

I guess the nature of the issue would mean it would require some Cmake changes and not really drop nicely into the existing unit test setup.

Probably correct. I don't know that much about the CMake stuff; IIRC it was @p-groarke who set that all up. At first, you might skip anything to do with CMake, write your new test as a simple shell script, and just modify the script: line in .travis.yml to run your new test directly. Hopefully someone could help you integrate it better once you get to the pull-request stage.

kamrann commented 4 years ago

Integration didn't require too heavy changes in the end. I've added the tests to this branch in my fork: https://github.com/kamrann/SG14/tree/inplace-shared-lib Since at this point the change is just new failing tests I assume there is no point in a PR, but if opening one makes it easier for you to try things, just let me know.

I added a few simple tests of using inplace_function across a shared library boundary, but the very first one demonstrate the issue, triggering an assert on Windows. Travis CI seems to indicate that this isn't a problem on Linux as all tests pass there. I actually struggle to understand how it works - the more I think about it, the more it makes sense that separate images have their own version of an inline static variable.

Anyway as to your proposed solutions, yes the first one would be the obvious one, perhaps that could be made a compilation conditional? Something along these lines might be possible and minimize added ugliness:

#ifndef NO_INLINE_STATIC

[existing empty_vtable definition]
template <...>
constexpr auto empty_vtable_ptr = std::addressof(empty_vtable<...>);
template <class F, class... Args>
using safe_invoke = std::invoke<F, Args...>;

#else

template <...>
constexpr auto empty_vtable_ptr = nullptr;
template <class F, class... Args>
auto safe_invoke(F f, Args... args) { if (f) return f(args); else throw...; }

#endif

I haven't looked much at the vtable implementation though, I guess there are more considerations.

The second approach of letting the client define the variable is something I've been experimenting with a bit in some of my own libraries, and I agree it's awkward, especially so in this case with it being a template. Though it would have the advantage of minimal changes to existing code.

WPMGPRoSToTeMa commented 4 years ago

@kamrann some related reading on this. I think you could try to load it manually via dlopen using some special flags like RTLD_LOCAL and/or RTLD_DEEPBIND to reproduce the issue.

Also, this global variable may be handled by STB_GNU_UNIQUE, so you could also try compiling with -fno-gnu-unique.