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.

Sign up for GitHub

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

Jump to bottom

[5.3 regression] Failure when using the C API in C++ code #13422

Open
kit-ty-kate opened this issue Sep 5, 2024 · 10 comments · May be fixed by ocaml-opam/ocaml-mccs#56
Open

[5.3 regression] Failure when using the C API in C++ code #13422

kit-ty-kate opened this issue Sep 5, 2024 · 10 comments · May be fixed by ocaml-opam/ocaml-mccs#56

Comments

@kit-ty-kate
Copy link
Member

Trying to compile mccs with the 5.3 branch (a4e7cbd) breaks with the following error message at least on macOS:

File "src_ext/mccs/src/dune", line 5, characters 63-73:
  5 |    new_criteria notuptodate_criteria removed_criteria mccscudf mccs_stubs)
                                                                     ^^^^^^^^^^
  (cd _build/default/src_ext/mccs/src && /usr/bin/gcc -O2 -fno-strict-aliasing -fwrapv -pthread -Wall -Wextra -Wno-unused-parameter -x c++ -DUSEGLPK -g -I /Users/runner/.cache/ocaml-local/lib/ocaml -I ../../cudf/lib -I ../../extlib/src -I glpk -o mccs_stubs.o -c mccs_stubs.cpp)
  In file included from mccs_stubs.cpp:7:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/mlvalues.h:20:
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/misc.h:353:1: error: expected expression
  CAMLnoret CAMLextern void caml_fatal_error (char *, ...)
  ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/misc.h:105:22: note: expanded from macro 'CAMLnoret'
    #define CAMLnoret [[noreturn]]
                       ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/misc.h:353:11: error: expected unqualified-id
  CAMLnoret CAMLextern void caml_fatal_error (char *, ...)
            ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/misc.h:140:34: note: expanded from macro 'CAMLextern'
  #define CAMLextern CAMLDLLIMPORT extern
                                   ^
  In file included from mccs_stubs.cpp:7:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/mlvalues.h:70:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:32:
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.tbl:17:1: error: expected parameter declarator
  DOMAIN_STATE(atomic_uintnat, young_limit)
  ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:31:44: note: expanded from macro 'DOMAIN_STATE'
  #define DOMAIN_STATE(type, name) CAMLalign(8) type name;
                                             ^
  In file included from mccs_stubs.cpp:7:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/mlvalues.h:70:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:32:
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.tbl:17:1: error: expected ')'
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:31:44: note: expanded from macro 'DOMAIN_STATE'
  #define DOMAIN_STATE(type, name) CAMLalign(8) type name;
                                             ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.tbl:17:1: note: to match this '('
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:31:34: note: expanded from macro 'DOMAIN_STATE'
  #define DOMAIN_STATE(type, name) CAMLalign(8) type name;
                                   ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/misc.h:156:29: note: expanded from macro 'CAMLalign'
  #define CAMLalign(n) alignas(n)
                              ^
  In file included from mccs_stubs.cpp:7:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/mlvalues.h:70:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:32:
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.tbl:17:1: error: a type specifier is required for all declarations
  DOMAIN_STATE(atomic_uintnat, young_limit)
  ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:31:34: note: expanded from macro 'DOMAIN_STATE'
  #define DOMAIN_STATE(type, name) CAMLalign(8) type name;
                                   ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/misc.h:156:22: note: expanded from macro 'CAMLalign'
  #define CAMLalign(n) alignas(n)
                       ^
  In file included from mccs_stubs.cpp:7:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/mlvalues.h:70:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:32:
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.tbl:17:14: error: expected ';' at end of declaration list
  DOMAIN_STATE(atomic_uintnat, young_limit)
               ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.tbl:28:1: error: expected parameter declarator
  DOMAIN_STATE(value*, young_ptr)
  ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:31:44: note: expanded from macro 'DOMAIN_STATE'
  #define DOMAIN_STATE(type, name) CAMLalign(8) type name;
                                             ^
  In file included from mccs_stubs.cpp:7:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/mlvalues.h:70:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:32:
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.tbl:28:1: error: expected ')'
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:31:44: note: expanded from macro 'DOMAIN_STATE'
  #define DOMAIN_STATE(type, name) CAMLalign(8) type name;
                                             ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.tbl:28:1: note: to match this '('
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:31:34: note: expanded from macro 'DOMAIN_STATE'
  #define DOMAIN_STATE(type, name) CAMLalign(8) type name;
                                   ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/misc.h:156:29: note: expanded from macro 'CAMLalign'
  #define CAMLalign(n) alignas(n)
                              ^
  In file included from mccs_stubs.cpp:7:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/mlvalues.h:70:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:32:
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.tbl:28:1: error: a type specifier is required for all declarations
  DOMAIN_STATE(value*, young_ptr)
  ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:31:34: note: expanded from macro 'DOMAIN_STATE'
  #define DOMAIN_STATE(type, name) CAMLalign(8) type name;
                                   ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/misc.h:156:22: note: expanded from macro 'CAMLalign'
  #define CAMLalign(n) alignas(n)
                       ^
  In file included from mccs_stubs.cpp:7:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/mlvalues.h:70:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:32:
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.tbl:28:14: error: expected ';' at end of declaration list
  DOMAIN_STATE(value*, young_ptr)
               ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.tbl:31:1: error: expected parameter declarator
  DOMAIN_STATE(value*, young_start)
  ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:31:44: note: expanded from macro 'DOMAIN_STATE'
  #define DOMAIN_STATE(type, name) CAMLalign(8) type name;
                                             ^
  In file included from mccs_stubs.cpp:7:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/mlvalues.h:70:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:32:
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.tbl:31:1: error: expected ')'
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:31:44: note: expanded from macro 'DOMAIN_STATE'
  #define DOMAIN_STATE(type, name) CAMLalign(8) type name;
                                             ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.tbl:31:1: note: to match this '('
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:31:34: note: expanded from macro 'DOMAIN_STATE'
  #define DOMAIN_STATE(type, name) CAMLalign(8) type name;
                                   ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/misc.h:156:29: note: expanded from macro 'CAMLalign'
  #define CAMLalign(n) alignas(n)
                              ^
  In file included from mccs_stubs.cpp:7:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/mlvalues.h:70:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:32:
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.tbl:31:1: error: a type specifier is required for all declarations
  DOMAIN_STATE(value*, young_start)
  ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:31:34: note: expanded from macro 'DOMAIN_STATE'
  #define DOMAIN_STATE(type, name) CAMLalign(8) type name;
                                   ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/misc.h:156:22: note: expanded from macro 'CAMLalign'
  #define CAMLalign(n) alignas(n)
                       ^
  In file included from mccs_stubs.cpp:7:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/mlvalues.h:70:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:32:
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.tbl:31:14: error: expected ';' at end of declaration list
  DOMAIN_STATE(value*, young_start)
               ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.tbl:34:1: error: expected parameter declarator
  DOMAIN_STATE(value*, young_end)
  ^
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:31:44: note: expanded from macro 'DOMAIN_STATE'
  #define DOMAIN_STATE(type, name) CAMLalign(8) type name;
                                             ^
  In file included from mccs_stubs.cpp:7:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/mlvalues.h:70:
  In file included from /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:32:
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.tbl:34:1: error: expected ')'
  /Users/runner/.cache/ocaml-local/lib/ocaml/caml/domain_state.h:31:44: note: expanded from macro 'DOMAIN_STATE'
  #define DOMAIN_STATE(type, name) CAMLalign(8) type name;
                                             ^
