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

[WIP] Executables define rule for C object files (-output-obj) #23

Merged
34 commits merged into from
Mar 12, 2018

Conversation

bobot
Copy link
Collaborator

@bobot bobot commented Mar 14, 2017

Should fix #22

Remains:

  • select the right extension .so/.dll
  • disable the rule if not a compiler with +PIC? or a better error messages?

@bobot
Copy link
Collaborator Author

bobot commented Mar 14, 2017

In ocamlbuild we used include $(shell ocamlc -where)/Makefile.config to get EXT_DLL, should I do the same logic here?

@ghost
Copy link

ghost commented Mar 14, 2017

You can use anything from Context.t, in particular you can use ctx.ext_dll :)

Then, so that we can reference this shared object file without hardcoding .so in jbuild files, you should also add an ext_dll variable in dollar_var_map.

We shouldn't disable the rule, instead we should use:

Build.fail { fail = fun () -> die "blah blah this is not supported" }

So that we get a nice failure when requesting to build this target.

@bobot bobot force-pushed the add-output-obj branch 2 times, most recently from d643151 to 223669b Compare March 22, 2017 12:23
@bobot
Copy link
Collaborator Author

bobot commented Mar 22, 2017

I'm a little worried about the disabling of the rule, because I don't know when shared library should work or not (specially on windows). Moreover I should perhaps detect -fpic using the Makefile variable WITH_FPIC directly. Do you have an advice?

@ghost
Copy link

ghost commented Mar 22, 2017

I'm sure fPIC is actually the right thing to look at. For instance I can see that there is a libasmrun_shared.so, which I assume is used for -output-obj. Maybe it's only needed for -output-complete-obj and/or only when you have C stubs. For pure OCaml code, I assume that it wouldn't be an issue unless -nodynlink is used.

/cc @mshinwell who has been using it and might know more.

If it's too complicated, let's just always enable the rule for now. It's only going to be a problem if you want to optionally install such a .so with opam. But I assume nobody installs such a .so in opam currently?

BTW there is the question of -output-obj vs -output-complete-obj. I don't know if it make sense to generate both at once. If it does maybe we could have foo.so and foo.complete.so

@bobot
Copy link
Collaborator Author

bobot commented Mar 22, 2017

I'm wondering if the rule is correct because it links the runtime and cstubs but it seems to be the goal of -output-complete-obj [1] (which doesn't appear anywhere in the manual). @whitequark has perhaps an advice.

@whitequark
Copy link
Member

I'm sure fPIC is actually the right thing to look at. For instance I can see that there is a libasmrun_shared.so, which I assume is used for -output-obj. Maybe it's only needed for -output-complete-obj and/or only when you have C stubs. For pure OCaml code, I assume that it wouldn't be an issue unless -nodynlink is used.

This is not correct. There are three runtimes:

  • libasmrun.a, which is built without -fPIC,
  • libasmrun_pic.a, which is built with -fPIC,
  • libasmrun_shared.so, which is libasmrun_pic.a linked as a shared object.

I do not know what is the purpose of libasmrun_shared.so, and I do not believe the compiler will use it anywhere by itself (though you could specify it with -runtime-variant if you so wish).

Any of the runtimes can be used with either -output-obj and -output-complete-obj. You want to use the PIC ones when the output of -output*-obj is linked into a shared object, and non-PIC ones otherwise.

As for the difference between -output-obj and -output-complete-obj, the latter links in the C stubs from autolink libraries but the former does not. Arguably -output-obj should have always done that, but I tried it and I think it was a compatibility hazard. In general, no one should use -output-obj anymore.

@ghost
Copy link

ghost commented Mar 22, 2017

@whitequark thanks for the clarifications.

Let's use only -output-complete-obj then. I hadn't though about it but it's true that you can also produce a .o with -output*-obj. In the end we have a lot of possible combinations: {byte,native}x{executable,object,shared-object} and I'm not sure it's worth systematically setting up all the rules given that they'll rarely be used.

