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

ocamlrun.swg not compatible with CAML_SAFE_STRING (new default in ocaml) #2083

Closed
dmacks opened this issue Sep 30, 2021 · 2 comments
Closed
Labels

Comments

@dmacks
Copy link

dmacks commented Sep 30, 2021

I originally filed this as a FTBFS in graphviz, but now I think the actual cause is a bug in swig. In line 372 of ocaml/ocamlrun.swg:

	memcpy(String_val(SWIG_Field(vv,0)),p,len);

String_val() might be prototyped to return (char *) or (const char *) in ocaml's mlvalues.h:

#ifdef CAML_SAFE_STRING
#define String_val(x) ((const char *) Bp_val(x))
#else
#define String_val(x) ((char *) Bp_val(x))
#endif

with CAML_SAFE_STRING being the default in recent ocaml releases. When CAML_SAFE_STRING is defined, swig's memcpy() is a compiler error, for example (from graphviz):

gv_ocaml.cpp:1324:2: error: no matching function for call to 'memcpy'
        memcpy(String_val(SWIG_Field(vv,0)),p,len);
        ^~~~~~
/usr/include/string.h:72:7: note: candidate function not viable: no known conversion from 'const char *' to 'void *' for 1st argument
void    *memcpy(void *__dst, const void *__src, size_t __n);
         ^

Essentially, swig seems to be trying to copy data to a location that is declared as const. If I undef CAML_SAFE_STRING, compiling succeeds (no problem to write data to a non-const destination). I don't know anything about ocaml or swig, just that CAML_SAFE_STRING is the default in ocaml and therefore graphviz is blocked.

@ojwb ojwb added the OCaml label Dec 2, 2021
@ojwb
Copy link
Member

ojwb commented Jan 29, 2022

Looking at where String_val() is defined, changing to use Bp_val() instead should compile, but I've no idea if that would be correct.

@ZackerySpytz Can you take a look? This seems to stop SWIG working with recently ocaml (I confirmed with 4.13.1).

@ojwb ojwb closed this as completed in 9f74955 Feb 1, 2022
@ojwb
Copy link
Member

ojwb commented Feb 1, 2022

Using Bp_val() in the one place where we need a non-const pointer allows our test suite to pass, so I've pushed that as it seems a step forward.

@ZackerySpytz Would still appreciate a review that this is the correct approach.

ojwb added a commit that referenced this issue Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants