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

-output-obj should support autolink #6797

Closed
vicuna opened this issue Feb 27, 2015 · 13 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link

commented Feb 27, 2015

Original bug ID: 6797
Reporter: @whitequark
Assigned to: @damiendoligez
Status: closed (set by @xavierleroy on 2017-02-16T14:18:15Z)
Resolution: fixed
Priority: normal
Severity: minor
Target version: 4.02.2+dev / +rc1
Fixed in version: 4.02.2+dev / +rc1
Category: ~DO NOT USE (was: OCaml general)
Tags: patch
Related to: #6625 #6733 #6796 #6918 #6929
Monitored by: @gasche

Bug description

As I've just realized, in #6733 I actually abused -output-obj: the compiler thought it was building an executable and thus invoked gcc and not ld -r ($PARTIALLD in Makefile.config). Thus it also linked in the runtime and most importantly the C stubs.

In fact if you try to use -output-obj as "intended", it doesn't link in the runtime, which is not too problematic, but also it doesn't link in the C stubs. It is unreasonable to expect the user to hunt down the C stubs manually and link them in explicitly, therefore I think -output-obj is completely broken.

File attachments

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 27, 2015

Comment author: @whitequark

It would seem as the same problem as when you don't pass -custom to ocamlc, however there is of course no ocamlopt -custom.

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 27, 2015

Comment author: @whitequark

It actually seems that ocamlc -output-obj suffers from the same problem.

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 27, 2015

Comment author: @whitequark

@gasche, I've attached a patch that fixes this. Specifically:

  • Any defined behavior is unchanged.
  • When -output-obj is used together with -runtime-variant, a previously nonsensical combination, both runtime and autolink libraries are linked inside the output object.

This also fixes issue 0006796 in the case where -runtime-variant is passed.

The remove_Wl function is necessary now because some packages (ctypes specifically) rely on the fact that the linker options are processed by C compiler driver (the gcc or clang binary, usually), and try to pass arguments to actual linker by escaping them via -Wl. However, partial linking, on which -output-obj relies, invokes the linker (the ld binary, usually) directly.

The remove_Wl function performs unescaping in the same way as the compiler driver would. Furthermore, it would be nonsensical to pass any options not recognized either directly by the driver (like -L) or escaped for passing to the linker (i.e. -Wl), so this should cover all reasonable behavior. Even if not, which is unlikely, this would not break any existing code.

Normally the user would pass -runtime_variant '', or, equivalently, when 0006733 is merged, put runtime_variant() in _tags.

The following line:
(if !Clflags.nopervasives || main_obj_runtime then "" else Config.native_c_libraries)
is needed because Config.native_c_libraries includes -ldl, which naturally has no static variant, and so linking that in would fail. It is reasonable (unlike with autolink) to require the host application to link this small, platform-specific, unchanging list of libraries.

Since the change is made to be noninvasive, I'd like to nominate it for 4.02.2.

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 27, 2015

Comment author: @damiendoligez

You need to protect the calls to String.sub by testing the length of cclib first.

Also, I don't understand how passing -runtime-variant '' would make any difference, as the empty string is already the default value.

@vicuna

This comment has been minimized.

Copy link
Author

commented Feb 27, 2015

Comment author: @whitequark

Oops. Okay, I need some other method to indicate "put the runtime and stubs into this object file". Suggestions? Introducing a separate flag just for this seems mildly fishy.

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 20, 2015

Comment author: @damiendoligez

I don't think it's a good idea to use -runtime-variant for this purpose.

You should probably introduce a new flag. How about -output-standalone-obj or -output-complete-obj?

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 20, 2015

Comment author: @whitequark

-output-complete-obj looks fine. Can we get this into 4.02.2?

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 25, 2015

Comment author: @whitequark

Patch updated to use -output-complete-obj and also extended to ocamlc for completeness.

@vicuna

This comment has been minimized.

Copy link
Author

commented Apr 27, 2015

Comment author: @damiendoligez

patch applied to 4.02 branch (rev 16054).

@vicuna

