gh-libtom/libtomcrypt

Watch

Fork 0

[RFC/POC] Static Math Library Binding #542

Open

pattop wants to merge 1 commit from unknown repository into develop

pattop commented

2020-07-22 01:15:37 -05:00

(Migrated from github.com)

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

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_<libname>.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_<libname>.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 commented

2020-08-13 18:33:44 -05:00

(Migrated from github.com)

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?

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 commented

2020-08-16 19:19:48 -05:00

(Migrated from github.com)

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

levitte commented

2024-08-20 05:44:20 -05:00

(Migrated from github.com)

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?

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?

This pull request is broken due to missing fork information.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 'unknown repository:unknown repository'

git switch 'unknown repository'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'develop'

git merge --no-ff 'unknown repository'

git switch 'unknown repository'

git rebase 'develop'

git switch 'develop'

git merge --ff-only 'unknown repository'

git switch 'unknown repository'

git rebase 'develop'

git switch 'develop'

git merge --no-ff 'unknown repository'

git switch 'develop'

git merge --squash 'unknown repository'