Skip to content

Provide explicit symbols for bn_xxx_l and bn_xxx_ll functions. #321

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

Merged
merged 6 commits into from
Jul 3, 2019

Conversation

nijtmans
Copy link
Collaborator

Please, provide explicit functions for those 8 symbols, in stead of mapping those to the i/u 32/64 versions: There is no guarantee that long or long long are either 32 or 64 bits, there could be platforms where they are 128 bits. Since we have macro's now, it's trivial.

Also I adapted the .tex-file, some symbols were still not documented. Added the long long variants as well.

@nijtmans nijtmans requested review from sjaeckel, minad and karel-m June 12, 2019 11:28
@minad
Copy link
Member

minad commented Jun 12, 2019

I guess you want these symbols since you want to hide the u32/u64 symbols on msvc6 and instead use the l/ul variants. I disagree with providing different APIs on different compilers. This is not good, things should be consistent across platforms. Note that you can replace the u32/u64 functions with long/long long prototypes on msvc6 due to abi compatibility in the no-stdint-h branch. Only the typedef is not available.

I don't buy the argument regarding 128 bit platforms. If there such platforms we can instead add i128/u128 variants.

I think we should not merge this PR in its current form. We should first decide what the API should look like and then be consistent. Right now it seems different people are pulling in different directions (stdint vs nostdint for example) and this will lead nowhere and the result won't be good.

@karel-m
Copy link
Member

karel-m commented Jun 12, 2019

This is not good, things should be consistent across platforms.

Quite a strong statement and completely wrong. It is only your opinion which you are pushing deeper and deeper into libtommath during last couple of months.

Have you got a chance to look into what big guys from top bignum libraries (GMP, OpenSSL/BIGNUM guys, LibreSSL ...) do?

Look, the libtommath is a library which means that the goal is not to have libtommath as nice and shiny DEB/RPM/DLL or whatever else packages. Our goal is (or IMO should be) that libtommath is widely used in final SW products by either linking or bundling or whatever.

We are not the most frequently used bignum library; however, our past track record is pretty good and many projects use us (some say it publicly, some not). We also have some "prominent" consumers like tcl, dropbear, rakudo/perl6 that give us feedback or even some coding effort. We cannot ignore them, but we do, we make their life more complicated, what is worse in some case it looks like we complicate their lives for no obvious reasons.

By our effort to be consistent across platforms we are just narrowing down the set of supported compilers and the set of supported platforms. According to my experience with my perl bindings to libtommath + libtomcrypt there are still a lot of people living out of GNU make + gcc/clang wonderland on a pretty obscure operating systems and exotic HW.

If we will continue with this approach we will end up in our bubble totally rigorously consistent across platforms but will be losing our users, our consumers.

@minad
Copy link
Member

minad commented Jun 12, 2019

This is not good, things should be consistent across platforms.

I still stand with this statement and would like to hear why it is wrong. I haven't seen an argument from your side beside arguing what other libraries do. The libraries you mentioned and also ltm have a lot of legacy code but the decisions taken back then are not necessarily the right ones forever.

If I want to write code which is portable across platforms I need guarantees regarding the size of types. The argument for these imprecise types is usually performance and we still have mp_set which can be used to set a single platform dependent digit.

Please read through the discussion in #285 and #278. Both @sjaeckel and @czurnieden voted for using precise types.

Providing a least basic signed/unsigned long setter/getter is de facto standard.

Actually we provide mp_get/set_l/ul as macros. Therefore I am not against adding the functions @nijtmans proposed in this PR. However I am not fond of compromising in the light of goals I consider questionable, like avoiding stdint.h.

Our goal is (or IMO should be) that libtommath is widely used in final SW products by either linking or bundling or whatever.

I agree. But a design discussion should be allowed, which takes the goals into consideration. During such a discussion the maintainers decide which API to provide or not. Furthermore things are not frozen in time and there must be the possibility to improve and remove bad functions.

The design discussion should be driven by technical arguments. You refuse to argue about stdint.h. A missing stdint.h can be fixed trivially by providing your own header. This will be a much better solution than doing search and replace on the code base as in your no-stdint-h branch.

what is worse in some case it looks like we complicate their lives for no obvious reasons.

Please show me those cases. If they seem questionable I can probably tell you the reason. If I don't manage to do so on technical grounds @sjaeckel can revert the changes.

@karel-m karel-m removed their request for review June 12, 2019 20:23
@karel-m
Copy link
Member

karel-m commented Jun 12, 2019

I am tired. I am leaving.

@nijtmans nijtmans requested a review from karel-m June 13, 2019 06:57
@nijtmans
Copy link
Collaborator Author

@karel-m , Before you leave, at least put your approval here. From your remarks I conclude you approve ;-)

@minad wrote:

If I want to write code which is portable across platforms I need guarantees regarding the size of types.
The argument for these imprecise types is usually performance

Well, Tcl uses "long long" for all calculations, and everything below LLONG_MIN or above LLONG_MAX is handled over to libtommath. So, you are telling me that Tcl is not portable across platforms ??? Then you have been in another school than I have been ....

