Rename internals #172

minad · 2019-03-07T03:11:38Z

See #164. These are only internal functions, no API or ABI changes. The renaming is done by the script. If something needs tweaking only the script should be changed and executed again.

minad · 2019-03-07T03:21:32Z

@sjaeckel You mentioned that the non-hidden symbols in the private header are exposed and are thus available to ld etc. This is true. However they still don't belong to neither the API nor the (stable) ABI. If some library users deliberately access hidden symbols (as clearly indicated by the private header), they must assume that those symbols vanish at some point.

I would argue that this change improves the API/ABI situation since it makes clear that the ld-exposed symbols with s_ prefix are private, while still being visible.

sjaeckel · 2019-03-07T08:38:17Z

This very much collides with 'Such things need to get announced early enough and loud enough and at least two releases in advance.' [1] and therefore I'm not sure what I should do with it now.

Also I'm not sure if we shouldn't first make a plan where we want to go, especially in regards to [2], [3] resp. [4], [5] and potentially more...

[1] #164 (comment)
[2] #160 (comment)
[3] #164 (comment)
[4] #155
[5] libtom/tomsfastmath#10

minad · 2019-03-07T10:29:21Z

Sorry for repeating myself: Why do you need to announce a change of internals? The possibility of change is implied by the fact that these are marked private.

See for example
http://www.ros.org/reps/rep-0009.html
which explicity allows removing/adding static privates. Well its c++ but it somehow transfers.

sjaeckel · 2019-03-07T11:29:49Z

which explicity allows removing/adding static privates

but the functions you renamed are not static

BTW regarding the 'two releases in advance': I count minor releases, so I don't see this only changing in 3.0.0

how about adding something like this for all renamed functions?

diff --git a/bn_s_mp_exptmod_fast.c b/bn_s_mp_exptmod_fast.c
index c1b6da2..299ed40 100644
--- a/bn_s_mp_exptmod_fast.c
+++ b/bn_s_mp_exptmod_fast.c
@@ -313,2 +313,7 @@ LBL_M:
 }
+
+int mp_exptmod_fast(const mp_int *G, const mp_int *X, const mp_int *P, mp_int *Y, int redmode)
+{
+   return s_mp_exptmod_fast(G, X, P, Y, redmode);
+}
 #endif
diff --git a/tommath_private.h b/tommath_private.h
index f651984..917d080 100644
--- a/tommath_private.h
+++ b/tommath_private.h
@@ -17,2 +17,18 @@
 
+#if defined(__GNUC__) && (__GNUC__ * 100 + __GNUC_MINOR__ >= 301)
+   #define LTM_DEPRECATED(x) __attribute__((deprecated(#x " replaces this")))
+#elif defined(_MSC_VER) && _MSC_VER >= 1500
+   /* supported since Visual Studio 2008 */
+   #define LTM_DEPRECATED(x) __declspec(deprecated(#x " replaces this"))
+#else
+   #define LTM_DEPRECATED
+#endif
+
+/* ToDo: This needs some more work */
+#if __GNUC__ >= 4
+   #define LTM_PRIVATE  __attribute__ ((visibility ("hidden")))
+#else
+   #define LTM_PRIVATE
+#endif
+
 #ifndef MIN
@@ -75,3 +91,3 @@ int s_mp_invmod_slow(const mp_int *a, const mp_int *b, mp_int *c);
 int s_mp_montgomery_reduce_fast(mp_int *x, const mp_int *n, mp_digit rho);
-int s_mp_exptmod_fast(const mp_int *G, const mp_int *X, const mp_int *P, mp_int *Y, int redmode);
+LTM_PRIVATE int s_mp_exptmod_fast(const mp_int *G, const mp_int *X, const mp_int *P, mp_int *Y, int redmode);
 int s_mp_exptmod(const mp_int *G, const mp_int *X, const mp_int *P, mp_int *Y, int redmode);
@@ -79,2 +95,8 @@ void s_mp_reverse(unsigned char *s, int len);
 
+/* Soon to be deprecated functions
+ * These functions will be removed in v2.0.0.
+ * Please make sure to use the appropriate API functions instead of these ones.
+ */
+LTM_DEPRECATED(mp_exptmod) int mp_exptmod_fast(const mp_int *G, const mp_int *X, const mp_int *P, mp_int *Y, int redmode);
+
 extern const char *const mp_s_rmap;

