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

Enable build of dynamic linked library #76

Closed
wants to merge 2 commits into from

Conversation

bkmgit
Copy link
Contributor

@bkmgit bkmgit commented Nov 22, 2022

Partially addressing #74

@bkmgit bkmgit marked this pull request as ready for review November 25, 2022 17:43
@bkmgit
Copy link
Contributor Author

bkmgit commented Nov 25, 2022

@kawakami-k Initial review would be helpful. Can also:

  1. Update example Makefiles for samples and testing
  2. migrate to CMake

@kawakami-k
Copy link
Collaborator

Hi, @bkmgit
Thank you for the patch! But I think it is better to provide Xbyak_aarch64 only as a static library.

Xbyak_aarch64 is a very low-level library that manipulates the instruction-level code. Even if a single instruction generated by Xbyak_aarch64 is different from the one expected by applications linking with Xbyak_aarch64, it can causes serious bugs. To avoid this situation, I think it is safe to provide Xbyak_aarch64 as a static library.

@bkmgit
Copy link
Contributor Author

bkmgit commented Nov 27, 2022

@kawakami-k Thanks for your feedback.

Fedora prefers to ship dynamically linked libraries where possible so that improvements can be easily incorporated in end user applications without recompilation of the application, even though there is a performance overhead. Can carry the patch to make a dynamic library, but if it is possible to incorporate it and include testing of the dynamic library it would be very helpful. Happy to refactor the Makefile to make it cleaner.

The original xbyak is header only, and so similar to a static library, but can look at updating this as well to make a dynamically linked library. Many of the other JIT compilers are rather large, so this is very helpful in constrained environments.

@kawakami-k
Copy link
Collaborator

@bkmgit, @herumi
It is up to the application to choose whether to use static or dynamic linking, but it maybe a good idea to prepare a CMakeLists.txt file for both dynamic and static linking. What do you think, @herumi ?

@herumi
Copy link
Collaborator

herumi commented Nov 28, 2022

@bkmgit , @kawakami-k
Thank you for the proposal and discussion.
I also think optional is better for dynamic libraries.
The header of xbyak_aarch64 contains a huge class having many instruction member functions.
So most changes may not be binary ABI compatible, and header files must be maintained for each version.
The cost to manage will be complicated.

@kawakami-k
Copy link
Collaborator

kawakami-k commented Nov 30, 2022

@herumi
Can we abandon Makefile and switch to CMakeLists.txt? The existing CMakeLists.txt file seems to be working correctly for both Linux and Windows. I'll test CMakeLists.txt on Apple Silicon Mac. If it works on Mac, can we delete Makefile?

@bkmgit
If we can switch to CMakeLists.txt, it's good to add the knob of generating dynamic library to CMakeLists.txt.

@herumi
Copy link
Collaborator

herumi commented Nov 30, 2022

@kawakami-k
Okay, I'll add the option to make a dynamic library to CMakeLists.txt.

@bkmgit
Copy link
Contributor Author

bkmgit commented Nov 30, 2022

Thanks. Will close this pull request then. Using CMake is ok.

@bkmgit bkmgit closed this Nov 30, 2022
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

Successfully merging this pull request may close these issues.

3 participants