[...]

After talking with @Octachron i'll try to go back through #11557 soon and polishing it in hope this would fix this kind of error forever
@kit-ty-kate kit-ty-kate mentioned this issue Sep 5, 2024
Draft
@gasche
Copy link
Member

gasche commented Sep 5, 2024

I think that @MisterDA is probably the right person to get in touch with to fix these specific issues you are reporting -- adding more conditional macros for [[noreturn]], _Alignas etc. that work for C++.

@ocaml ocaml deleted a comment Sep 5, 2024
@MisterDA
Copy link
Contributor

MisterDA commented Sep 5, 2024

This one is fun. See how Dune invokes the C compiler and passes -x c++ to tell the compiler driver to treat the file as C++ instead of C.

(cd _build/default/src && /usr/bin/cc -O2 -fno-strict-aliasing -fwrapv -pthread -Wall -Wextra -Wno-unused-parameter -x c++ -DUSEGLPK -g -I /Users/antonin/Tarides/ocaml/5.3/_opam/lib/ocaml -I /Users/antonin/Tarides/ocaml/5.3/_opam/lib/cudf -I /Users/antonin/Tarides/ocaml/5.3/_opam/lib/extlib -I glpk -o mccs_stubs.o -c mccs_stubs.cpp)
$ cc -dM -E -x c++ - < /dev/null | grep __cplusplus
#define __cplusplus 199711L
$ cc --version
Apple clang version 15.0.0 (clang-1500.3.9.4)
$ /opt/homebrew/Cellar/llvm/18.1.8/bin/clang -dM -E -x c++ - < /dev/null | grep __cplusplus
#define __cplusplus 201703L

The C compiler driver defaults to C++97 whereas the [[noreturn]] keyword was introduced in C++11. We need other features from C++11, such as the thread_local keyword, and atomics. Not sure why we didn't catch this issue before.