like that we don't break existing stuff but tell the users that it will break in the future

sure, if they stick with <1.2.0 until 2.0.0 is out they won't notice and stuff will break, but that's not something we can solve

This definitely needs some more work, but we shouldn't just remove stuff that was there forever.

minad · 2019-03-07T11:33:34Z

I recommend the following plan:

[1, 3]: Rename internals #172 can be merged as is since there is no API change
[2]: made set_double arch/mem-layout independent #160 floating point enhancements should be merged since it arguably improves portability. There are no downsides except complexity. Also no API problems.
[4]: Compilation error on Debian x32 arch #154 should not be merged yet since it is unclear what the reason for the linking error is. Is there some explanation somewhere? I find the patch a bit unsatisfying. We could explore a possibility to avoid introducing these accessor functions, as I suggested before.
[5]: What does fastmath have to do with this library? Are they going to be merged at some point? If this is the case, this is kind of a longshot and should not impede making changes to the libraries in the meantime.

minad · 2019-03-07T11:34:55Z

@sjaeckel Oh, I just saw you commented. You are wrong about "but the functions you renamed are not static". In C++ private static functions are still linker visible as in our case. Our case with private functions in tommath_private.h transfers exactly.

Edit: Note that in the ROS guide they mean "class static functions". Sorry for the confusion, shouldn't have linked to that. Therere are probably other ABI guides specific to C.

minad · 2019-03-07T11:43:25Z

Concerning your proposal:

We should probably add LTM_PRIVATE macros, but this is separate work from this PR.
I would not add these deprecated wrappers, since they are unneeded and we are not doing an API/ABI change here. This PR is about reducing cruft, not introducing more of it.
If you want to go through with this using versioning, we could remove the deprecate wrappers in 2.2?

But I don't see a reason for going through the versioning here. And I have yet to see a good argument. Please let you convince otherwise, even if you and @czurnieden's reaction was initially critical to the change. I argue that you are unnecessarily careful. libtommath already has a good separation between private and public and we can use that.

sjaeckel · 2019-03-07T12:25:20Z

Well TBH I don't care about C++ semantics and how whatnot translates in theory to this project.

If we remove these symbols it will mean

an ABI bump
breakage if someone uses the symbols

Regarding the ABI bump:
Yeah sure, in itself it's no big deal, but

I don't want to end up with ABI version 42 just because... [1] fits here pretty well
I don't know about most of the distro guidelines, but if we merge this now as is I don't see an effortless update of ltm in e.g. debian until bullseye. Sure someone could start backporting but we were just in a state where most distros don't need any bigger patches to package this library ... (@dod38fr please correct me if I'm wrong)

[1] https://semver.org/#if-even-the-tiniest-backwards-incompatible-changes-to-the-public-api-require-a-major-version-bump-wont-i-end-up-at-version-4200-very-rapidly

3\. If you want to go through with this using versioning, we could remove the deprecate wrappers in 2.2?

in 2.0.0

I recommend the following plan:
* [1, 3]: #172 can be merged as is since there is no API change

see above

* [2]: #160 floating point enhancements should be merged since it arguably improves portability. There are no downsides except complexity. Also no API problems.

ACK

* [4]: #154 should not be merged yet since it is unclear what the reason for the linking error is. Is there some explanation somewhere? I find the patch a bit unsatisfying. We could explore a possibility to avoid introducing these accessor functions, as I suggested before.

Yes, this needs some work

* [5]: What does fastmath have to do with this library? Are they going to be merged at some point? If this is the case, this is kind of a longshot and should not impede making changes to the libraries in the meantime.

Yes, the original idea was to merge ltm and tfm. To prevent further drifting apart it should've been done yesterday, but yeah...

minad · 2019-03-07T13:16:05Z

Ok, I stop arguing here since I disagree with your definition of the public ABI. Note that this PR doesn't have any urgency and can be easily merged at some point in time when you see fit in your versioning scheme by just executing the rename.sh script again.

It seems on the other points of the plan we agree.

Yes, the original idea was to merge ltm and tfm. To prevent further drifting apart it should've been done yesterday, but yeah...

When I started using ltm, I also checked fastmath but it seemed to have a wildly different API and a different purpose using fixed size integers. (Maybe I have to check again). Given that there are a few libraries providing arbitrarily sized integers (ltm, gmp, etc), I think fastmath has its own niche and the two libraries should not be merged.

