ECL has too few arguments and two many on file dpp.c #9917

sagetrac-drkirkby · 2010-09-16T10:27:54Z

When I'm building ecl-10.2.1 as part of Sage I get too warning messages from gcc.

/export/home/drkirkby/sage-4.6.alpha0/spkg/build/ecl-10.2.1.p2/src/src/c/dpp.c: In function 'put_declaration':
/export/home/drkirkby/sage-4.6.alpha0/spkg/build/ecl-10.2.1.p2/src/src/c/dpp.c:678:5: warning: too few arguments for format
/export/home/drkirkby/sage-4.6.alpha0/spkg/build/ecl-10.2.1.p2/src/src/c/dpp.c:680:13: warning: too many arguments for format

Looking at line 678 of dpp.c, I see:

    fprintf(out, "\tif (ecl_unlikely(narg!=%d))");

So there's a %d, but what is associate with the %d? There should be an integer, but there is not one. So it seems to me gcc is right to complain there are too few arguments for format.

Likewise, on line 680, I see:

    fprintf(out, "\t   FEwrong_num_arguments(MAKE_FIXNUM(%d));\n",
            nreq, function_code);

There we observe two arguments supplied, but only one %d is there. That does not make any sense to me. Both "nreq" and "function_code" are declared as integers, so should there not two %d's and not one.

Again, it seems gcc is right to complain that.

There are thousands of warning messages in Sage, but I'm a bit concerned about resolving those in ecl, as the ecl library being built has text relocation problems - see #9840

Dave

Upstream: Fixed upstream, in a later stable release.

CC: @jhpalmieri

Component: build

Keywords: ecl

Author: David Kirkby, Leif Leonhardy

Reviewer: John Palmieri

Merged: sage-4.6.alpha2

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

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

nexttime · 2010-09-16T16:18:04Z

comment:2

Perhaps temporarily fix this in Sage's 10.2.1 by patching it to the obvious:

    fprintf(out, "\tif (ecl_unlikely(narg!=%d))", nreq);

and

    fprintf(out, "\t   FEwrong_num_arguments(MAKE_FIXNUM(%d));\n",
            function_code);

(You don't have to report this upstream since it is already fixed.)

sagetrac-drkirkby · 2010-09-16T23:29:55Z

comment:3

Yes, I just downloaded the latest ecl using "git" and it is indeed fixed upstream. I'll create a package with the upstream fixes to these two lines of code. It will remove two warnings. I doubt it is the cause of my relocation problems, but at least it will not leave me wondering if it is.

sagetrac-drkirkby · 2010-09-17T00:03:54Z

comment:4

Now when this is compiled, the warnings messages have gone:

        gcc -I/export/home/drkirkby/sage-4.6.alpha0/spkg/build/ecl-10.2.1.p3/src/src/c -I/export/home/drkirkby/sage-4.6.alpha0/spkg/build/ecl-10.2.1.p3/src/build -I./ /export/home/drkirkby/sage-4.6.alpha0/spkg/build/ecl-10.2.1.p3/src/src/c/dpp.c  -I/export/home/drkirkby/sage-4.6.alpha0/local/include  -O2  -g  -Wall  -fPIC  -Dsun4sol2 -o dpp

An updated package can be found at

http://boxen.math.washington.edu/home/kirkby/patches/ecl-10.2.1.p3.spkg

This needs further testing, as I've only currently tested this on OpenSolaris, but I think it is as safe as a bank vault. Hence for now I've marked it "needs info" until such time as I can confirm it works fully on other systems.

Dave

sagetrac-drkirkby · 2010-09-17T00:03:54Z

Author: David Kirkby, Leif Leonhardy

sagetrac-drkirkby · 2010-09-17T00:04:57Z

Attachment: 9917-makes-changes-to-dpp.c.patch.gz

Makes acouple of changes to the file dpp.c, which will remove a couple of compiler warnings which looked potentially harmful.

sagetrac-drkirkby · 2010-09-17T01:30:34Z

comment:5

I've checked this now on OpenSolaris, Solaris 10 on SPARC (t2). Linux (sage.math) and OS X (bsd.math).

ECL installed ok on all of them. Maxima failed on OS X, but the failure is already known - see #8645.

As such, I'm now marking this for review. I don't know whether fixing these compiler warnings will solve the problems on Solaris (I doubt it will), but its not a good idea to have code like it was before.

Since this is already fixed upstream in their git repository, I've marked it as such.

Dave

sagetrac-drkirkby · 2010-09-17T01:30:34Z

Upstream: Fixed upstream, but not in a stable release.

jhpalmieri · 2010-09-17T01:40:12Z

comment:6

In the spkg, the file dpp.c.patch is empty. That should be fixed. Other than that, the changes look fine. I'll test it on a few systems, but I can't imagine there being a problem.

Regarding

    fprintf(out, "\t   FEwrong_num_arguments(MAKE_FIXNUM(%d));\n",
            nreq, function_code);

I find it amusing that this problem occurs in a line containing "wrong_num_arguments"...

nexttime · 2010-09-17T01:45:28Z

comment:7

It's also fixed in 10.4.1, which I think is a stable release.

That's called "Literal programming" I think, self-referential.

nexttime · 2010-09-17T01:55:09Z

comment:8

diff -Nu ecl-10.2.1.p1/src/src/c/dpp.c ecl-10.4.1/src/src/c/dpp.c
--- ecl-10.2.1.p1/src/src/c/dpp.c	2010-02-13 15:04:41.000000000 +0100
+++ ecl-10.4.1/src/src/c/dpp.c	2010-04-04 16:31:27.000000000 +0200
@@ -251,13 +251,26 @@
 }
 
 char *