I'm not sure if it's an OCaml problem or a Dune problem. I suppose it's Dune, as AFAIK OCaml doesn't invoke the C++ compiler nor provide flags through ocamlc -config to help build C++ code. IMO OCaml has all rights of expecting (at least) a C++11 compiler. So maybe dune should check whether the C++ compiler it calls defaults (at least) to C++11, or pass an appropriate -std=c++XX flag.

@dustanddreams
Copy link
Contributor

I'd consider this a Dune problem.

Dune will have to either invoke cxx (or whatever binary has been detected as the C++ compiler) instead of cc for C++ files, or pass an explicit -std option if -x c++ is used.

@gasche
Copy link
Member

gasche commented Sep 5, 2024 •
edited
Loading

It looks like this is not a upstream Dune problem, as -x c++ is passed by the configuration layer of the mcss package itself (see ocaml-opam/ocaml-mccs#53):

https://github.com/ocaml-opam/ocaml-mccs/blob/072fbd17eb38536a73aa73a29ec15b27738cb272/src/context_flags.ml#L26

What value of the -x parameter should be used instead?

Note: maybe the conf-c++ package should be updated to check that the C++ compiler installed is C++11-compliant?

@dustanddreams
Copy link
Contributor

I don't understand the rationale behind invoking a C compiler and having to pass explicit -l flags to link against the C++ runtime libraries, rather than invoking a C++ compiler, but that's irrelevant.

The use of -x c++ causes the C compiler to default to the LCD standard, i.e. C++98, which is unfit (as of 5.3) for the OCaml headers.

We can relax the OCaml requirements by coping with older standards, but it really would be nice if the compiler invocation was fixed to pass at least -std=c++11 - keep in mind that C++ prior to C++11 is a quite different language than modern C++, and it would be bad not to benefit from the C++11 features (such as move semantics which in turn speed-up many STL operations).

@MisterDA
Copy link
Contributor

MisterDA commented Sep 5, 2024

It does sound like a Dune problem if mccs has to hard-code a specific flag to tell Dune to invoke a C++ compiler instead of a C compiler on C++ files. Dune already has support for building C++ code. It has to be noted that mccs uses (lang dune 1.0) and maybe the support is fixed in (lang dune 3.x), by using foreign_stubs field.

@MisterDA
Copy link
Contributor

MisterDA commented Sep 5, 2024 •
edited
Loading

It has to be noted that mccs uses (lang dune 1.0) and maybe the support is fixed in (lang dune 3.x), by using foreign_stubs field.

Spoiler: no.

It looks like this is not a upstream Dune problem, as -x c++ is passed by the configuration layer of the mcss package itself (see ocaml-opam/ocaml-mccs#53):

This is unneeded as Dune 3.16 knowns how to invoke the C++ compiler from the C driver (but not Dune 2.0).

This was referenced Sep 5, 2024
Draft
Closed
@kit-ty-kate
Copy link
Member Author

It was fixed in dune in ocaml/dune#10962 and ocaml/dune#11009. Hopefully a release will appear before the next 5.3 alpha or beta.

The consensus above was that it was a dune problem, however thinking about it again, i'm not sure why this option has to be added by dune instead of being present in the compiler's existing native_cppflags (etc..) ocamlc -config values. What is the role of those C++ specific flags?

@MisterDA
Copy link
Contributor

being present in the compiler's existing native_cppflags (etc..) ocamlc -config values. What is the role of those C++ specific flags?

They are the C preprocessors flags, not C++ flags. The runtime is completely written in C and afaik never calls a C++ compiler. The -std=c++11 flag was needed because Dune was building C++ stubs with a C compiler driver through the -x c++ option, and the C++ compiler would default to an old C++ standard. The configure script checks that the C compiler used to build OCaml supports C11, and the installed headers assume that the C/C++ compiler used supports C11/C++11 constructs. We could add something like this to caml/config.h:

#if defined(__cplusplus) && __cplusplus < 201103L ||
    defined(__STDC_VERSION__) && __STDC_VERSION < 201103L
#error "A C11/C++11 compiler is required"
#endif

@kit-ty-kate
Copy link
Member Author

They are the C preprocessors flags, not C++ flags.

🤯 TIL

We could add something like this to caml/config.h:

#if defined(__cplusplus) && __cplusplus < 201103L ||
    defined(__STDC_VERSION__) && __STDC_VERSION < 201103L
#error "A C11/C++11 compiler is required"
#endif

If caml/config.h is always the first header to be included in every public headers then this would help a little to make the error clearer, but otherwise this would only be useful in a small number of cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add support for OCaml 5.3 when using certain C++ compilers kit-ty-kate/ocaml-mccs
4 participants
@gasche @MisterDA @kit-ty-kate @dustanddreams