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

recint seems to abuse 32 bits asm instructions #130

Closed
ClementPernet opened this issue Jul 10, 2019 · 6 comments · Fixed by #137
Closed

recint seems to abuse 32 bits asm instructions #130

ClementPernet opened this issue Jul 10, 2019 · 6 comments · Fixed by #137
Assignees
Labels

Comments

@ClementPernet
Copy link
Member

The problem arose here:
https://trac.sagemath.org/ticket/26932#comment:56

I could reproduce by building sage on this branch on the i386 machine on ci.inria.
From what I understand, recint hard codes limb to be int64_t. However in recint/rudiv.h line 222, it calls umul_ppmm with 4 arguments of limb type. And accordind to reclonglong.h line 866, the implementation of umul_ppmm on intel 32 bits (i386) is an asm call to mull which expects 32 bits ints.
I don't know why this bug only shows up in the sage build and not in givaro's test-suite on this machine.

@ClementPernet
Copy link
Member Author

Note that the compilation failure is reproducible by typing make in /builds/sage on the 32 bit ubuntu ci.inria slave.

@Breush
Copy link
Contributor

Breush commented Jul 11, 2019

@ClementPernet Interesting, I'm going to check that.

@Breush
Copy link
Contributor

Breush commented Jul 11, 2019

@ClementPernet We used #define NO_ASM and #define W_TYPE_SIZE 64 in recdefine.h.
So we probably shouldn't go to the umul_ppmm line 866 in any case.

Looking for it, I think that, for me, the umul_ppmm is the plain C one defined line 1971.

On the 32 bit machine, however, I don't really know.

@ClementPernet
Copy link
Member Author

For the record, I have identified the bug: this is because in sage, givaro's reclonglong.h yields for flint's longlong.h which does not include the NO_ASM section.
See: https://trac.sagemath.org/ticket/26932#comment:64
Workaround in progress...

@ClementPernet
Copy link
Member Author

The workaround used in https://trac.sagemath.org/ticket/26932 consists in avoiding name conflicts between givaro's reclonglong.h and flint's longlong.h. I ended up adding a recint_ prefix to all macros defined in reclonglong.h. For the moment, this is just a patch that sage applies to givaro. If nobody objects, I'll apply this patch upstream.

@tobihan
Copy link

tobihan commented Sep 13, 2019

We also hit this issue in Debian now. From our side it would be welcome if you apply the patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants