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

Generate a hard reference to "caml_startup" in Asmlink #1740

Merged
merged 6 commits into from May 1, 2018

Conversation

Projects
None yet
4 participants
@diml
Copy link
Member

diml commented Apr 26, 2018

Following a discussion on caml-devel, this PR makes sure that object files generated by -output-complete-obj always contain caml_startup.

We have tests for -output-complete-obj in Dune and they test all the shared/dynamic and byte/native combinations. On FreeBSD, most of them fail with master and they all pass with this PR.

Generate a hard reference to "caml_startup" in Asmlink
Without that, startup.o wasn't linked in object files generated by
-output-complete-obj on some platforms such as FreeBSD.
@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Apr 27, 2018

@@ -203,6 +203,9 @@ let scan_file obj_name tolink = match read_file obj_name with

(* Second pass: generate the startup file and link it with everything else *)

let force_linking_of_startup ppf =
Asmgen.compile_phrase ppf (Cmm.Cdata([Cmm.Cglobal_symbol "caml_startup"]))

This comment has been minimized.

@xavierleroy

xavierleroy Apr 27, 2018

Contributor

Like @shindere I'm tempted to have this (or maybe its two calls) protected by if !Clflags.output_complete_object then ..., if only to document-in-code that this forced linking is required only in that case.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Apr 27, 2018

@diml

This comment has been minimized.

Copy link
Member Author

diml commented Apr 30, 2018

I added the condition and enabled the output-complete-obj test. According to precheck, the test succeeds everywhere (including Windows) except on OSX. The error on OSX is the same with and without this PR:

$ gcc -O2 -fno-strict-aliasing -fwrapv -Wall -Werror -D_FILE_OFFSET_BITS=64 -D_REENTRANT -DCAML_NAME_SPACE -Wl,-no_compact_unwind -I/Users/jdimino/ocaml/byterun -o test.ml_stub.exe test.ml.exe.o -lpthread test.ml_stub.c
Undefined symbols for architecture x86_64:
  "_caml_code_area_end", referenced from:
      _segv_handler in test.ml.exe.o
  "_caml_code_area_start", referenced from:
      _segv_handler in test.ml.exe.o
  "_caml_startup", referenced from:
      _main in test-75e20c.o
     (maybe you meant: _caml_startup__frametable, _caml_startup__data_end , _caml_startup__data_begin , _caml_startup__code_end , _caml_startup__code_begin , _caml_startup_aux )
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
@diml

This comment has been minimized.

Copy link
Member Author

diml commented Apr 30, 2018

Actually it's just that the .globl _caml_startup wasn't enough to generate a reference to caml_startup. I changed the code to generate a global symbol named force_link_of_caml_startup that references caml_startup instead. It works when I test manually on OSX, I'm running precheck again with the new code.

@diml

This comment has been minimized.

Copy link
Member Author

diml commented Apr 30, 2018

precheck is happy. @shindere @xavierleroy This PR is ready for another round of review

@xavierleroy
Copy link
Contributor

xavierleroy left a comment

Looks good to me, with a possible simplification (see below).

(Cdata ([Cglobal_symbol name;
Cdefine_symbol name;
Csymbol_address "caml_startup"]))

This comment has been minimized.

@xavierleroy

xavierleroy Apr 30, 2018

Contributor

I suspect Csymbol_address "caml_startup" is enough, there is no need to give this piece of data a label force_link_of_caml_startup. But you are correct that a mere declaration Cglobal_symbol "caml_startup" is not enough, putting the symbol value into the data section is needed to convince the linker.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Apr 30, 2018

@diml

This comment has been minimized.

Copy link
Member Author

diml commented May 1, 2018

Ah, I see. It wasn't clear to me that this was a guard, I thought it was just a title. It would be good to get this to work on Windows indeed.

Regarding this PR, I did the simplification and re-run precheck, so it's good to merge.

@diml diml merged commit 1acaf6d into ocaml:trunk May 1, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented May 1, 2018

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented May 1, 2018

diml added a commit that referenced this pull request May 1, 2018

Generate a hard reference to "caml_startup" in Asmlink (#1740)
Without that, startup.o wasn't linked in object files generated by
-output-complete-obj on some platforms such as FreeBSD.
@diml

This comment has been minimized.

Copy link
Member Author

diml commented May 1, 2018

Agreed, @damiendoligez I cherry-picked this in 4.07 given that this is strictly a bug-fix.

diml added a commit that referenced this pull request May 1, 2018

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented May 1, 2018

@shindere - FlexDLL 0.36 includes an heuristic to convert -lfoo automatically for the MSVC ports (see alainfrisch/flexdll#32). Is that relevant to this scenario?

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented May 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.