Fix remaining C++ issues of Lcalc (also to let it build with clang) #12437

ohanar · 2012-02-04T18:56:51Z

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.

CC: @kiwifb @SnarkBoojum @tscrim

Component: packages: standard

Keywords: C++11 C++14 GCC 5 clang

Author: François Bissey

Branch/Commit: f498bff

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/12437

The text was updated successfully, but these errors were encountered:

ohanar · 2012-02-04T18:57:09Z

Attachment: lcalc-defaults.patch.gz

for review purposes

nexttime · 2012-03-22T12:32:17Z

Dependencies: #12681

nexttime · 2012-03-22T12:32:17Z

comment:3

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.

nexttime · 2012-03-22T12:33:12Z

Work Issues: Create a new spkg.

nexttime · 2014-04-21T10:55:55Z

comment:8

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.

nexttime · 2014-04-21T10:58:20Z

comment:9

I'll check [later^TM] whether Andrew's patch still applies (and whether it is sufficient for GCC 4.9 at least).

nexttime · 2014-04-21T13:37:05Z

comment:10

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.^*):

diff -Naur lcalc-1.23-vanilla/include/Ldirichlet_series.h lcalc-1.23-fixed-gcc.4.9/include/Ldirichlet_series.h
--- lcalc-1.23-vanilla/include/Ldirichlet_series.h	2012-08-08 23:21:55.000000000 +0200
+++ lcalc-1.23-fixed-gcc.4.9/include/Ldirichlet_series.h	2014-04-21 14:37:59.027464849 +0200
@@ -43,7 +43,7 @@
  //XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 template <class ttype>
 Complex L_function <ttype>::
-dirichlet_series(Complex s, long long N=-1)
+dirichlet_series(Complex s, long long N)
 {
     Complex z=0.;
     long long m,n;
diff -Naur lcalc-1.23-vanilla/include/L.h lcalc-1.23-fixed-gcc.4.9/include/L.h
--- lcalc-1.23-vanilla/include/L.h	2012-08-08 23:21:55.000000000 +0200
+++ lcalc-1.23-fixed-gcc.4.9/include/L.h	2014-04-21 14:32:04.003467348 +0200
@@ -491,7 +491,7 @@
 
     //#include "Ldirichlet_series.h" //for computing Dirichlet series
     Complex partial_dirichlet_series(Complex s, long long N1, long long N2);
-    Complex dirichlet_series(Complex s, long long N);
+    Complex dirichlet_series(Complex s, long long N=-1LL);
 
     //#include "Ltaylor_series.h" //for computing taylor series for Dirichlet series
     //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.)

^*

diff --git a/src/module_list.py b/src/module_list.py
index fa460be..b5449be 100755
--- a/src/module_list.py
+++ b/src/module_list.py
@@ -700,10 +700,12 @@ ext_modules = [
 
     Extension('sage.libs.lcalc.lcalc_Lfunction',
               sources = ['sage/libs/lcalc/lcalc_Lfunction.pyx'],
-              libraries = ['m', 'ntl', 'mpfr', 'gmp', 'gmpxx',
+              libraries = ['m', 'ntl', 'mpfr', 'gmp', 'gmpxx',         # XXX Are all of these really needed? (NTL??)
                            'Lfunction'],
               include_dirs = [SAGE_INC + "/libLfunction"],
-              extra_compile_args=["-O3", "-ffast-math"],
+              depends = [ os.path.join(SAGE_INC, "libLfunction", header)
+                          for header in ["L.h"] ],                     # maybe more headers from there
+              extra_compile_args=["-O3", "-ffast-math"],               # probably add '-Wno-deprecated', as of Lcalc 1.23
               language = 'c++'),

(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.)

ohanar · 2014-04-21T18:40:18Z

comment:11

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.

nexttime · 2014-04-21T23:42:18Z

comment:13

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.

nexttime · 2014-05-19T01:07:11Z

comment:15

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.

tobihan · 2014-05-20T22:43:30Z

comment:16

Hi leif,

these are quite some patches. Do you plan to send them upstream?

nexttime · 2014-05-21T09:54:44Z

comment:17

Replying to @tobihan:

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, someone^TM 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.)

tobihan · 2014-05-21T10:23:06Z

comment:18

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.

kiwifb · 2016-09-18T21:40:53Z

Author: François Bissey

kiwifb · 2016-09-18T21:42:49Z

comment:35

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.

jdemeyer · 2016-09-19T15:15:02Z

Changed dependencies from #12681, #18316 to none

kiwifb · 2016-09-29T21:50:54Z

comment:39

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.

@nexttime do you still have those patches and can you share them?

sagetrac-git · 2016-09-30T00:36:05Z

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

`f498bff`	`Adapt old patch from Andrew Ohana for VLA in C++`

sagetrac-git · 2016-09-30T00:36:05Z

Changed commit from 8a9fe64 to f498bff

kiwifb · 2016-09-30T00:40:47Z

comment:41

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).

tscrim · 2016-10-04T19:16:37Z

comment:42

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.

kiwifb · 2016-10-04T19:25:30Z

comment:43

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?

tscrim · 2016-10-04T19:50:58Z

comment:44

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.

kiwifb · 2016-10-05T21:26:06Z

comment:45

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).

tscrim · 2016-10-05T21:28:46Z

Reviewer: Travis Scrimshaw

tscrim · 2016-10-05T21:28:46Z

Changed work issues from Locate patches and upload them... ;-) to none

tscrim · 2016-10-05T21:28:46Z

comment:46

It's in the realm of bikeshedding, so I'm sending this off to the buildbots.

vbraun · 2016-10-21T07:04:42Z

Changed branch from u/fbissey/lcalc_clang to f498bff

ohanar added this to the sage-5.11 milestone Feb 4, 2012

ohanar added c: build labels Feb 4, 2012

ohanar assigned sagetrac-GeorgSWeber Feb 4, 2012

ohanar added the s: needs review label Feb 4, 2012

This comment has been minimized.

Sign in to view

nexttime mannequin added s: needs work and removed s: needs review labels Mar 22, 2012

jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013

sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014

jdemeyer added c: packages: standard and removed c: build labels Feb 4, 2014

sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014

This comment has been minimized.

Sign in to view

kiwifb modified the milestones: sage-6.7, sage-7.5 Sep 18, 2016

This comment has been minimized.

Sign in to view

kiwifb modified the milestones: sage-7.5, sage-7.4 Sep 30, 2016

kiwifb added s: needs review and removed s: needs work labels Sep 30, 2016

tscrim added s: positive review and removed s: needs review labels Oct 5, 2016

vbraun removed the s: positive review label Oct 21, 2016

vbraun closed this as completed in 455d8f4 Oct 21, 2016

nexttime mannequin mentioned this issue Mar 22, 2012

lcalc does not respect global CXX flag #12435

Closed

ohanar mentioned this issue Jan 21, 2017

lcalc uses variable length arrays #12436

Closed

nexttime mannequin mentioned this issue Apr 29, 2015

Lcalc fails to build with GCC 5.x (C++ issue) #18316

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix remaining C++ issues of Lcalc (also to let it build with clang) #12437

Fix remaining C++ issues of Lcalc (also to let it build with clang) #12437

ohanar commented Feb 4, 2012

ohanar commented Feb 4, 2012

This comment has been minimized.

nexttime mannequin commented Mar 22, 2012

nexttime mannequin commented Mar 22, 2012

nexttime mannequin commented Mar 22, 2012

nexttime mannequin commented Apr 21, 2014

nexttime mannequin commented Apr 21, 2014

nexttime mannequin commented Apr 21, 2014

ohanar commented Apr 21, 2014

nexttime mannequin commented Apr 21, 2014

nexttime mannequin commented May 19, 2014

tobihan commented May 20, 2014

nexttime mannequin commented May 21, 2014

tobihan commented May 21, 2014

This comment has been minimized.

kiwifb commented Sep 18, 2016

kiwifb commented Sep 18, 2016

This comment has been minimized.

jdemeyer commented Sep 19, 2016

kiwifb commented Sep 29, 2016

sagetrac-git mannequin commented Sep 30, 2016

sagetrac-git mannequin commented Sep 30, 2016

kiwifb commented Sep 30, 2016

tscrim commented Oct 4, 2016

kiwifb commented Oct 4, 2016

tscrim commented Oct 4, 2016

kiwifb commented Oct 5, 2016

tscrim commented Oct 5, 2016

tscrim commented Oct 5, 2016

tscrim commented Oct 5, 2016

vbraun commented Oct 21, 2016

Fix remaining C++ issues of Lcalc (also to let it build with clang) #12437

Fix remaining C++ issues of Lcalc (also to let it build with clang) #12437

Comments

ohanar commented Feb 4, 2012

ohanar commented Feb 4, 2012

This comment has been minimized.

nexttime mannequin commented Mar 22, 2012

nexttime mannequin commented Mar 22, 2012

nexttime mannequin commented Mar 22, 2012

nexttime mannequin commented Apr 21, 2014

nexttime mannequin commented Apr 21, 2014

nexttime mannequin commented Apr 21, 2014

ohanar commented Apr 21, 2014

nexttime mannequin commented Apr 21, 2014

nexttime mannequin commented May 19, 2014

tobihan commented May 20, 2014

nexttime mannequin commented May 21, 2014

tobihan commented May 21, 2014

This comment has been minimized.

kiwifb commented Sep 18, 2016

kiwifb commented Sep 18, 2016

This comment has been minimized.

jdemeyer commented Sep 19, 2016

kiwifb commented Sep 29, 2016

sagetrac-git mannequin commented Sep 30, 2016

sagetrac-git mannequin commented Sep 30, 2016

kiwifb commented Sep 30, 2016

tscrim commented Oct 4, 2016

kiwifb commented Oct 4, 2016

tscrim commented Oct 4, 2016

kiwifb commented Oct 5, 2016

tscrim commented Oct 5, 2016

tscrim commented Oct 5, 2016

tscrim commented Oct 5, 2016

vbraun commented Oct 21, 2016