(library ...) stanzas have a (modes ...) field. We should add something similar for (executalbes ...) stanzas but with a few more modes: {byte,native}_{object,shared_object}. The default would be (byte native) as for libraries.

@bobot, BTW there is now a test directory with sandboxed tests. Can you add a test for this feature?

@bobot
Copy link
Collaborator Author

bobot commented Mar 22, 2017

Thanks @whitequark for these precisions.

@diml , ok I will add a test. But I again had a problem with an out of date boot.exe, perhaps you could add a timestamp in the repository (a file with an only increasing number) that you change and commit when boot.exe should be rebuilt.

@ghost
Copy link

ghost commented Mar 28, 2017

@bobot I think we'll often forget to update the timestamp file. In practice, we should only need to rebuild boot.exe when the jbuild format evolves and we use the new feature in the jbuild files of jbuilder, so probably not that often. If this is a persistent problem, we could try to add {bin,src,test,...}/jbuild as dependencies of boot.exe in the Makefile. That should do the trick

@bobot
Copy link
Collaborator Author

bobot commented Mar 28, 2017

I think we'll often forget to update the timestamp file. In practice, we should only need to rebuild boot.exe when the jbuild format evolves and we use the new feature in the jbuild files of jbuilder, so probably not that often.

Indeed we will need to update the timestamp file very rarely.

If this is a persistent problem, we could try to add {bin,src,test,...}/jbuild as dependencies of boot.exe in the Makefile. That should do the trick

Yes it could be sufficient.

@ghost ghost mentioned this pull request Jul 27, 2017
            ie -output-obj ocamlopt options.
@wokalski
Copy link

wokalski commented Jan 2, 2018

This is important when it comes to cross compilation. With cross compilation I want to be able to produce an object file which would be linked to the final executable.

@ghost
Copy link

ghost commented Feb 2, 2018

@bobot, sorry i missed your last commits... I'll review this ASAP

@bobot
Copy link
Collaborator Author

bobot commented Feb 2, 2018

