Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[inplace_function] usage at shared library boundaries #170

Open
kamrann opened this issue Jan 1, 2020 · 5 comments
Open

[inplace_function] usage at shared library boundaries #170

kamrann opened this issue Jan 1, 2020 · 5 comments

Comments

@kamrann
Copy link

kamrann commented Jan 1, 2020

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
Copy link
Contributor

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

  • Replace all uses of empty_vtable with nullptr (whose representation will be the same on both sides of the shared-library boundary), and add null-checks to places like operator() and operator=. (This is what std::function usually does.) I resist this idea because it is slower and less elegant than the current empty_vtable idea.

  • Maybe, add some #if NON_HEADER_ONLY mode that the client programmer could set to make empty_vtable a non-inline variable, and then some macro to place its definition in a single object file of the client programmer's choice. But this seems quite difficult both to implement and to use.

@kamrann
Copy link
Author

kamrann commented Jan 2, 2020

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
Copy link
Contributor

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

Yes, the GitHub model is fork-based:

  • Fork WG21-SG14/SG14 to kamrann/SG14
  • git clone [email protected]:kamrann/SG14.git locally
  • git checkout -b feature-branch in your local copy; work in that feature branch; git push origin feature-branch
  • Eventually, submit a "cross-fork pull request" from kamrann/SG14/feature-branch into WG21-SG14/SG14/master

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
Copy link
Author

kamrann commented Jan 7, 2020

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
Copy link

WPMGPRoSToTeMa commented Jan 7, 2020

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants