Opened 5 years ago
Closed 5 years ago
#22711 closed defect (fixed)
fixing c++ issues in eclib
Reported by:  dimpase  Owned by:  

Priority:  critical  Milestone:  sage8.0 
Component:  packages: standard  Keywords:  
Cc:  fbissey, cremona, jdemeyer  Merged in:  
Authors:  Dima Pasechnik  Reviewers:  Jeroen Demeyer 
Report Upstream:  None of the above  read trac for reasoning.  Work issues:  
Branch:  4d2f8db (Commits, GitHub, GitLab)  Commit:  4d2f8db1c7b47612f005ebc39a12471bb4ddfefa 
Dependencies:  Stopgaps: 
Description (last modified by )
clang++
refuses to guess the correct return
statement. Also, delete[]
must not be mixed up with delete
. This ticket fixes these issues, and allows #22679 to go forward.
The tarball at http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/eclib20170330.tar.bz2
Change History (36)
comment:1 Changed 5 years ago by
 Branch changed from u/dimpase/cxx_eclib to u/dimpase/cxx_ecl
 Commit set to 4c4cca8d52584822af93c3acfa6bac342fe391c5
comment:2 Changed 5 years ago by
 Cc fbissey cremona added
 Report Upstream changed from N/A to Reported upstream. No feedback yet.
Please have a look. This is a very straightfoward change, basically just following meaningful warnings emitted by clang++.
comment:3 Changed 5 years ago by
John, it seems that a large part of it is already in your https://github.com/JohnCremona/eclib
Perhaps you can just pick what's not yet there from the patch, and make a new release...
comment:4 Changed 5 years ago by
 Cc jdemeyer added
OK perhaps, but not until next week. I am very receptive to pull requests into eclib. I am strongly opposed to patching eclib for Sage independently of that.
Note also that there has been a new release of eclib for several weeks waiting for a positive review jdemeyer got close but then it got forgotten.
Please change the "no feedback yet" tag  this is feedback.
comment:5 followup: ↓ 6 Changed 5 years ago by
eclib20170122
is in Volker's tree. I believe it is positively reviewed and should be in the next beta. Do you mean a more recent one? I cannot see a more recent tag on github.
comment:6 in reply to: ↑ 5 Changed 5 years ago by
comment:7 Changed 5 years ago by
That's good, I missed that had happened 3 days ago.
comment:8 Changed 5 years ago by
OK, let me see if this patch adds anything to what's already done on #22164.
comment:9 Changed 5 years ago by
 Dependencies set to #22164
 Priority changed from major to critical