@diml, after the fixes have been merged to ocaml there are less problems. But this feature is really a configuration nightmare with many possible choices to provide to the user: "Should we give the possibility to link or not the stub libraries, link or not the dependencies of stub libraries, link or not the runtime, to produce shared or not library, using native or bytecode compiler" (all the choices are not given by ocaml and depend on the version). I though that we should link all the libraries that are needed for the ocaml program, ie --output-complete-obj, because it simplifies the life of the user but some sees value in --output-obj (cf: ocaml/ocaml#1351).

So it is a needed feature, but perhaps we could scale back to the really needed part, production of shared libraries (principally native but byte should work too). It is what I needed and it is what #301 needs.

@ghost
Copy link

ghost commented Feb 2, 2018

Ok, yh that seems complicated indeed. Happy to scale back. So technically we just remove the Object mode and keep only Executable and Shared_object, is that what you have in mind?

Another thought: currently, if the user wants to do themselves one of the mode that is not supported, it's really painful to construct the right command line. Maybe what we could do is the following: add a custom_linkages field in executables stanzas:

(executable
 (...
  (custom_linkages
   ((mode native)
    (extension ${ext_obj})
    (flags (--output-obj))))))

then, if a common pattern appears we can add builtin support for it.

What do you think?

@wokalski
Copy link

wokalski commented Feb 2, 2018

Sorry for chiming in but static libraries are useful in certain situations. For instance in my use case I can’t use sharedl ibraries because of Apple’s restrictions. They are supported - yes but it’s not allowed to use them in an App Store app.

@ghost
Copy link

ghost commented Feb 2, 2018

@wokalski would the custom_linkages idea work for you?

I imagine that you would write:

(executable
 (name foo)
 (custom_linkages
  ((mode native)
   (extension ${ext_obj})
   (flags (--output-obj)))))

it's a bit verbose, but it would unblock the situation until it's clearer what to do

@wokalski
Copy link

wokalski commented Feb 2, 2018

Yes, it would. I’m sorry; I missed your message.

@ghost ghost mentioned this pull request Mar 5, 2018
@ghost
Copy link

ghost commented Mar 7, 2018

Ok, the doc still need to be updated but I think the code is ready.

In the end we have the following: the (modes ...) field of executable stanzas now defines what linking modes will be used. A linking mode is a pair (<mode> <kind>) where <mode> is either byte or native and <kind> is one of exe, object and shared_object. And we can use the following short-hands:

  • exe or native for (native exe)
  • object for (native object)
  • shared_object for (native object)
  • byte for (byte exe)

The following extensions are used for each linking mode:

mode kind ext
byte exe .bc
native exe .exe
byte object .bc${ext_obj}
native object .exe${exe_obj}
byte shared_object .bc${ext_dll}
native shared_object ${ext_dll}

Issues

when compiling a .so with ocamlopt -ouput-complete-obj, -ldl -lm is not passed to the C compiler and as a result we get an error about the math symbols when dynamically loading the .so. To workaround this issue Dune is manually passing -ccopt X for every X in native_c_libraries.

I also checked the Jane Street jenga rules, and I can see that we are passing -ccopt -Wl,-z,now when compiling a .so with either ocamlc or ocamlopt with the following comment:

        (* side-step issue where the compiler expects register r10 to be unchanged across
           a call to caml_call_gc@plt, which is not true if the symbol resolution happens
           right then. This works by forcing the symbol resolution to happen at load time
           instead.  *)

@ghost ghost requested a review from rgrinberg March 7, 2018 17:51
src/exe.ml Outdated
match m.kind with
| Exe ->
if mode = Native && not has_native then
custom
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the user specifies native mode explicitly, will they still get bytecode here?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I'm not completely sure about this, but it seems simpler overall.

@rgrinberg
Copy link
Member

rgrinberg commented Mar 8, 2018 via email

@ghost
Copy link

ghost commented Mar 8, 2018

Basically this is the problem: currently, if you don't put a modes field, then there will always be a rule for .exe that will be native if possible, and bytecode if not.

For shared objects, the rule are not setup by default. So one needs a modes field. However, there is no way to get the native or byte behavior. There are several choices for this problem:

  • we can systematically setup the rules for the various combination, and with a modes field, the choice will be native or byte
  • we add a mode best to represent native or byte

Thinking about it, the 2nd solution seems best.

@ghost
Copy link

ghost commented Mar 8, 2018

BTW, @whitequark, does the following issue rings a bell: ocamlc -output-complete-obj ... links with the C libraries listed in ocamlc -config | grep bytecomp_c_libraries but ocamlopt -output-complete-obj ... doesn't link with the C libraries listed in ocamlopt -config | grep native_c_libraries?

@ghost
Copy link

ghost commented Mar 8, 2018

@rgrinberg I made the change to add best. I think this is better indeed and it is less surprising for users, as in some cases only native code might be supported.

Though this is a breaking change as before native meant native_or_custom. This is only a breaking change for byte-code only architectures. I checked the dune universe and only 3 packages have (modes (native)) in their jbuild file. I think tensorflow can indeed only be compiled in native mode, but the other two I don't know.

@ghost
Copy link

ghost commented Mar 9, 2018

@rgrinberg I have added the doc. Could you review the last changes?

@rgrinberg
Copy link
Member

OK, looks good. Can we have a test for a mode that isn't available if we have a good way to hide ocamlopt in a test?

@ghost
Copy link

ghost commented Mar 12, 2018

Sure, we have some tests for this in byte-code-only, I added a test of an error case. The error message is not great, ideally we should still setup the rules but make them error out with a nice error message saying that native compilation is not available.

@ghost ghost merged commit feba082 into ocaml:master Mar 12, 2018
bobot pushed a commit that referenced this pull request Aug 23, 2019
This pull request was closed.
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.

Support for -output-obj
5 participants