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:

Status badges

Description (last modified by jdemeyer)

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)

lcalc-defaults.patch (4.0 KB) - added by ohanar 9 years ago.
for review purposes

Download all attachments as: .zip

Change History (48)

Changed 9 years ago by ohanar

for review purposes

comment:1 Changed 9 years ago by ohanar

  • Status changed from new to needs_review

comment:2 Changed 9 years ago by ohanar

  • Description modified (diff)

comment:3 Changed 9 years ago by leif

  • 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 leif

  • Work issues set to Create a new spkg.

comment:5 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:6 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:7 Changed 7 years ago by jdemeyer

  • Component changed from build to packages: standard

comment:8 Changed 7 years ago by leif

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 leif

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 leif

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  
    4343 //XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
    4444template <class ttype>
    4545Complex L_function <ttype>::
    46 dirichlet_series(Complex s, long long N=-1)
     46dirichlet_series(Complex s, long long N)
    4747{
    4848    Complex z=0.;
    4949    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  
    491491
    492492    //#include "Ldirichlet_series.h" //for computing Dirichlet series
    493493    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);
    495495
    496496    //#include "Ltaylor_series.h" //for computing taylor series for Dirichlet series
    497497    //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 = [ 
    700700
    701701    Extension('sage.libs.lcalc.lcalc_Lfunction',
    702702              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??)
    704704                           'Lfunction'],
    705705              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
    707709              language = 'c++'),
    708710
    709711

(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: Changed 7 years ago by ohanar

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 fbissey

  • Cc fbissey added

comment:13 in reply to: ↑ 11 Changed 7 years ago by leif

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 vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:15 Changed 7 years ago by leif

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: Changed 7 years ago by thansen

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 leif

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 thansen

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 vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:20 Changed 6 years ago by Snark

  • 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 leif

  • 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: Changed 6 years ago by fbissey

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 leif

Replying to fbissey:

The -fpermissive bit could live in module_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 leif

Replying to fbissey:

The -fpermissive bit could live in module_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: Changed 6 years ago by 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 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,

Last edited 6 years ago by fbissey (previous) (diff)

comment:26 Changed 6 years ago by leif

This looks like the second default parameters patch I mentioned:

  • lcalc-1.23/include/Lgamma.h

     
    7777//n=0 should just give log_GAMMA(z)... thus making log_GAMMA
    7878//code obsolete. But leave log_GAMMA intact anyways.
    7979template <class ttype>
    80 precise(ttype) log_GAMMA (ttype z,int n=0)
     80precise(ttype) log_GAMMA (ttype z,int n)
    8181{
    8282    int M;
    8383    precise(ttype) log_G,r,r2,y;
     
    230230//value exp_w which holds exp(-w)
    231231//computes G(z,w), so there's an extra w^(-z) factor.
    232232template <class ttype>
    233 Complex inc_GAMMA (ttype z,ttype w, const char *method="temme", ttype exp_w = 0, bool recycle=false)
     233Complex inc_GAMMA (ttype z,ttype w, const char *method, ttype exp_w, bool recycle)
    234234{
    235235
    236236    Complex G;
     
    334334
    335335
    336336template <class ttype>
    337 ttype cfrac_GAMMA (ttype z,ttype w, ttype exp_w=0, bool recycle=false)  //computes G(z,w) via continued fraction
     337ttype cfrac_GAMMA (ttype z,ttype w, ttype exp_w, bool recycle)  //computes G(z,w) via continued fraction
    338338{
    339339
    340340        ttype G;
     
    424424}
    425425
    426426template <class ttype>
    427 ttype asympt_GAMMA (ttype z,ttype w, ttype exp_w = 0, bool recycle=false)  //computes G(z,w) via asymptotic series
     427ttype asympt_GAMMA (ttype z,ttype w, ttype exp_w, bool recycle)  //computes G(z,w) via asymptotic series
    428428{
    429429
    430430        if(my_verbose>3) cout << "called asympt_GAMMA("<<z<<","<<w<<")"<< endl;
     
    446446
    447447
    448448template <class ttype>
    449 ttype comp_inc_GAMMA (ttype z,ttype w,ttype exp_w = 0, bool recycle=false)  //computes g(z,w)
     449ttype comp_inc_GAMMA (ttype z,ttype w,ttype exp_w, bool recycle)  //computes g(z,w)
    450450{
    451451
    452452    ttype g;
     
    604604}
    605605
    606606template <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")
     607Complex gamma_sum(Complex s, int what_type, ttype *coeff, int N, Double g, Complex l, Double Q, Long Period, Complex delta, const char *method)
    608608{
    609609    Complex SUM=0;
    610610

(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 leif

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 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,

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 leif

  • 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: Changed 6 years ago by jdemeyer

Can this be closed as duplicate of #18316?

comment:30 in reply to: ↑ 29 Changed 6 years ago by leif

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 leif

  • 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 leif

P.S.:

I also have to retest my patches with more recent versions of clang and g++.

comment:33 Changed 4 years ago by fbissey

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 fbissey

  • Authors set to François Bissey
  • 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:

8a9fe64Remove use of internal header- cmath should be sufficient. Fix declaration.

comment:35 Changed 4 years ago by fbissey

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 jdemeyer

  • Description modified (diff)

comment:37 Changed 4 years ago by jdemeyer

  • Dependencies #12681, #18316 deleted

comment:38 Changed 4 years ago by tscrim

  • Cc tscrim added

comment:39 Changed 4 years ago by fbissey

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 git

  • Commit changed from 8a9fe644d0cd9b5827ddcd0acf2b01d1816b48bf to f498bffb5d0c8cc88acc005e4af8def82c18237b

Branch pushed to git repo; I updated commit sha1. New commits:

f498bffAdapt old patch from Andrew Ohana for VLA in C++

comment:41 Changed 4 years ago by fbissey

  • 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 tscrim

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.

Last edited 4 years ago by tscrim (previous) (diff)

comment:43 Changed 4 years ago by fbissey

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 tscrim

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.

Last edited 4 years ago by tscrim (previous) (diff)

comment:45 Changed 4 years ago by fbissey

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 tscrim

  • 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 vbraun

  • Branch changed from u/fbissey/lcalc_clang to f498bffb5d0c8cc88acc005e4af8def82c18237b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.