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

Test foreign library properly add stdc++ dependency #4846

Closed
wants to merge 3 commits into from

Conversation

recoules
Copy link
Contributor

@recoules recoules commented Aug 5, 2021

It is currently impossible to create a bytecode executable linked with a library depending on a c++ foreign_archive.

For instance:

Error: Error on dynamically loaded library: ./lib/dllhello_world.so: ./lib/dllhello_world.so: undefined symbol: _ZSt4cout

Manually running the ocamlmklib command with "-lstdc++" solves the issue (modulo the RPATH issue because it is not installed).

@nojb
Copy link
Collaborator

nojb commented Aug 5, 2021

Hello, you seem to have messed up a bit your git history. Could you rebase on main and keep only the new commits? Thanks!

@recoules
Copy link
Contributor Author

recoules commented Aug 5, 2021

Updated but it seems the main branch do not pass the CI (without this).

@recoules
Copy link
Contributor Author

Rebase the branch and simplify a little bit the example.

@voodoos Actually, we do not understand yet why the cxx flags seem to never be used.

let base_cxx_flags = function
| Gcc -> [ "-x"; "c++"; "-lstdc++"; "-shared-libgcc" ]

@voodoos voodoos self-assigned this Sep 15, 2021
@recoules
Copy link
Contributor Author

Some remarks:
The flags coming from cxx_flags.ml appear when you use the following in dune-project

(use_standard_c_and_cxx_flags true)

Still I find the documentation about it very misleading because it says:

Since Dune 2.8, it is possible to deactivate the systematic prepending of flags coming from ocamlc -config to the C compiler command line. This is done adding the following field to the dune-project file:
(use_standard_c_and_cxx_flags true)

For me, to "deactivate it", it should be (use_standard_c_and_cxx_flags false) and it should be activated by default.

Yet, even if the compile command line contains -lstdc++, it has no effect when compiling to object file with -c.
I think the -lstdc++ should be added at the very end of the linking command (that I expect to be the ocamlmklib).

@voodoos
Copy link
Collaborator

voodoos commented Sep 15, 2021

  • use_standard_c_and_cxx_flagswill be enabled by default in Dune 3.0.
  • I agree that the documentation is misleading, I will reword it !
  • Is the behaviour the same using the byte-complete mode ?

@recoules
Copy link
Contributor Author

Is the behaviour the same using the byte-complete mode ?

Sorry I do not know what is the byte-complete mode

@voodoos
Copy link
Collaborator

voodoos commented Sep 15, 2021

It allows to build native executables with an embedded interpreter: https://dune.readthedocs.io/en/stable/dune-files.html#linking-modes

And finally, you can use the special mode byte_complete for building a bytecode executable as a native self-contained executable. I.e. an executable that does not require the ocamlrun program to run and does not requires the C stubs to be installed as shared object files.

@recoules
Copy link
Contributor Author

Is the behaviour the same using the byte-complete mode ?

The behaviour seems to be the same as -lstdc++ is only added when compiling the object .o.
To manually solve the issue, I had to add a (c_library_flags -lstdc++) added the library rule.
I think it should not be too hard to automatically add this flag if the library has foreign cxx stubs.

@recoules
Copy link
Contributor Author

The last commit is just an example of how it can be handled (not necessarily the best one).

Another option would be to automatically add the flag in the ordered set c_library_flags when the library is parsed if its buildable part contains cxx.

@voodoos
Copy link
Collaborator

voodoos commented Sep 16, 2021

@jeremiedimino do you have an opinion on what would be the best design here ?

Is the -lstdc++ flag the only one necessary ? (and it may not be for all C++ compilers like clang)

@ghost
Copy link

ghost commented Sep 16, 2021

What is the problem being fixed here? From the conversion, my understanding is that if a library has C++ stubs and the C compiler is gcc then we should pass -lstdc++. However, we already have this in Dune source code:

let base_cxx_flags = function
  | Gcc -> [ "-x"; "c++"; "-lstdc++"; "-shared-libgcc" ]
  | Clang -> [ "-x"; "c++" ]
  | Msvc -> [ "/TP" ]
  | _ -> []