sjaeckel · 2019-03-07T13:42:30Z

Ok, I stop arguing here since I disagree with your definition of the public ABI.

there's nothing to argue about, it's not me who defined this http://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html

czurnieden · 2019-03-07T23:33:18Z

[4]: #154 should not be merged yet since it is unclear what the reason for the linking error is.

The reason for the linking error lies in the intricacies of the x64-32 ABI. Basically long and the pointers are only 32 bit large. That causes trouble with exporting (internal) global variables/functions with shared libraries and PIC relocation. I knew the error from (bad) assembler before but not from C hence my initial befuddlement. Short description here:
https://stackoverflow.com/questions/48369771/relocation-r-x86-64-pc32-against-symbol-when-calling-function-from-inline-assemb

Is there some explanation somewhere?

Not really. At least no single point of information. I can share some links, but I found none for that exact problem. I could ask at stackoverflow? With a bit of luck we can get somebody who is good at explaining?

I think I already had the following link, hadn't I?
https://bugs.chromium.org/p/webm/issues/detail?id=46

A general link:
http://www.akkadia.org/drepper/dsohowto.pdf

Some thoughts about the future of x64_32
https://lwn.net/Articles/774734/

I find the patch a bit unsatisfying.

The patch itself is correct although a bit too specific for my taste--needs GCC>4 (which is really old, admitted) or Intel's ICC, and ELF, and an x86-64 (ARM has something similar to the x86 specific x64_32 with their arm64 but with a different ABI, not yet tested).

Another two links showing how others do it:

https://docs.microsoft.com/en-us/cpp/build/exporting-from-a-dll-using-declspec-dllexport?view=vs-2017
https://www.ibm.com/developerworks/aix/library/au-aix-symbol-visibility/index.html

We could explore a possibility to avoid introducing these accessor functions, as I suggested before.

It's just for x64_32 wouldn't invest much work into it. You can test for the x64_32 ABI and exclude the whole mess for the rest with the preprocessor. I'm sure it can be done more elegantly as I did it, though.

minad · 2019-03-07T23:55:52Z

The reason for the linking error lies in the intricacies of the x64-32 ABI. Basically long and the pointers are only 32 bit large. That causes trouble with exporting (internal) global variables/functions with shared libraries and PIC relocation. I knew the error from (bad) assembler before but not from C hence my initial befuddlement. Short description here:
https://stackoverflow.com/questions/48369771/relocation-r-x86-64-pc32-against-symbol-when-calling-function-from-inline-assemb

Is the x64-32 abi broken in this sense? I have no idea what the technical reason is for this. In theory x64-32 should not be much different from x32 + additional 64bit registers available.

I will check out your links... Thank you for collecting them.

Not really. At least no single point of information. I can share some links, but I found none for that exact problem. I could ask at stackoverflow? With a bit of luck we can get somebody who is good at explaining?

Maybe it could help. Would you like to ask? I won't do it since I don't have an account there and there are just to many janitors around over there...

Some thoughts about the future of x64_32
https://lwn.net/Articles/774734/

I've read that one before. And I hope that it gets dropped since it is a bad idea. For this reason I would also not put much effort in a fix.

It's just for x64_32 wouldn't invest much work into it. You can test for the x64_32 ABI and exclude the whole mess for the rest with the preprocessor. I'm sure it can be done more elegantly as I did it, though.

I am pretty sure it can be done more elegantly. But as you said, we shouldn't put effort in it. Just drop the patch and wontfix?

minad · 2019-03-08T00:13:10Z

What I don't get or assuming wrongly - in this x64-32 abi, is the virtual address space not artificially restricted such that only 32 bits are used? Are shared objects still placed arbitrarily within the 48 bit address space for ASLR etc? Then these relocation errors can occur obviously when the address does not fit within the 32 bit R_X86_64_PC32 relocation.

minad · 2019-03-08T00:15:09Z

See also this link
http://eli.thegreenplace.net/2011/11/11/position-independent-code-pic-in-shared-libraries-on-x64/

minad · 2019-04-09T10:24:36Z

@sjaeckel rebased and added deprecated functions. now it is 100% abi/api compatible

sjaeckel

Besides those comments 👍

sjaeckel · 2019-04-09T10:26:39Z