The patch here catches one more missing []
in delete (thus a memory leak) in libsrc/newforms.cc
(after line 1665), a missing return bd;
just after line 109 in libsrc/htconst.cc
(something which clang++ 3.8
does not forgive you, resulting in lots of errors).
So these absolutely must be fixed.
There is also some more, regarding shifting negative values (undefined behavour in C++).
For the latter, I haven't done the complete jobit appears to be still working without it. Let's complete this fixing, cause it's just trouble in waiting to hit after a compiler update... One should start off #22164, naturally.
comment:10 followup: ↓ 12 Changed 5 years ago by
Thank you very much for checking this. COuld you possibly make a pull request into eclib for the critical changes?
I just checked that the code in Sage (after #22164) exactly matches my own current branch (i.e. I have done no more work on eclib since then), and I agree with the two fixes you have identified. It is not impossible that I could make a bugfix release soon with those two changes.
Can you give one example of where the code shifts negative values?
comment:11 Changed 5 years ago by
I have a version 20170330 with the two fixes in and an increment to the library version.
I can push that now to https://github.com/JohnCremona/eclib or add some more  I have time to make these edits but not to go searching for things a compiler I don't have picks up.
comment:12 in reply to: ↑ 10 ; followup: ↓ 13 Changed 5 years ago by
Replying to cremona:
Thank you very much for checking this. COuld you possibly make a pull request into eclib for the critical changes?
Will do. For the master branch, right?
I just checked that the code in Sage (after #22164) exactly matches my own current branch (i.e. I have done no more work on eclib since then), and I agree with the two fixes you have identified. It is not impossible that I could make a bugfix release soon with those two changes.
Can you give one example of where the code shifts negative values?
in the diff fragment below #define QS_LONG_MASK (~(1L<<QS_LONG_SHIFT))
is such an example; putting extra pair of brackets as done in the diff fixes this.
 a/libsrc/eclib/sieve_search.h +++ b/libsrc/eclib/sieve_search.h @@ 76,7 +76,7 @@ class point_counter : public point_processor { #define QS_LONG_SHIFT ((QS_LONG_LENGTH == 16) ? 4 : \ (QS_LONG_LENGTH == 32) ? 5 : \ (QS_LONG_LENGTH == 64) ? 6 : 0) #define QS_LONG_MASK (~(1L<<QS_LONG_SHIFT)) +#define QS_LONG_MASK (~((1L<<QS_LONG_SHIFT))) #define QS_HALF_MASK ((bit_array)(~((unsigned long)(1L) / 3))) //#define PERCENT 0.6
This matches one from ratpoints I fixed some time ago. But there are more, hopefully equally unambiguous.
It's more problematic if you do x<<y
for x a signed variable;
I'd probably not want to touch these myself, as this would require either changing the type of x, or copying abs(x) to an unsigned thing, and putting the sign back then.
comment:13 in reply to: ↑ 12 Changed 5 years ago by
Replying to dimpase:
Replying to cremona:
Thank you very much for checking this. COuld you possibly make a pull request into eclib for the critical changes?
Will do. For the master branch, right?
Hold off since I have already fixed the two specific things you mentioned.
I just checked that the code in Sage (after #22164) exactly matches my own current branch (i.e. I have done no more work on eclib since then), and I agree with the two fixes you have identified. It is not impossible that I could make a bugfix release soon with those two changes.
Can you give one example of where the code shifts negative values?
in the diff fragment below
#define QS_LONG_MASK (~(1L<<QS_LONG_SHIFT))
is such an example; putting extra pair of brackets as done in the diff fixes this. a/libsrc/eclib/sieve_search.h +++ b/libsrc/eclib/sieve_search.h @@ 76,7 +76,7 @@ class point_counter : public point_processor { #define QS_LONG_SHIFT ((QS_LONG_LENGTH == 16) ? 4 : \ (QS_LONG_LENGTH == 32) ? 5 : \ (QS_LONG_LENGTH == 64) ? 6 : 0) #define QS_LONG_MASK (~(1L<<QS_LONG_SHIFT)) +#define QS_LONG_MASK (~((1L<<QS_LONG_SHIFT))) #define QS_HALF_MASK ((bit_array)(~((unsigned long)(1L) / 3))) //#define PERCENT 0.6This matches one from ratpoints I fixed some time ago. But there are more, hopefully equally unambiguous.
That's unfortunate  this code was adapted from a (now very old) version of ratpoints, and that sort of macro is not my style at all.
It's more problematic if you do
x<<y
for x a signed variable; I'd probably not want to touch these myself, as this would require either changing the type of x, or copying abs(x) to an unsigned thing, and putting the sign back then.
I am not sure what to do next, so I will push the changes I already made to github.
comment:14 Changed 5 years ago by
I also have a new eclib20170330.tar.bz2 ready to post if we make no more code changes, or I can remake it if we do.
comment:15 followup: ↓ 16 Changed 5 years ago by
comment:16 in reply to: ↑ 15 Changed 5 years ago by
Replying to dimpase:
the rapoints fix was done in the branch on #12473, including the macro fix I propose in the diff in the comment 12 on this (#22711) ticket, so I don't see why you won't include this in the tarball, too.
OK I will do that. Without a further increase in version number since I have not distributed that tarball yet.
comment:17 Changed 5 years ago by
Done. The extra fix is now in the master at github and the new distribution file is at http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/eclib20170330.tar.bz2
comment:18 Changed 5 years ago by
 Description modified (diff)
comment:19 Changed 5 years ago by
 Description modified (diff)
comment:20 followup: ↓ 21 Changed 5 years ago by
in case, I can't access the new tarball:
Forbidden You don't have permission to access /staff/J.E.Cremona/ftp/eclib20170330.tar.bz2 on this server. Apache/1.3.27 Server at homepages.warwick.ac.uk Port 80
comment:21 in reply to: ↑ 20 Changed 5 years ago by
Replying to dimpase:
in case, I can't access the new tarball:
Forbidden You don't have permission to access /staff/J.E.Cremona/ftp/eclib20170330.tar.bz2 on this server. Apache/1.3.27 Server at homepages.warwick.ac.uk Port 80
Sorry, fixed now.
comment:22 Changed 5 years ago by
 Commit changed from 4c4cca8d52584822af93c3acfa6bac342fe391c5 to 9dd970f3abfad71363c3a4cd9fa2ac68e89fac92
Branch pushed to git repo; I updated commit sha1. New commits:
9dd970f  use the latest upstream tarball

comment:23 Changed 5 years ago by
 Description modified (diff)
 Report Upstream changed from Reported upstream. No feedback yet. to None of the above  read trac for reasoning.
comment:24 Changed 5 years ago by
 Status changed from new to needs_review
comment:25 Changed 5 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_review to needs_work
This branch should be based on #22164.
comment:26 followup: ↓ 27 Changed 5 years ago by
... and #22164 is now in 8.0.beta0.
comment:27 in reply to: ↑ 26 Changed 5 years ago by
 Dependencies #22164 deleted
comment:28 Changed 5 years ago by
But note that the original patch / branch posted here by dimpase is *no longer needed* since it consisted of patches to eclib which have been fixed upstream; so all that is needed here is a trivial new branch to refer to the new eclib version. I will do that now.
comment:29 Changed 5 years ago by
 Commit changed from 9dd970f3abfad71363c3a4cd9fa2ac68e89fac92 to 4d2f8db1c7b47612f005ebc39a12471bb4ddfefa
Branch pushed to git repo; I updated commit sha1. New commits:
4d2f8db  rebased over the new \beta

comment:30 Changed 5 years ago by
 Status changed from needs_work to needs_review
Here cometh the rebased branch.
comment:31 followup: ↓ 32 Changed 5 years ago by
Fantastic. I will test your branch on my newly built 8.0beta0 and report back. I hope it will not be seen as a conflict of interest for me to review this!
comment:32 in reply to: ↑ 31 ; followup: ↓ 33 Changed 5 years ago by
Replying to cremona:
I hope it will not be seen as a conflict of interest for me to review this!
You know, the Permanent Committee of Sagemath Bugs might have an issue with this, and launch an inquiry...
comment:33 in reply to: ↑ 32 ; followup: ↓ 34 Changed 5 years ago by
Replying to dimpase:
Replying to cremona:
I hope it will not be seen as a conflict of interest for me to review this!
You know, the Permanent Committee of Sagemath Bugs might have an issue with this, and launch an inquiry...
Now which country did you grow up in, I seem to have forgotten?
Running ptestlong gives only these:
sage t long warnlong 111.3 src/sage/homology/simplicial_complex.py # 1 doctest failed sage t long warnlong 111.3 src/sage/calculus/interpolators.pyx # Timed out (and interrupt failed) sage t long warnlong 111.3 src/sage/calculus/riemann.pyx # Timed out (and interrupt failed) sage t long warnlong 111.3 src/sage/matrix/matrix_double_dense.pyx # Timed out (and interrupt failed) sage t long warnlong 111.3 src/sage/finance/time_series.pyx # Timed out (and interrupt failed)
none of w3hich are related to eclib  and it's a beta release which I had not tested with the old eclib.
comment:34 in reply to: ↑ 33 Changed 5 years ago by
Replying to cremona:
Replying to dimpase:
Replying to cremona:
I hope it will not be seen as a conflict of interest for me to review this!
You know, the Permanent Committee of Sagemath Bugs might have an issue with this, and launch an inquiry...
Now which country did you grow up in, I seem to have forgotten?
Let me assure you that I grew up listening to BBC :)
comment:35 Changed 5 years ago by
 Status changed from needs_review to positive_review
comment:36 Changed 5 years ago by
 Branch changed from u/dimpase/cxx_ecl to 4d2f8db1c7b47612f005ebc39a12471bb4ddfefa
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
more C++ standard conformance