Opened 9 years ago
Closed 4 years ago
#12437 closed defect (fixed)
Fix remaining C++ issues of Lcalc (also to let it build with clang)
Reported by: | ohanar | Owned by: | GeorgSWeber |
---|---|---|---|
Priority: | major | Milestone: | sage-7.4 |
Component: | packages: standard | Keywords: | C++11 C++14 GCC 5 clang |
Cc: | fbissey, Snark, tscrim | Merged in: | |
Authors: | François Bissey | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | f498bff (Commits, GitHub, GitLab) | Commit: | f498bffb5d0c8cc88acc005e4af8def82c18237b |
Dependencies: | Stopgaps: |
Description (last modified by )
Lcalc still uses some non-standard "features" of g++
.
C++ compilers that are pickier than g++
will not allow this.
GCC 5.x again got stricter and now also needs the second "default parameter" patch (or once again -fpermissive
) mentioned below.
Current branch fix usage of internal headers and some declarations. More to be done as I haven't even scratched the issues mentioned in this ticket - unless they have been fixed by another merged ticket.
Attachments (1)
Change History (48)
Changed 9 years ago by
comment:1 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 Changed 9 years ago by
- Description modified (diff)
comment:3 Changed 9 years ago by
- Dependencies set to #12681
- Status changed from needs_review to needs_work
We now need a new spkg (based on that of #12681) incorporating this patch.
We also should report this upstream.
Patch so far looks good (not sure whether I've seen all of it -- maybe trac truncated it); seems like most functions (also) had the default values in their declarations.
comment:4 Changed 9 years ago by
- Work issues set to Create a new spkg.
comment:5 Changed 8 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:6 Changed 7 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:7 Changed 7 years ago by
- Component changed from build to packages: standard
comment:8 Changed 7 years ago by
GCC 4.9 now also requires -fpermissive
to build Lcalc as well as the Cython extension modules that use Lcalc's headers, unless we fix Lcalc.
comment:9 Changed 7 years ago by
I'll check [laterTM] whether Andrew's patch still applies (and whether it is sufficient for GCC 4.9 at least).
comment:10 Changed 7 years ago by
Hmmm, haven't tested Andrew's patch [yet], but tried by myself.
For GCC 4.9, apparently the following tiny patch is sufficient to build Lcalc and the Sage library extension module (which currently lacks a depends
btw.*):
-
include/Ldirichlet_series.h
diff -Naur lcalc-1.23-vanilla/include/Ldirichlet_series.h lcalc-1.23-fixed-gcc.4.9/include/Ldirichlet_series.h
old new 43 43 //XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX 44 44 template <class ttype> 45 45 Complex L_function <ttype>:: 46 dirichlet_series(Complex s, long long N =-1)46 dirichlet_series(Complex s, long long N) 47 47 { 48 48 Complex z=0.; 49 49 long long m,n; -
include/L.h
diff -Naur lcalc-1.23-vanilla/include/L.h lcalc-1.23-fixed-gcc.4.9/include/L.h
old new 491 491 492 492 //#include "Ldirichlet_series.h" //for computing Dirichlet series 493 493 Complex partial_dirichlet_series(Complex s, long long N1, long long N2); 494 Complex dirichlet_series(Complex s, long long N );494 Complex dirichlet_series(Complex s, long long N=-1LL); 495 495 496 496 //#include "Ltaylor_series.h" //for computing taylor series for Dirichlet series 497 497 //void compute_taylor_series(int N, int K, Complex s_0, Complex *series);
(In addition to the patches to Lcalc we already have in Sage, of which only a trivial one is needed to build Lcalc outside of Sage.)
*
-
src/module_list.py
diff --git a/src/module_list.py b/src/module_list.py index fa460be..b5449be 100755
a b ext_modules = [ 700 700 701 701 Extension('sage.libs.lcalc.lcalc_Lfunction', 702 702 sources = ['sage/libs/lcalc/lcalc_Lfunction.pyx'], 703 libraries = ['m', 'ntl', 'mpfr', 'gmp', 'gmpxx', 703 libraries = ['m', 'ntl', 'mpfr', 'gmp', 'gmpxx', # XXX Are all of these really needed? (NTL??) 704 704 'Lfunction'], 705 705 include_dirs = [SAGE_INC + "/libLfunction"], 706 extra_compile_args=["-O3", "-ffast-math"], 706 depends = [ os.path.join(SAGE_INC, "libLfunction", header) 707 for header in ["L.h"] ], # maybe more headers from there 708 extra_compile_args=["-O3", "-ffast-math"], # probably add '-Wno-deprecated', as of Lcalc 1.23 707 709 language = 'c++'), 708 710 709 711
(And, despite having language="c++"
, the extension module is compiled with the C compiler, not the C++ compiler, such that -- besides other effects -- CXXFLAGS
aren't used. It gets linked with g++
though.)
comment:11 follow-up: ↓ 13 Changed 7 years ago by
Your patch looks like a subset of mine. I can't remember why I did all the things that I did in my patch, but I would guess that I needed them to compile with clang. It may be that gcc 4.9 is still too permissive in this case.
comment:12 Changed 7 years ago by
- Cc fbissey added
comment:13 in reply to: ↑ 11 Changed 7 years ago by
Replying to ohanar:
Your patch looks like a subset of mine.
Yes. My primary goal was to make Lcalc work with GCC; I'll take a closer look at other changes later.
Adding void
to the friend
declaration of reset()
is certainly correct (although slightly unrelated to what the name of the patch suggests, on the broader topic of this ticket, namely making Lcalc's code build with clang
).
In the other hunks, you just removed default parameters from definitions, but did not move them to corresponding declarations (probably because there aren't any, which is something I'll have to check). But from at least one comment, it looks like those parts were "dead" code anyway.
comment:14 Changed 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:15 Changed 7 years ago by
Ok, so I've now created 7 new patches to fix all remaining clang/non-GNU/C++11 issues with Lcalc (except for some deprecation warnings):
Makefile_2-dont_compile_C_with_CXXFLAGS.patch lcalc-1.23-fix_VLA_1.patch lcalc-1.23-fix_VLA_2.patch lcalc-1.23-fix_control_reaches_end_of_non-void_fn.patch lcalc-1.23-fix_typeof.patch lcalc-1.23-missing_return_type.patch lcalc-1.23_default_parameters_2.patch
But I'd prefer to put them on a single ticket, i.e., to either open a new one, or repurpose one of the two existing ones. (But before doing so, I'll anyway polish them a bit, and probably merge some of them into another or into one of the existing ones.)
I did take a slightly different approach to variable-length arrays; instead of using std::vector<>
, I'm using foo_t *_foo = new foo_t[dim1*dim2*dim3]
, along with a macro foo(i,j,k)
which maps the indices to a single one, and of course added delete [] _foo
where necessary.
So far tested with GCC 4.4, 4.8 and 4.9, clang 3.4.1, and a couple of -std=...
variations.
comment:16 follow-up: ↓ 17 Changed 7 years ago by
Hi leif,
these are quite some patches. Do you plan to send them upstream?
comment:17 in reply to: ↑ 16 Changed 7 years ago by
Replying to thansen:
Hi leif,
these are quite some patches. Do you plan to send them upstream?
Well, upstream... ;-)
The patches are pretty small, and I'll merge them into fewer I think, hopefully next weekend.
Pretty a while ago, someoneTM wanted to create an autotools project for Michael, but that apparently never happened, so we keep heavily patching the existing Makefile.
AFAIK, some previous patches (e.g. dealing with newer PARI versions) went upstream, i.e., Michael should at least be aware of them; I'll certainly send him the new / updated ones later. Not sure whether he's interested in changes removing GNU extensions. (Variable-length arrays can still be used depending on macros / the compiler though.)
comment:18 Changed 7 years ago by
Well I hope upstreaming them works out. Sage is not the only downstream who needs these patches. There's at least Debian (where I'm maintaining the package) and Fedora. Having them upstream would make everyones live a lot easier.
comment:19 Changed 7 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:20 Changed 6 years ago by
- Cc Snark added
I'm also interested in the issue. What is the status of the patches? Did upstream answer?
comment:21 Changed 6 years ago by
- Description modified (diff)
- Keywords C++11 C++14 GCC 5 clang added
- Milestone changed from sage-6.4 to sage-6.7
- Work issues changed from Create a new spkg. to Locate patches and upload them... ;-)
GCC 5.x now also needs a small subset of the fixes for clang (or -fpermissive
; then also a couple of Sage extension modules which include Lcalc headers have to get built with -fpermissive
, in CFLAGS
(!)). (cf. ticket description)
I'll probably deal with that on another ticket.
comment:22 follow-ups: ↓ 23 ↓ 24 Changed 6 years ago by
The -fpermissive
bit could live in module_list.py
with the appropriate sage extensions. But can the headers be fixed?
comment:23 in reply to: ↑ 22 Changed 6 years ago by
Replying to fbissey:
The
-fpermissive
bit could live inmodule_list.py
with the appropriate sage extensions. But can the headers be fixed?
Sure, trivial for g++
at least.
We of course could also add -fpermissive
in Lcalc's spkg-install
, but I rather meant
env CXXFLAGS="-fpermissive" ./sage -f lcalc env CFLAGS="-fpermissive" ./sage -b
to temporarily work around the errors when building Sage (with GCC 5.x).
I had two other issues (with gf2x and ncurses) which hopefully vanish tomorrow [i.e., later today down under] when the final 5.1 (or 5.0.1?) gets released.
comment:24 in reply to: ↑ 22 Changed 6 years ago by
Replying to fbissey:
The
-fpermissive
bit could live inmodule_list.py
with the appropriate sage extensions. But can the headers be fixed?
I probably misunderstood you; the (GCC 5.x/default parameters) patch is the same for Lcalc and the headers the Sage library uses (i.e., Lcalc uses the same so the errors are exactly the same, one patch for both issues).
comment:25 follow-up: ↓ 27 Changed 6 years ago by
I think I had misunderstood your description. Final release of gcc will be 5.1
. X.0.x
are now pre-release. In effect betas and release candidates. Was a pain in the ass to build 5.0.1_rc
on my cluster, there is regression in the build system, using --with-gmp=
and friends can bring system headers that you don't want in phase 2 and 3. Happened in the past and been fixed and now it's back,
comment:26 Changed 6 years ago by
This looks like the second default parameters patch I mentioned:
-
lcalc-1.23/include/Lgamma.h
77 77 //n=0 should just give log_GAMMA(z)... thus making log_GAMMA 78 78 //code obsolete. But leave log_GAMMA intact anyways. 79 79 template <class ttype> 80 precise(ttype) log_GAMMA (ttype z,int n =0)80 precise(ttype) log_GAMMA (ttype z,int n) 81 81 { 82 82 int M; 83 83 precise(ttype) log_G,r,r2,y; … … 230 230 //value exp_w which holds exp(-w) 231 231 //computes G(z,w), so there's an extra w^(-z) factor. 232 232 template <class ttype> 233 Complex inc_GAMMA (ttype z,ttype w, const char *method ="temme", ttype exp_w = 0, bool recycle=false)233 Complex inc_GAMMA (ttype z,ttype w, const char *method, ttype exp_w, bool recycle) 234 234 { 235 235 236 236 Complex G; … … 334 334 335 335 336 336 template <class ttype> 337 ttype cfrac_GAMMA (ttype z,ttype w, ttype exp_w =0, bool recycle=false) //computes G(z,w) via continued fraction337 ttype cfrac_GAMMA (ttype z,ttype w, ttype exp_w, bool recycle) //computes G(z,w) via continued fraction 338 338 { 339 339 340 340 ttype G; … … 424 424 } 425 425 426 426 template <class ttype> 427 ttype asympt_GAMMA (ttype z,ttype w, ttype exp_w = 0, bool recycle=false) //computes G(z,w) via asymptotic series427 ttype asympt_GAMMA (ttype z,ttype w, ttype exp_w, bool recycle) //computes G(z,w) via asymptotic series 428 428 { 429 429 430 430 if(my_verbose>3) cout << "called asympt_GAMMA("<<z<<","<<w<<")"<< endl; … … 446 446 447 447 448 448 template <class ttype> 449 ttype comp_inc_GAMMA (ttype z,ttype w,ttype exp_w = 0, bool recycle=false) //computes g(z,w)449 ttype comp_inc_GAMMA (ttype z,ttype w,ttype exp_w, bool recycle) //computes g(z,w) 450 450 { 451 451 452 452 ttype g; … … 604 604 } 605 605 606 606 template <class ttype> 607 Complex gamma_sum(Complex s, int what_type, ttype *coeff, int N, Double g, Complex l, Double Q, Long Period, Complex delta =1, const char *method="temme")607 Complex gamma_sum(Complex s, int what_type, ttype *coeff, int N, Double g, Complex l, Double Q, Long Period, Complex delta, const char *method) 608 608 { 609 609 Complex SUM=0; 610 610
(Not yet checked whether it's complete/sufficient; the GCC 5.0.1 release candidate is on another machine.)
comment:27 in reply to: ↑ 25 Changed 6 years ago by
Replying to fbissey:
I think I had misunderstood your description. Final release of gcc will be
5.1
.X.0.x
are now pre-release. In effect betas and release candidates. Was a pain in the ass to build5.0.1_rc
on my cluster, there is regression in the build system, using--with-gmp=
and friends can bring system headers that you don't want in phase 2 and 3. Happened in the past and been fixed and now it's back,
I was surprised building the first RC went smooth (on a pretty "clean" machine though); this time no cloog/PPL/ISL version hazards or the like... (make clean
still bails out though IIRC.)
W.r.t. the versioning: 5.1 in the subject, 5.0.1 in the message body; RC1 tarball called 5.1.0-, identifies as 5.0.1 (prerelease)... B)
comment:28 Changed 6 years ago by
- Dependencies changed from #12681 to #12681, #18316
The lcalc-1.23_default_parameters_2.patch is now there, and sufficient for GCC 5.x (needs review).
comment:29 follow-up: ↓ 30 Changed 6 years ago by
Can this be closed as duplicate of #18316?
comment:30 in reply to: ↑ 29 Changed 6 years ago by
Replying to jdemeyer:
Can this be closed as duplicate of #18316?
Nope, we just need a good new title (as the other issues with clang
/ C++, cf. the comment above) have been discussed here as well... (There's also #12436, but this one is probably a bit fresher.)
comment:31 Changed 6 years ago by
- Description modified (diff)
- Summary changed from lcalc defines default values for functions outside of declarations to Fix remaining C++ issues of Lcalc (also to let it build with clang)
comment:32 Changed 6 years ago by
P.S.:
I also have to retest my patches with more recent versions of clang
and g++
.
comment:33 Changed 4 years ago by
Leif, can you put the remaining stuff in a branch to test? And I would say close #12436 as a duplicate.
comment:34 Changed 4 years ago by
- Branch set to u/fbissey/lcalc_clang
- Commit set to 8a9fe644d0cd9b5827ddcd0acf2b01d1816b48bf
- Description modified (diff)
- Milestone changed from sage-6.7 to sage-7.5
New commits:
8a9fe64 | Remove use of internal header- cmath should be sufficient. Fix declaration.
|
comment:35 Changed 4 years ago by
Next issue
[lcalc-1.23.p14] clang++ -O3 -ffast-math -fPIC -I../include -c Lriemannsiegel.cc [lcalc-1.23.p14] In file included from Lriemannsiegel.cc:24: [lcalc-1.23.p14] In file included from ../include/L.h:539: [lcalc-1.23.p14] ../include/Ldokchitser.h:72:14: error: variable length array of non-POD element type 'Complex' (aka 'complex<double>') [lcalc-1.23.p14] Complex m[N+1]; [lcalc-1.23.p14] ^ [lcalc-1.23.p14] ../include/Ldokchitser.h:81:23: error: variable length array of non-POD element type 'Complex' (aka 'complex<double>') [lcalc-1.23.p14] Complex sum_log_Gamma[N+1][MYDIGITS+1]; [lcalc-1.23.p14] ^ [lcalc-1.23.p14] ../include/Ldokchitser.h:106:27: error: variable length array of non-POD element type 'Complex' (aka 'complex<double>') [lcalc-1.23.p14] Complex exp_sum_log_Gamma[N+1][MYDIGITS+1][MYDIGITS+1]; // symmetric functions [lcalc-1.23.p14] ^ [lcalc-1.23.p14] ../include/Ldokchitser.h:107:15: error: variable length array of non-POD element type 'Complex' (aka 'complex<double>') [lcalc-1.23.p14] Complex gamma[N+1][MYDIGITS+1]; // gamma(s+m[j]) for j = 1 to N [lcalc-1.23.p14] ^ [lcalc-1.23.p14] In file included from Lriemannsiegel.cc:24: [lcalc-1.23.p14] In file included from ../include/L.h:542: [lcalc-1.23.p14] ../include/Lexplicit_formula.h:28:12: error: variable length array of non-POD element type 'Complex' (aka 'complex<double>') [lcalc-1.23.p14] Complex b[num_coeffs+1]; [lcalc-1.23.p14] ^ [lcalc-1.23.p14] 5 errors generated.
comment:36 Changed 4 years ago by
- Description modified (diff)
comment:37 Changed 4 years ago by
- Dependencies #12681, #18316 deleted
comment:38 Changed 4 years ago by
- Cc tscrim added
comment:39 Changed 4 years ago by
The issue I hit now with this is pretty much the VLA. I am guessing I can get the first VLA patch from #12436 but not the second one.
@leif do you still have those patches and can you share them?
comment:40 Changed 4 years ago by
- Commit changed from 8a9fe644d0cd9b5827ddcd0acf2b01d1816b48bf to f498bffb5d0c8cc88acc005e4af8def82c18237b
Branch pushed to git repo; I updated commit sha1. New commits:
f498bff | Adapt old patch from Andrew Ohana for VLA in C++
|
comment:41 Changed 4 years ago by
- Milestone changed from sage-7.5 to sage-7.4
- Status changed from needs_work to needs_review
Actually doing the VLA like Andrew did initially in #12436 is good enough. And while we have warnings for all the other stuff mentioned in this tickets it all compiled successfully with clang (latest on Sierra, based on 3.8).
comment:42 Changed 4 years ago by
I'm hesitant about the changes of the form:
+diff --git a/include/Ldokchitser.h b/include/Ldokchitser.h
+index c67f01a..7b3e5c9 100644
+--- a/include/Ldokchitser.h
++++ b/include/Ldokchitser.h
+@@ -69,7 +69,7 @@ phi_series(int precision)
+
+ // compute the values m[j] for the respective lambda_k[j]
+
+- Complex m[N+1];
++ std::vector<Complex> m(N+1);
+ for (j=1;j<=N;j++)
+ m[j] = -2*lambda_k[j] + 2;
+
because my understanding is g++ to treats Complex m[N+1]
as Complex *m = new Complex[N+1]
, which changing to std::vector
could have an impact on performance. I haven't looked at the code itself, but my guess is that lcalc wanted to treat these as fixed length arrays whose size is determined at runtime.
comment:43 Changed 4 years ago by
Correct, they are VLA. The replacement with new
works very well for one dimensional arrays. Multidimensional ones are another story, I guess it would be similar with more ugly loops. It seems to me that the accepted behavior in C++ is to use vector for these (from stack overflow examples).
The mystery to me is that there are many more VLA in that particular code but only a few of them raise a flag. What gives?
comment:44 Changed 4 years ago by
I can understand why std::vector
would be a natural choice because you don't have to worry about memory management (specifically the deallocation). It just there is some slight overhead from using it rather than direct system calls with the array, but that is more likely a micro-optimization. Perhaps some of the others aren't raising warnings because their length can be determined at compile time?
One little change to a patch:
std::vector<std::vector<std::vector<Complex> > > exp_sum_log_Gamma(N+1); // symmetric functions std::vector<std::vector<Complex> > gamma(N+1); // gamma(s+m[j]) for j = 1 to N for (j=1;j<=N;j++){ exp_sum_log_Gamma[j].resize(MYDIGITS+1); + for (n=0;n<=MYDIGITS;n++) exp_sum_log_Gamma[j][n].resize(MYDIGITS+1); gamma[j].resize(MYDIGITS+1); } - for (j=1;j<=N;j++) for (n=0;n<=MYDIGITS;n++) exp_sum_log_Gamma[j][n].resize(MYDIGITS+1);
as I think it is (slightly) more readable considering we are already working with the 3d array.
comment:45 Changed 4 years ago by
I wouldn't worry too much about optimization at this stage. The compiler does a better job than you'll ever do on that. The only thing the compiler cannot do is find a better algorithm (although it may be able able to re-order your loops in a way you wouldn't have thought of). For the little change I copied the structure of the original patch which does that in separate stages. If you feel it is clearer your way, feel free to commit your change as a part of your review (I can do it myself if you insist).
comment:46 Changed 4 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
- Work issues Locate patches and upload them... ;-) deleted
It's in the realm of bikeshedding, so I'm sending this off to the buildbots.
comment:47 Changed 4 years ago by
- Branch changed from u/fbissey/lcalc_clang to f498bffb5d0c8cc88acc005e4af8def82c18237b
- Resolution set to fixed
- Status changed from positive_review to closed
for review purposes