The only function that Tcl uses for setting mp_int's is mp_set_long_long, which is bad because this function expects an unsigned long long: I agree with it's deprecation. But I don't agree with using mp_set_i64 in stead: There is no guarantee that long long is 64 bits, the standard says that long long is at least 64 bits. libtommath still lacks a suitable replacement function in my view. It would mean that I am forced to change Tcl's limit checkout to hardcoded 64-bit values in stead of LLONG_MIN/LLONG_MAX. I'm not prepared to do that, and I'm sure I'm not the only one.

Please hear me! I'm not trying to remove or disable or discourage the xxx_i32/64 functions. They simply are not sufficient for everyone. And the following macro's are not what I learned at school. It's as bad as the ##w## constructs used earlier in the macro's (thank God they are gone now). Please consider putting the REAL signatures of functions in tommath.h, and implement them as documented. And provide a long long variant aside the long variant, so Tcl doesn't have to invent its own weel. That's what this PR is about IMHO.

/* get integer, set integer (long) */
#define mp_get_l(a) (sizeof (long) == 8 ? (long)mp_get_i64(a) : (long)mp_get_i32(a))
#define mp_set_l(a, b) (sizeof (long) == 8 ? mp_set_i64((a), (b)) : mp_set_i32((a), (int32_t)(b)))

/* get integer, set integer (unsigned long) */
#define mp_get_ul(a) (sizeof (long) == 8 ? (unsigned long)mp_get_u64(a) : (unsigned long)mp_get_u32(a))
#define mp_set_ul(a, b) (sizeof (long) == 8 ? mp_set_u64((a), (b)) : mp_set_u32((a), (uint32_t)(b)))
#define mp_get_magl(a) (sizeof (long) == 8 ? (unsigned long)mp_get_mag64(a) : (unsigned long)mp_get_mag32(a))

Enough techical arguments for now. ;-)

@minad
Copy link
Member

minad commented Jun 13, 2019

Ok, as I said - I am not 100% opposed to adding those functions. Therefore I added the macros for long since long varies between 32 and 64 bit on common platforms. long long is 64 bit on all platforms I am aware of. Therefore the ll variants are not necessary. Given that I am not happy with having both ll and i64 variants of the functions. They are only needed if you take the standpoint that you want to avoid stdint.h under all circumstances.

These days it is good common practice to use stdint. And I don't think we should compromise in order to support no-stdint. We already did in #311 which is ok since macro readability improved, but duplicating apis is not good.

Regarding stdint:

I think we should at this point agree on how to proceed. It could well be that we add more apis which rely on stdint.h what shall we do then? See for example #294.

Therefore please let's not work against each other and pull in different directions regarding the api. In the long run it is not feasible to have an api which is compatible with stdint and no-stdint.

@sjaeckel what do you think?

@nijtmans
Copy link
Collaborator Author

Well, in case you ask: I'm not interested in "short"/"int" variants of those functions, "i32", "i64" "long" and "long long" are more than enough for any practical use I can think of. I would approve (u)int128_t and (u)intmax_t variants, but that's stuff for another PR ;-)

@minad
Copy link
Member

minad commented Jun 13, 2019

@nijtmans my point here is mostly - how should we proceed in the light of the two incompatible opinions regarding stdint. Therefore I want to have this resolved now. Otherwise the discussion will come up again and again. It seems you want to somehow shape the api such that both an stdint and long variants are available of all functions. I don't find that satisfying. This leads to unnecessary duplication. How should we proceed in such cases in the future?

And I still don't understand your opposition in that point. I am all for reducing libc usage to keep this library minimal. But stdint is the most benign header in libc I know. It only contains very well defined things and it is small, can be easily provided by the one porting the library. Unfortunately you refuse to discuss this.

@nijtmans
Copy link
Collaborator Author

It seems you want to somehow shape the api such that both an stdint and long variants are available of all functions. I don't find that satisfying

Hm, I think you are refering to the PR about mp_expt_d and mp_n_root. Well, talking about exponents/roots: Tcl has a hardcoded limit of 28 bits here, because exponents higher than that simply cannot practially be calculated. The reason: it fits in a mp_digit. So, in this case, there is really no need to go beyond 32-bits. I would go for (unsigned) 32-bit there, and - indeed - I would prefer an "unsigned int" parameter, you most likely prefer "uint32_t". But that's a matter of taste: we should choose one or the other, we shouldn't duplicate. Using "long" would be overkill. Does that answer your assumption?

@minad minad self-requested a review June 13, 2019 14:38
Copy link
Member

@minad minad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add the missing functions and reorder the prototypes. then I think this is good.

@nijtmans nijtmans requested a review from minad June 13, 2019 15:33
@nijtmans
Copy link
Collaborator Author

ping @sjaeckel . I think this one is ready to be merged. @karel-m do you agree?

Copy link
Member

@sjaeckel sjaeckel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest LGTM & sorry for the delay, but I was busy in the last weeks.

@sjaeckel sjaeckel force-pushed the explicit_symbols_for_l_and_ll branch from 232ee0d to 58ae5c2 Compare July 3, 2019 10:50
@sjaeckel sjaeckel removed the request for review from karel-m July 3, 2019 10:51
@sjaeckel sjaeckel merged commit dad0fbd into develop Jul 3, 2019
@sjaeckel sjaeckel deleted the explicit_symbols_for_l_and_ll branch July 3, 2019 10:51
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.

4 participants