-search_symbol(char *name, int *symbol_code)
+search_symbol(char *name, int *symbol_code, int code)
 {
 	int i;
 	for (i = 0; cl_symbols[i].name != NULL; i++) {
 		if (!strcasecmp(name, cl_symbols[i].name)) {
 			name = poolp;
-			if (i == 0) {
+                        if (code) {
+                                pushstr("MAKE_FIXNUM(/*");
+                                pushstr(cl_symbols[i].name);
+				pushstr("*/");
+				if (i >= 1000)
+					pushc((i / 1000) % 10 + '0');
+				if (i >= 100)
+					pushc((i / 100) % 10 + '0');
+				if (i >= 10)
+					pushc((i / 10) % 10 + '0');
+				pushc(i % 10 + '0');
+				pushstr(")");
+				pushc(0);
+                        } else if (i == 0) {
 				pushstr("Cnil");
 				pushc(0);
 			} else {
@@ -283,19 +296,20 @@
 }
 
 char *
-read_symbol()
+read_symbol(int code)
 {
 	char c, *name = poolp;
+        char end = code? ']' : '\'';
 
 	c = readc();
-	while (c != '\'') {
+	while (c != end) {
 		if (c == '_') c = '-';
 		pushc(c); 
 		c = readc();
 	}
 	pushc(0);
 
-	name = search_symbol(poolp = name, 0);
+	name = search_symbol(poolp = name, 0, code);
 	if (name == NULL) {
 		name = poolp;
 		printf("\nUnknown symbol: %s\n", name);
@@ -387,7 +401,10 @@
 		} else if (c == '@') {
 			c = readc();
 			if (c == '\'') {
-				(void)read_symbol();
+				(void)read_symbol(0);
+				poolp--;
+			} else if (c == '[') {
+				(void)read_symbol(1);
 				poolp--;
 			} else if (c == '@') {
 				pushc(c);
@@ -448,7 +465,7 @@
 get_function(void)
 {
 	function = read_function();
-	function_symbol = search_symbol(function, &function_code);
+	function_symbol = search_symbol(function, &function_code, 0);
 	if (function_symbol == NULL) {
 		function_symbol = poolp;
 		pushstr("Cnil");
@@ -675,9 +692,9 @@
   }
   if (nopt == 0 && !rest_flag && !key_flag) {
     put_lineno();
-    fprintf(out, "\tif (ecl_unlikely(narg!=%d))");
+    fprintf(out, "\tif (ecl_unlikely(narg!=%d))", nreq);
     fprintf(out, "\t   FEwrong_num_arguments(MAKE_FIXNUM(%d));\n",
-            nreq, function_code);
+            function_code);
   } else {
     simple_varargs = !rest_flag && !key_flag && ((nreq + nopt) < 32);
     if (key_flag) {
@@ -833,7 +850,14 @@
 	} else if (c == '\'') {
 		char *p;
 		poolp = pool;
-		p = read_symbol();
+		p = read_symbol(0);
+		pushc('\0');
+		fprintf(out,"%s",p);
+		goto LOOP;
+	}  else if (c == '[') {
+		char *p;
+		poolp = pool;
+		p = read_symbol(1);
 		pushc('\0');
 		fprintf(out,"%s",p);
 		goto LOOP;

If that convinces you more.

nexttime · 2010-09-17T01:59:50Z

comment:9

s/literal/literate/

It's getting late...

sagetrac-drkirkby · 2010-09-17T07:15:09Z

Attachment: 9917-non-empty-patch-file.patch.gz

Somehow the previous patch file I made was empty - this corrects that.

sagetrac-drkirkby · 2010-09-17T07:40:00Z

comment:10

I've updated the patch file - I've no idea how I managed to make it empty before!

There are of course several changes to the upstream code, but since the snapshot I took was an unstable release, I think it is wise to limit the changes to just what were causing a problem in Sage.

I'm not sure if there is a later stable release available or not - I've found the version number at the top right of the ECL web site rarely agrees with the version number one can actually download. I think we probably have the latest stable release due to a comment made by Juanjo on the ECL mailing list in my response to my question about this matter.

''Sage cannot rely on unstable releases, which is why I was slowly working on
another stable release -- unfortunately actual work got in the way.

Juanjo''

I've updated the .spkg at

http://boxen.math.washington.edu/home/kirkby/patches/ecl-10.2.1.p3.spkg

Dave

sagetrac-drkirkby · 2010-09-18T12:23:07Z

comment:11

Replying to @jhpalmieri:

I find it amusing that this problem occurs in a line containing "wrong_num_arguments"...

Yes the line in ECL is rather funny. It's a bit like that ATLAS file make_correct_shared.sh, which seems to screw up making the shared libraries!

Dave

jhpalmieri · 2010-09-18T15:36:16Z

Reviewer: John Palmieri

nexttime · 2010-09-18T16:06:01Z

Changed upstream from Fixed upstream, but not in a stable release. to Fixed upstream, in a later stable release.

qed777 · 2010-09-29T08:39:54Z

Merged: sage-4.6.alpha2

sagetrac-drkirkby mannequin added this to the sage-4.6 milestone Sep 16, 2010

sagetrac-drkirkby mannequin added c: build labels Sep 16, 2010

sagetrac-drkirkby mannequin assigned sagetrac-GeorgSWeber Sep 16, 2010

This comment has been minimized.

Sign in to view

sagetrac-drkirkby mannequin added the s: needs info label Sep 17, 2010

sagetrac-drkirkby mannequin added s: needs review and removed s: needs info labels Sep 17, 2010

jhpalmieri added s: positive review and removed s: needs review labels Sep 18, 2010

qed777 mannequin removed the s: positive review label Sep 29, 2010

qed777 mannequin closed this as completed Sep 29, 2010

johanrosenkilde mentioned this issue Nov 16, 2010

Move generally usable decorators to misc.decorators #9907

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ECL has too few arguments and two many on file dpp.c #9917

ECL has too few arguments and two many on file dpp.c #9917

sagetrac-drkirkby mannequin commented Sep 16, 2010

This comment has been minimized.

nexttime mannequin commented Sep 16, 2010

sagetrac-drkirkby mannequin commented Sep 16, 2010

sagetrac-drkirkby mannequin commented Sep 17, 2010

sagetrac-drkirkby mannequin commented Sep 17, 2010

sagetrac-drkirkby mannequin commented Sep 17, 2010

sagetrac-drkirkby mannequin commented Sep 17, 2010

sagetrac-drkirkby mannequin commented Sep 17, 2010

jhpalmieri commented Sep 17, 2010

nexttime mannequin commented Sep 17, 2010

nexttime mannequin commented Sep 17, 2010

nexttime mannequin commented Sep 17, 2010

sagetrac-drkirkby mannequin commented Sep 17, 2010

sagetrac-drkirkby mannequin commented Sep 17, 2010

sagetrac-drkirkby mannequin commented Sep 18, 2010

jhpalmieri commented Sep 18, 2010

nexttime mannequin commented Sep 18, 2010

qed777 mannequin commented Sep 29, 2010

ECL has too few arguments and two many on file dpp.c #9917

ECL has too few arguments and two many on file dpp.c #9917

Comments

sagetrac-drkirkby mannequin commented Sep 16, 2010

This comment has been minimized.

nexttime mannequin commented Sep 16, 2010

sagetrac-drkirkby mannequin commented Sep 16, 2010

sagetrac-drkirkby mannequin commented Sep 17, 2010

sagetrac-drkirkby mannequin commented Sep 17, 2010

sagetrac-drkirkby mannequin commented Sep 17, 2010

sagetrac-drkirkby mannequin commented Sep 17, 2010

sagetrac-drkirkby mannequin commented Sep 17, 2010

jhpalmieri commented Sep 17, 2010

nexttime mannequin commented Sep 17, 2010

nexttime mannequin commented Sep 17, 2010

nexttime mannequin commented Sep 17, 2010

sagetrac-drkirkby mannequin commented Sep 17, 2010

sagetrac-drkirkby mannequin commented Sep 17, 2010

sagetrac-drkirkby mannequin commented Sep 18, 2010

jhpalmieri commented Sep 18, 2010

nexttime mannequin commented Sep 18, 2010

qed777 mannequin commented Sep 29, 2010