tommath_private.h

 /* define heap macros */
-#ifndef XMALLOC
+#ifndef MP_MALLOC


I like the idea, but I guess we have to keep these X-versions around until 2.0.0

No. You just added CALLOC and removed it etc etc. This is internal API not even ABI. And these are only macros.

Uhm, you're right... we already broke this part in #192

The removal of XCALLOC doesn't break, since if someone overrides those macros with their own version it's simply not called anymore, whereas with #192 the function signatures changed and now even the name.

IMO we have to revert #192 (and part of #209) as well.

This is not an issue. Please! This is a private header!

I added the deprecated functions to keep the private ABI stable (which is already unnecessary in my opinion). Changes to tommath_private.h are even more minor. Let's change this to move forward :)

regarding the deprecated API functions we're forced to keep them, as otherwise we'd have to bump the ABI

@sjaeckel

am now tending towards accepting as proposed

A clean-up is necessary, no question, and we have to start at one point in time and (code)space so why not here and now?

at least a 1.2.0 release which announces the deprecation of all this stuff in 2.0.0

Sounds good to me. And gives enough time to "round the edges".

@sjaeckel Did you decide to merge this? If you do, ping me and I will update this PR again.

thanks, yes please rebase

btw. would you be fine with me rebasing your commits if I sign them?

Yes, you can also do the rebase without signing, that's ok. You are the committer.

demo/timing.c

tommath_private.h

changes.txt

minad · 2019-04-09T10:48:16Z

Fixed the MAX/MIN renaming issues.

czurnieden

(This UI is kiilin gme, if this comment comes up multiple times I'll blame MS ;-) )

@sjaeckel

am now tending towards accepting as proposed

A clean-up is necessary, no question, and we have to start at one point in time and (code)space so why not here and now?

at least a 1.2.0 release which announces the deprecation of all this stuff in 2.0.0

Sounds good to me. And gives enough time to "round the edges".

czurnieden · 2019-04-11T16:42:17Z

tommath_private.h

 /* define heap macros */
-#ifndef XMALLOC
+#ifndef MP_MALLOC


@sjaeckel

am now tending towards accepting as proposed

A clean-up is necessary, no question, and we have to start at one point in time and (code)space so why not here and now?

at least a 1.2.0 release which announces the deprecation of all this stuff in 2.0.0

Sounds good to me. And gives enough time to "round the edges".

minad · 2019-04-12T12:52:30Z

@sjaeckel rebased, please merge!

sjaeckel · 2019-04-12T12:53:41Z

@sjaeckel rebased, please merge!

but not on develop

minad · 2019-04-12T12:57:18Z

@sjaeckel Right I confused a few branches. Now it should be ok.

sjaeckel · 2019-04-12T12:58:56Z

@sjaeckel Right I confused a few branches. Now it should be ok.

I'll merge as soon as travis is done

buggywhip · 2019-04-12T19:47:36Z

On 9Apr, 2019, at 4:17 AM, Steffen Jaeckel ***@***.***> wrote: But renaming something that was there for ages, only for the sake of consistency, without any other gain, I don't consider as a such important improvement.

Agree

On 9Apr, 2019, at 4:40 AM, Steffen Jaeckel ***@***.***> wrote: ... This change breaks users' implementations if they rely on the fact that they can override XMALLOC etc by defining XMALLOC at compile time of the library. This entire stuff has been public for ages, then it has been "private" for some time, now we should make it (as possible) really private but this just needs some time (and at least a 1.2.0 release which announces the deprecation of all this stuff in 2.0.0).

I concur. Some consistency across LTM and LTC is desirable, especially for otherwise-common low-level utilities like the MALLOC family. While I personally don't rely on XMALLOC in this way, I easily could when my situation warranted. That said, I suppose I could support both XMALLOC and MP_MALLOC in tommath_private.h. Then, after a stronger case can be made, perhaps deprecate XMALLOC for later removal?

minad · 2019-04-12T20:02:32Z

@buggywhip I don't think it is easy to support both macros. You have to check if one or the other is defined etc. Then you have to add some deprecated pragmas as in #223. This is not worth it. People who overwrite the macros of the private API, know what they are doing and they will accept if they change (I am overwriting them for example).

sjaeckel · 2019-04-12T20:58:25Z

Some consistency across LTM and LTC is desirable, especially for otherwise-common low-level utilities like the MALLOC family.

I like that thought! probably it'd be better to call it LT_MALLOC etc. and then also rename it in libtomcrypt

People who overwrite the macros of the private API, know what they are doing and they will accept if they change

I think so too, even if they're already there for a long time people who overwrite them will (hopefully) know what they do and well then they have to adapt their sources...

minad · 2019-04-12T21:01:40Z

I like that thought! probably it'd be better to call it LT_MALLOC etc. and then also rename it in libtomcrypt

I am not very fond of it. I see tommath and crypt as distinct. Right now I am also only using tommath (maybe also crypt in the future). I would prefer if we can unify all the tommath macros and functions under the mp_* prefix for consistency. I think consistency within the library is more important than consistency between two distinct libraries.

minad · 2019-04-12T21:03:27Z

We could however rename everything to ltm_ and ltc_ and then you can use the lt_ macro, but I think that would really classify as needless renaming. 😆

sjaeckel · 2019-04-12T21:06:25Z

ah I don't know how to name it, but for sure we have to change the signature of the XMALLOC class of macros in ltc as well to the one that is used here...

minad · 2019-04-12T21:07:50Z

I haven't looked at ltc but I would just use the canonical namespace prefix if there is one?

sjaeckel · 2019-04-12T21:09:27Z

they're called the exact same as they were called here before XMALLOC XFREE etc.

ltm and ltc were once one library and then they got separated... ;-)