So I'm surprised this is not the case already.

@recoules
Copy link
Contributor Author

recoules commented Sep 16, 2021

I think at the end there is two problems:

  • the aforementioned flags are only active if we set (use_standard_c_and_cxx_flags true) while it should be the default case (@voodoos said it will be changed in Dune 3.0)
  • the -lstdc++ has strictly no effect when compiling to object file .o (since there is no linking phase). The flags should be added during the linking that is either when compiling the cma or the dll*.so (using ocamlmklib)

(note that I do not know what was its purpose here but I think that -shared-libgcc is also a link option)

@ghost
Copy link

ghost commented Sep 16, 2021

the aforementioned flags are only active if we set (use_standard_c_and_cxx_flags true) while it should be the default case (@voodoos said it will be changed in Dune 3.0)

Yes, it is a better default but it is a breaking change so we can only change it in the next major version of the language. In the meantime we can only encourage users to use this option.

the -lstdc++ has strictly no effect when compiling to object file .o (since there is no linking phase). The flags should be added during the linking that is either when compiling the cma or the dll*.so (using ocamlmklib)

Right, so you are saying that the -lstdc++ (and maybe also -shared-libgcc) we have in the code is useless because it is not passed at the right stage. @voodoos what do you think?

@voodoos
Copy link
Collaborator

voodoos commented Sep 17, 2021

That sound right indeed. I am not an expert on that matter, but if I understand well these two flags being link flags should be passed to ocamlmklib when building a library and ocamlc when building an executable with foreign stubs.

For libraries Dune has the field c_library_flags which is used in fact for both C and C++ linking. I think the correct thing would be to have default flags from Cxx_flags added to the standard set for this field.

For executables it looks like the link flags for foreign stubs are specified in the common (executable (link_flags ...) ...) field, so we should also add the defaults flags here ?

@ghost
Copy link

ghost commented Sep 20, 2021

That makes sense to me. If these flags are link flags we should pass them at the link stage.

@voodoos voodoos added this to the 3.0 milestone Sep 23, 2021
@rgrinberg
Copy link
Member

@voodoos do you think you'll get a chance to work on this soon? Otherwise I can give this a try.

@voodoos
Copy link
Collaborator

voodoos commented Oct 22, 2021

@voodoos do you think you'll get a chance to work on this soon? Otherwise I can give this a try.

We submitted our PR on the compiler but still have a lot of thing to do on Merlin's side. Maybe it's best if you could have a look.

I started splitting flags between compile and link time in that commit: voodoos@465b111
Feel free to cherry-pick.

There are two things left to solve:

  • It is not obvious to know when we should add the cxx link flags to the standard set of link_flags for executables
  • Correctly testing the feature is tricky as it depends on the installed compiler

@rgrinberg
Copy link
Member

We submitted our PR on the compiler but still have a lot of thing to do on Merlin's side. Maybe it's best if you could have a look.

What's the connection to the compiler and merlin? Perhaps I'm completely understanding what the issue is, but it seems like just in an issue with dune spitting out correct flags?

@voodoos
Copy link
Collaborator

voodoos commented Oct 22, 2021

Sorry, my answer was misleading. This has no relation with Dune or that precise PR. Just meant that I was quite busy with other projects.

If you are too I will try to finish this next week. (but I am unsure yet of the best way to solve the issues I mentionned in my previous post)

@ghost ghost removed this from the 3.0 milestone Nov 3, 2021
@voodoos
Copy link
Collaborator

voodoos commented Nov 30, 2021

I thought this would be fixed with #5185 but another issue arised linked to compiler detection:
Error: No rule found for lib/.dune/ccomp/ccomp

I am having a look right now and adding the 3.0.0 milestone.

@voodoos voodoos added this to the 3.0.0 milestone Nov 30, 2021
@voodoos voodoos mentioned this pull request Nov 30, 2021
@voodoos
Copy link
Collaborator

voodoos commented Nov 30, 2021

Closing in favor of #5249

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

Successfully merging this pull request may close these issues.

None yet

4 participants