fujitsu / xbyak_aarch64

Apache License 2.0
186 stars 39 forks source link

Enable build of dynamic linked library #76

Closed bkmgit closed 1 year ago

bkmgit commented 1 year ago

Partially addressing https://github.com/fujitsu/xbyak_aarch64/issues/74

bkmgit commented 1 year ago

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

  1. Update example Makefiles for samples and testing
  2. migrate to CMake
kawakami-k commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

bkmgit commented 1 year ago

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