buggywhip · 2019-04-12T21:12:49Z

On 12Apr, 2019, at 2:06 PM, Steffen Jaeckel ***@***.***> wrote: ah I don't know how to name it

Consistency does not have to be sameness. You can have consistency at the paradigm level, and perhaps that is all that is needed here.

minad mentioned this pull request Mar 7, 2019

Cleanup naming of internal functions? #164

Closed

sjaeckel mentioned this pull request Apr 8, 2019

do not expose mp_balance_mul, add MP_ENABLED feature detection #213

Closed

minad requested a review from sjaeckel April 9, 2019 10:24

sjaeckel reviewed Apr 9, 2019

View reviewed changes

sjaeckel mentioned this pull request Apr 10, 2019

Better interoperability between MSVC and mingw-w64, 2 #216

Closed

czurnieden approved these changes Apr 11, 2019

View reviewed changes

minad added 6 commits April 12, 2019 14:55

rename macros in tommath_private to use MP_* prefix

c4fb224

remove s_mp_mul which is only used once

e09a5c9

add MP_DEPRECATED macro

df8fcb0

move mp_balance_mul to tommath_private.h

87292ed

add rename script

7469e85

apply rename

fbfcb66

use MP_IS_ZERO in bn_mp_ilogb

8bf2eaa

sjaeckel merged commit a752242 into libtom:develop Apr 12, 2019

minad mentioned this pull request May 21, 2019

Get rid of BN prefix #284

Merged

Rename internals #172

Rename internals #172

Uh oh!

Conversation

minad commented Mar 7, 2019

Uh oh!

minad commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjaeckel commented Mar 7, 2019

Uh oh!

minad commented Mar 7, 2019

Uh oh!

sjaeckel commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

minad commented Mar 7, 2019

Uh oh!

minad commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

minad commented Mar 7, 2019

Uh oh!

sjaeckel commented Mar 7, 2019

Uh oh!

minad commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjaeckel commented Mar 7, 2019

Uh oh!

czurnieden commented Mar 7, 2019

Uh oh!

minad commented Mar 7, 2019

Uh oh!

minad commented Mar 8, 2019

Uh oh!

minad commented Mar 8, 2019

Uh oh!

minad commented Apr 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjaeckel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minad Apr 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

minad commented Apr 9, 2019

Uh oh!

czurnieden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minad commented Apr 12, 2019

Uh oh!

sjaeckel commented Apr 12, 2019

Uh oh!

minad commented Mar 7, 2019 •

edited

Loading

sjaeckel commented Mar 7, 2019 •

edited

Loading

minad commented Mar 7, 2019 •

edited

Loading

minad commented Mar 7, 2019 •

edited

Loading

minad commented Apr 9, 2019 •

edited

Loading

minad Apr 9, 2019 •

edited

Loading

minad commented Apr 12, 2019 •

edited

Loading