Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build on macOS with ecl fails due to strict C compiler #59

Closed
dimpase opened this issue May 28, 2021 · 14 comments
Closed

build on macOS with ecl fails due to strict C compiler #59

dimpase opened this issue May 28, 2021 · 14 comments

Comments

@dimpase
Copy link
Contributor

dimpase commented May 28, 2021

see https://trac.sagemath.org/ticket/31863
typical errors are

;;; End of Pass 1.
;;; Internal error:
;;;   ** Error code 1 when executing
;;; (EXT:RUN-PROGRAM "gcc" ("-I." "-I/Users/palmieri/Desktop/Sage/git/sage/local/include/" "-O2" "-g" "-march=native" "-fPIC" "-fno-common" "-Ddarwin" "-O2" "-c" "fricas-lisp.c" "-o" "fricas-lisp.o")):
;;; fricas-lisp.c:608:25: error: implicit declaration of function 'writeablep' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
;;;   value0 = ecl_make_int(writeablep(ecl_base_string_pointer_safe(v2)));
;;;                         ^
;;; /Users/palmieri/Desktop/Sage/git/sage/local/include/ecl/external.h:1107:23: note: expanded from macro 'ecl_make_int'
;;; # define ecl_make_int ecl_make_int32_t
;;;                       ^
;;; fricas-lisp.c:639:25: error: implicit declaration of function 'remove_directory' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
;;;   value0 = ecl_make_int(remove_directory(ecl_base_string_pointer_safe(v2)));
;;;                         ^
;;; /Users/palmieri/Desktop/Sage/git/sage/local/include/ecl/external.h:1107:23: note: expanded from macro 'ecl_make_int'
;;; # define ecl_make_int ecl_make_int32_t
;;;                       ^
;;; fricas-lisp.c:639:25: note: did you mean 'L25_remove_directory_'?
;;; /Users/palmieri/Desktop/Sage/git/sage/local/include/ecl/external.h:1107:23: note: expanded from macro 'ecl_make_int'
;;; # define ecl_make_int ecl_make_int32_t
;;;                       ^
;;; ./fricas-lisp.eclh:35:18: note: 'L25_remove_directory_' declared here
;;; static cl_object L25_remove_directory_(cl_object );
;;;                  ^
;;; fricas-lisp.c:670:25: error: implicit declaration of function 'open_server' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
;;;   value0 = ecl_make_int(open_server(ecl_base_string_pointer_safe(v2)));
;;;                         ^
;;; /Users/palmieri/Desktop/Sage/git/sage/local/include/ecl/external.h:1107:23: note: expanded from macro 'ecl_make_int'
;;; # define ecl_make_int ecl_make_int32_t
;;;                       ^
;;; fricas-lisp.c:698:24: error: implicit declaration of function 'sock_get_int' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
;;;  value0 = ecl_make_int(sock_get_int(ecl_fixnum(v1fricas_lisp__purpose)));
;;;                        ^
;;; /Users/palmieri/Desktop/Sage/git/sage/local/include/ecl/external.h:1107:23: note: expanded from macro 'ecl_make_int'
;;; # define ecl_make_int ecl_make_int32_t
;;;                       ^
;;; fricas-lisp.c:710:24: error: implicit declaration of function 'sock_send_int' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
;;;  value0 = ecl_make_int(sock_send_int(ecl_fixnum(v1fricas_lisp__purpose),ecl_fixnum(v2fricas_lisp__val)));
;;;                        ^

One can either set CFLAGS to make these errors back into warnings, or fix the root cause.

@dimpase
Copy link
Contributor Author

dimpase commented May 28, 2021

perhaps @spaghettisalat can tell whether (defun ecl-foreign-call (name c-name return-type arguments))
needs to be fixed, or whether it's a ECL bug.

@dimpase
Copy link
Contributor Author

dimpase commented May 28, 2021

the generated C code looks tricky, they apparently do some kind of renaming trick I don't understand (this is a part of fricas-lisp.c related to remove_directory())

/*      function definition for G90                                   */
/*      optimize speed 3, debug 0, space 0, safety 0                  */
static cl_object L24__g90(cl_object v1)
{
 cl_object env0 = ECL_NIL;
 const cl_env_ptr cl_env_copy = ecl_process_env();
 cl_object value0;
TTL:
 {
  cl_object v2;
  v2 = si_copy_to_simple_base_string(v1);
  value0 = ecl_make_int(remove_directory(ecl_base_string_pointer_safe(v2)));
  cl_env_copy->nvalues = 1;
  return value0;
 }
}
/*      function definition for remove_directory                      */
/*      optimize speed 3, debug 0, space 0, safety 0                  */
static cl_object L25_remove_directory_(cl_object v1fricas_lisp__dir_name)
{
 cl_object env0 = ECL_NIL;
 const cl_env_ptr cl_env_copy = ecl_process_env();
 cl_object value0;
TTL:
 {
  cl_object v2;
  v2 = si_copy_to_simple_base_string(v1fricas_lisp__dir_name);
  value0 = L24__g90(v2);
  return value0;
 }
}
...

ecl_function_dispatch(cl_env_copy,ECL_SYM("ANNOTATE",1836))(4, VV[30], ECL_SYM("LOCATION",1842), VVtemp[58], VVtemp[52]) /*  ANNOTATE */;
ecl_function_dispatch(cl_env_copy,ECL_SYM("ANNOTATE",1836))(4, VV[30], ECL_SYM("LAMBDA-LIST",1000), ECL_NIL, VVtemp[59]) /*  ANNOTATE */;
ecl_cmp_defun(VV[121]);                         /*  remove_directory */
...

it looks that by design of ECL compiler there is such a skew naming of function implementations - so it seems most reasonable to go for setting flags to not treat these warnings as errors.

@spaghettisalat
Copy link

The issue is that there are simply include statements missing (at least for sockio-c.H1 and cfuns-c.H1). Unfortunately, these header files are a bit of a mess and they can't be straightforwardly included. I have implemented a partial fix for the problem in
spaghettisalat@378ceaa, but I don't have time to dig into these header files and clean them up as well.

Note also that there are some hacks in fricas-lisp.lisp for GCL using LISP:clines statements which presumably could also be made obsolete by proper include statements for GCL.

@dimpase
Copy link
Contributor Author

dimpase commented Jun 2, 2021

we now have a fix on https://git.sagemath.org/sage.git/diff?id2=6aa4ecee167d8152ad17370f674b5e778ba7cbc6&id=a2dc07f50f42477838b1c3712e79042684cf28ad
In addition to spaghettisalat/fricas@378ceaa it was necessary to add two extern declarations
as follows:

--- a/src/include/cfuns-c.H1
+++ b/src/include/cfuns-c.H1
@@ -3,5 +3,7 @@ extern int directoryp(char *);
 extern int make_path_from_file(char *, char *);
 extern int writeablep(char *);
 extern int readablep(char *);
+extern int remove_directory(char *);
+extern int makedir(char *);
 extern long findString(char *, char *);
 extern int copyEnvValue(char *, char *); 

Should we make a PR with all this?

@hebisch
Copy link
Collaborator

hebisch commented Jun 9, 2021 via email

@dimpase
Copy link
Contributor Author

dimpase commented Jun 9, 2021 via email

@dimpase
Copy link
Contributor Author

dimpase commented Jun 9, 2021

On the other hand, I agree with @hebisch that it looks suspicious that these header declarations are needed. At least the trivial UFFI example, calling sin() from libm, from ECL manual works on macOS 11 without any math.h header included explicitly. @spaghettisalat - maybe you could comment on this?

@dkochmanski
Copy link

Re UFFI example and sin() - math.h is included by ECL itself, that's why it works.

Re implicit declarations there are two ways around this problem:

  1. use dynamic ffi (a bonus point is that it will work when ecl is compiled only with the bytecodes compiler, downside is a big performance penalty - around 40x slower function call)
  2. expand your macro to add a declaration (that's what cffi does for static calls) or include the appropriate header

Adding a declaration is somewhat problematic, because in conforming C your pointer types must match (i.e no (void *) as a wilidcard pointer, it must be (int *), (float *) etc).

I suppose that it is ECL who is at fault for doesn't add these declarations. That said when you use the static ffi the manual provides the example where (ffi:clines "#include <math.h>") is included:

https://common-lisp.net/project/ecl/static/manual/Foreign-Function-Interface.html#index-SFFI-usage

@dimpase
Copy link
Contributor Author

dimpase commented Jun 9, 2021

  1. performance penalty - around 40x slower function call)

Is it only the 1st function call that it so much slower (due to runtime loading)?

@dkochmanski
Copy link

dkochmanski commented Jun 9, 2021

I don't know, I remember someone mentioning this benchmark but I did not do it myself. My impression is that this was reported as a constant tradeoff.

@hebisch
Copy link
Collaborator

hebisch commented Jun 9, 2021 via email

@dkochmanski
Copy link

dkochmanski commented Jun 9, 2021

I understand that ECL may have its internal limitations, but in such case documentation should clearly say this.

Sure, I've acknowledged in the previous post that it is ECL who is at fault. As of why ECL does not include them, see below.

Correct types are already requirement, so nothing really new here.

I'm mentioning this, because cffi (and ecl uffi implementation) treats all pointers as, well, pointers. From the ABI perspective it doesn't matter (but it matters for ECL because it generates C code), and C pointers have funny syntax to get them 100% right (i.e for 1% more than 99% cases).

@dimpase
Copy link
Contributor Author

dimpase commented Jun 9, 2021

I've started to address ECL's docs shortcomings in https://gitlab.com/embeddable-common-lisp/ecl/-/merge_requests/254

I suppose I should also mention there why in the example with sin() one can get away without adding #include <...>.

@oldk1331
Copy link
Collaborator

Fixed by ddf6723 and eea78e1, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants