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

[RFC/POC] Static Math Library Binding #542

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

[RFC/POC] Static Math Library Binding #542

wants to merge 1 commit into from

Conversation

pattop
Copy link
Collaborator

@pattop pattop commented Jul 22, 2020

Hi,

this is a very rough proof of concept to support binding math libraries statically. The primary motivation for this is building for embedded systems with LTO enabled. When using LTO static bindings allow the compiler to remove many unused code paths and thereby reduce code size.

This patch is by no means complete and serves merely as a discussion point to establish whether LibTomCrypt is open to merge support for something like this and how it might look.

The patch does build & link with static support for libtommath only. Dynamic support for all libraries should still work too.

A few things to consider:

  • mp_* #defines should probably be removed, but this would mean touching more source files as many mp_* function names collide with functions from libtommath. We would need to choose a new prefix (ltc_mp_?)
  • How should library linking be structured? The patch currently builds libtomcrypt.a (dynamic binding) and libtomcrypt_.a (static binding). A cleaner solution may be to use weak aliases in libtomcrypt.a and override them with another library to bind statically? e.g. link both libtomcrypt.a and libtomcrypt_static_.a. This is a bit more complex but avoids duplicating everything. We could also split out the dynamic binding support, but that would require current users to update their link lines.
  • The example implementation of ltm_static.c is far from ideal. Including ltm_desc.c and hooking up to its internals was just a quick way to hack this together.
  • Tests would need to be updated to include statically linked versions.

Patrick

@sjaeckel
Copy link
Member

I like the idea a lot, but we'd have to work a bit more on that!

FYI I already had a patch locally which renamed all mp_* defines into ltc_mp_* ... ;)

You also already added the missing direct invocations of ltc_mp.* as macros.

IMO we should have two separate builds of ltc, dynamic and static MPI provider. Either you build one or the other.

I have to think a bit more about how to accomplish this ... something in this area maybe?

First we add ltc_mp_ prefix to all macros, then we do something like the following?

diff --git a/src/math/ltm_desc.c b/src/math/ltm_desc.c
index e07badfa..b5dd9cb2 100644
--- a/src/math/ltm_desc.c
+++ b/src/math/ltm_desc.c
@@ -26,2 +26,10 @@ static const struct {
 
+#ifdef LTC_MPI_STATIC
+#define LTC_MPI_DECL_STATIC
+#define LTC_MPI_FN_STATIC(n) n
+#else
+#define LTC_MPI_DECL_STATIC static
+#define LTC_MPI_FN_STATIC(n) s_ ## n
+#endif
+
 /**
@@ -55,3 +63,3 @@ static int init_mpi(void **a)
 
-static int init(void **a)
+LTC_MPI_DECL_STATIC int LTC_MPI_FN_STATIC(ltc_mp_init)(void **a)
 {

naming is definitely not optimal yet and I'm not sure whether this is such a good idea but it could work ... and this could be done for all MPI providers in the same fashion ... or did I overlook something?

maybe @minad has a good idea?

@pattop
Copy link
Collaborator Author

pattop commented Aug 17, 2020

Sorry I haven't been able to dedicate any more time to this yet. I haven't forgotten about it though!

@levitte
Copy link
Collaborator

levitte commented Aug 20, 2024

Considering that all mp_* macros map to a corresponding ltc_mp_ function with this PR, couldn't all mp_* calls be replaced with ltc_mp_ calls, and these macros be dropped entirely?

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