This comment has been minimized.

Copy link
Author

commented May 2, 2015

Comment author: @whitequark

Reopening because of a slight bug in ocamlc handlign (ocamlopt is fine), demonstrated by:

$ ocamlfind -toolchain android ocamlc -verbose -package sqlite3 -linkpkg t.ml -o foo.o -passopt -output-complete-obj
Effective set of compiler predicates: pkg_sqlite3,autolink,byte

  • /home/whitequark/.opam/4.02.1+32bit/arm-linux-androideabi/bin/ocamlc -verbose -o foo.o -output-complete-obj -I /home/whitequark/.opam/4.02.1+32bit/arm-linux-androideabi/lib/sqlite3 /home/whitequark/.opam/4.02.1+32bit/arm-linux-androideabi/lib/sqlite3/sqlite3.cma t.ml
  • /home/whitequark/.opam/4.02.1+32bit/lib/android-ndk/toolchains/arm-linux-androideabi-4.8/prebuilt/linux-x86/bin/arm-linux-androideabi-gcc --sysroot /home/whitequark/.opam/4.02.1+32bit/lib/android-ndk/platforms/android-17/arch-arm -I /home/whitequark/.opam/4.02.1+32bit/arm-linux-androideabi/include -L /home/whitequark/.opam/4.02.1+32bit/arm-linux-androideabi/lib -O2 -fno-defer-pop -Wall -D_FILE_OFFSET_BITS=64 -D_REENTRANT -fPIC -c '-I/home/whitequark/.opam/4.02.1+32bit/arm-linux-androideabi/lib/sqlite3' -I'/home/whitequark/.opam/4.02.1+32bit/arm-linux-androideabi/lib/ocaml' 'foo.c'
  • /home/whitequark/.opam/4.02.1+32bit/lib/android-ndk/toolchains/arm-linux-androideabi-4.8/prebuilt/linux-x86/bin/arm-linux-androideabi-ld --sysroot /home/whitequark/.opam/4.02.1+32bit/lib/android-ndk/platforms/android-17/arch-arm -r -o'foo.o' '-L/home/whitequark/.opam/4.02.1+32bit/arm-linux-androideabi/lib/sqlite3' '-L/home/whitequark/.opam/4.02.1+32bit/arm-linux-androideabi/lib/ocaml' '/tmp/camlobjc51d50.o' '-lsqlite3_stubs' '-lcamlrun'
    /home/whitequark/.opam/4.02.1+32bit/lib/android-ndk/toolchains/arm-linux-androideabi-4.8/prebuilt/linux-x86/bin/arm-linux-androideabi-ld: error: /tmp/camlobjc51d50.o: file is empty
    /home/whitequark/.opam/4.02.1+32bit/lib/android-ndk/toolchains/arm-linux-androideabi-4.8/prebuilt/linux-x86/bin/arm-linux-androideabi-ld: internal error in target, at /s/ndk-toolchain/src/build/../binutils/binutils-2.24/gold/parameters.h:105
    File "t.ml", line 1:
    Error: Error while building custom runtime system
    /home/whitequark/.opam/4.02.1+32bit/arm-linux-androideabi/bin/ocamlc returned with exit code 2

The fix is to change bytecomp/bytelink.c:576:

let c_file = basename ^ ".c"

to:

let c_file =
  if !Clflags.output_complete_object
  then Filename.temp_file "camlobj" ".c"
  else basename ^ ".c"
@vicuna

This comment has been minimized.

Copy link
Author

commented May 2, 2015

Comment author: @whitequark

After the fix it should also be forward-ported to trunk, I think.

@vicuna

This comment has been minimized.

Copy link
Author

commented May 6, 2015

Comment author: @damiendoligez

I applied your fix to 4.02 (rev 16095).

By default, this will be merged into trunk right after the release of 4.02.2. If you want it sooner, please say so.

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 11, 2015

Comment author: @alainfrisch

The -L options were not recognized by Microsoft linker, thus producing warnings when -pack'ing. I've changed them /libath:.

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.