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

RFE: build libasmrun_shared too #6693

Closed
vicuna opened this issue Dec 6, 2014 · 34 comments
Closed

RFE: build libasmrun_shared too #6693

vicuna opened this issue Dec 6, 2014 · 34 comments
Assignees
Milestone

Comments

@vicuna
Copy link

vicuna commented Dec 6, 2014

Original bug ID: 6693
Reporter: Richard Jones
Assigned to: @gasche
Status: closed (set by @xavierleroy on 2017-02-16T14:15:13Z)
Resolution: fixed
Priority: normal
Severity: minor
Platform: amd64
OS: Linux
Target version: 4.02.2+dev / +rc1
Fixed in version: 4.02.2+dev / +rc1
Category: runtime system and C interface
Tags: patch
Related to: #6733 #6845
Monitored by: @whitequark @gasche @ygrek @hcarty

Bug description

I want to routinely build Linux .so files containing OCaml code. This can be done, see Gerd's instructions here:

http://www.camlcity.org/knowledge/kb_002_shared_library.html

but it requires that you recompile OCaml with -fPIC, otherwise you get this error:

/usr/bin/ld: /usr/lib64/ocaml/libasmrun.a(startup.o): relocation R_X86_64_32 against `.rodata.str1.1' can not be used when making a shared object; recompile with -fPIC

So it would be nice if this just worked out of the box. I think by analogy to libcamlrun_shared.so, there should be libasmrun_shared.so, and ocamlopt should select '-lasmrun_shared' when asked to build a .so file.

This has been requested multiple times on the mailing list:

http://caml.inria.fr/pub/ml-archives/caml-list/2009/12/19e3dc20e585bd216f200789cbea922a.en.html
https://groups.google.com/d/topic/fa.caml/T6blKLDFDog/discussion
http://caml.inria.fr/pub/ml-archives/caml-list/2009/12/7a299d294aa5b8152ce26a18eb03a2dc.en.html

File attachments

@vicuna
Copy link
Author

vicuna commented Dec 6, 2014

Comment author: @yallop

I agree that this would be useful. Ctypes provides facilities for building shared objects from OCaml libraries, and it'd be better if the instructions for doing so didn't have to begin with "First, recompile OCaml with -fPIC" ...

Example here:

https://github.com/yallop/ocaml-ctypes-inverted-stubs-example/

@vicuna
Copy link
Author

vicuna commented Dec 6, 2014

Comment author: Richard Jones

My specific need stems from writing nbdkit plugins in OCaml:

libguestfs/nbdkit@e3abac4

@vicuna
Copy link
Author

vicuna commented Dec 8, 2014

Comment author: @mshinwell

We must sort this out. I will see what I can do.

@vicuna
Copy link
Author

vicuna commented Dec 29, 2014

Comment author: @whitequark

I've attached a patch that builds libasmrun_shared.so like libcamlrun_shared.so. It also builds libasmrun_pic.a and libcamlrun_pic.a, since in many cases it is inconvenient to refer to a shared library that is not in default library search path.

@vicuna
Copy link
Author

vicuna commented Jan 3, 2015

Comment author: @gasche

Would the "people that know this stuff" give an opinion on the proposed patch? It's a dependency to get the ocamlbuild-related patch in #6733 (which I'm ready to merge) to work properly.

@vicuna
Copy link
Author

vicuna commented Jan 5, 2015

Comment author: @mshinwell

I will have a look at it, when I can find a moment...

@vicuna
Copy link
Author

vicuna commented Jan 28, 2015

Comment author: @whitequark

Ping? This is needed for #6733...

@vicuna
Copy link
Author

vicuna commented Feb 5, 2015

Comment author: @mshinwell

I had a look for two minutes, and I spotted two things.

  1. It appears that PICFLAGS does not include -O. Is this intentional?

  2. Why does e.g. the implicit rule for %.pic.o include ASPPPROFFLAGS? Those are for profiling. (Should we build shared library versions of the profiling-enabled libraries too? I tend to think that there's no point for Linux since perf is almost certainly more useful than gprof---but for other platforms, this argument may not hold.)

@vicuna
Copy link
Author

vicuna commented Feb 5, 2015

Comment author: @whitequark

  1. -O added.
  2. ASPPPROFFLAGS removed.

I think we can add profiling-enabled shared libraries when someone needs them.

@vicuna
Copy link
Author

vicuna commented Feb 25, 2015

Comment author: @whitequark

Ping?

@vicuna
Copy link
Author

vicuna commented Feb 25, 2015

Comment author: @mshinwell

Pong. (I will look at this when I have a moment.)

@vicuna
Copy link
Author

vicuna commented Mar 6, 2015

Comment author: @whitequark

Ping?

@vicuna
Copy link
Author

vicuna commented Mar 15, 2015

Comment author: @avsm

A minor nit on this patch; SHARED is quite a general variable name to go in the top-level Makefile. Would something like SHARED_LIBS or SHARED_LIBS_MODE to reflect the purpose of the variable be clearer?

@vicuna
Copy link
Author

vicuna commented May 2, 2015

Comment author: @gasche

The patch is slightly wrong as there are dangling ; fi in the byterun/Makefile:install-shared target, but that is easy to fix.

@vicuna
Copy link
Author

vicuna commented May 2, 2015

Comment author: @gasche

There was another minor problem with the patch:
libcamlrun_pic.a: $(OBJS)
should be
libcamlrun_pic.a: $(PICOBJS)

@vicuna
Copy link
Author

vicuna commented May 2, 2015

Comment author: @gasche

Merged in 4.02 and trunk, thanks!

@vicuna
Copy link
Author

vicuna commented May 2, 2015

Comment author: @whitequark

Thank you for the review!

@vicuna
Copy link
Author

vicuna commented May 2, 2015

Comment author: @gasche

There seems to be an issue on MacOSX, with the failure message shown below from out continuous integration test. If you have ideas of how to fix it, I can apply them right now, otherwise I may have to revert the patch and let someone with access to a test machine handle it.

[...]

gcc -c -I../byterun -DCAML_NAME_SPACE -DNATIVE_CODE -DTARGET_amd64 -DSYS_macosx -O -D_FILE_OFFSET_BITS=64 -D_REENTRANT -o debugger.pic.o debugger.c
gcc -c -I../byterun -DCAML_NAME_SPACE -DNATIVE_CODE -DTARGET_amd64 -DSYS_macosx -O -D_FILE_OFFSET_BITS=64 -D_REENTRANT -o meta.pic.o meta.c
gcc -c -I../byterun -DCAML_NAME_SPACE -DNATIVE_CODE -DTARGET_amd64 -DSYS_macosx -O -D_FILE_OFFSET_BITS=64 -D_REENTRANT -o dynlink.pic.o dynlink.c
clang -arch x86_64 -c -DSYS_macosx -DMODEL_default -o amd64.pic.o amd64.S
rm -f libasmrun_pic.a
ar rc libasmrun_pic.a startup.pic.o main.pic.o fail.pic.o roots.pic.o globroots.pic.o signals.pic.o signals_asm.pic.o misc.pic.o freelist.pic.o major_gc.pic.o minor_gc.pic.o memory.pic.o alloc.pic.o compare.pic.o ints.pic.o floats.pic.o str.pic.o array.pic.o io.pic.o extern.pic.o intern.pic.o hash.pic.o sys.pic.o parsing.pic.o gc_ctrl.pic.o terminfo.pic.o md5.pic.o obj.pic.o lexing.pic.o printexc.pic.o callback.pic.o weak.pic.o compact.pic.o finalise.pic.o custom.pic.o unix.pic.o backtrace.pic.o natdynlink.pic.o debugger.pic.o meta.pic.o dynlink.pic.o amd64.pic.o
ranlib libasmrun_pic.a
gcc -bundle -flat_namespace -undefined suppress -Wl,-no_compact_unwind -o libasmrun_shared.so startup.pic.o main.pic.o fail.pic.o roots.pic.o globroots.pic.o signals.pic.o signals_asm.pic.o misc.pic.o freelist.pic.o major_gc.pic.o minor_gc.pic.o memory.pic.o alloc.pic.o compare.pic.o ints.pic.o floats.pic.o str.pic.o array.pic.o io.pic.o extern.pic.o intern.pic.o hash.pic.o sys.pic.o parsing.pic.o gc_ctrl.pic.o terminfo.pic.o md5.pic.o obj.pic.o lexing.pic.o printexc.pic.o callback.pic.o weak.pic.o compact.pic.o finalise.pic.o custom.pic.o unix.pic.o backtrace.pic.o natdynlink.pic.o debugger.pic.o meta.pic.o dynlink.pic.o amd64.pic.o
ld: illegal text reloc in '_caml_start_program' to '_caml_program' for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[4]: *** [libasmrun_shared.so] Error 1
make[3]: *** [makeruntimeopt] Error 2
make[2]: *** [opt-core] Error 2
make[1]: *** [opt.opt] Error 2
make: *** [world.opt] Error 2

@vicuna
Copy link
Author

vicuna commented May 2, 2015

Comment author: @whitequark

The crux of the issue is that GCALL(_caml_start_program)(%rip) (unlike, say, GCALL(caml_apply1)(%rip), which works), is an actual unresolved reference when compiled into a shared object, and thus it would have to be updated by dyld on startup. dyld normally refuses to update references in __TEXT; on i386, this can be ameliorated by passing -read_only_relocs suppress to ld, but on x86_64, this is a hard error.

Let me see how it can be fixed.

@vicuna
Copy link
Author

vicuna commented May 2, 2015

Comment author: @whitequark

Correction: the references to caml_apply were also invalid. I'm attaching a patch that fixes this. As far as I can see this code was never 'correct', you shouldn't use a relocation mode intended for calls with lea.

@vicuna
Copy link
Author

vicuna commented May 2, 2015

Comment author: @whitequark

A new version of the patch attached that doesn't break Windows.

@vicuna
Copy link
Author

vicuna commented May 2, 2015

Comment author: @gasche

With the proposed patch, ./ocamlc.opt segfaults on my machine (and the testsuite breaks).

I reverted the change from 4.02. We might be able to fix things in trunk, but I'm not qualified to decide whether it can go in the stable branch.

@vicuna
Copy link
Author

vicuna commented May 2, 2015

Comment author: @whitequark

Ok, I completely forgot that the @GOTPCREL entry just contains the address of the global (i.e. function in this case) but @plt entry contains executable code that jumps onto the function entry point. Thus we need to remove a level of indirection. I will attach a patch in a moment.

@vicuna
Copy link
Author

vicuna commented May 2, 2015

Comment author: @whitequark

Patch updated.

@vicuna
Copy link
Author

vicuna commented May 2, 2015

Comment author: @gasche

The new patch fixes the build failure, thanks.

@vicuna
Copy link
Author

vicuna commented May 2, 2015

Comment author: @whitequark

It only uses a technique (LEA_VAR) that is already applied elsewhere, so in my opinion it should be safe to apply to both trunk and stable. The choice is yours, of course.

@vicuna
Copy link
Author

vicuna commented May 5, 2015

Comment author: @jhjourdan

The second patch seems OK to me.

Notice it has a slightly different behaviour, because LEA_VAR loads the actual address of the function being called, while leaq GCALL(..%PLT) loads the entry in the PLT.

It would be really nice to document properly this feature.

Concerning portability, the only architecture where the runtime contains PIC code is arm64, but the compiler backend does not generate PIC code for arm64, which make this kind of useless. Analogously, the arm backend can produce PIC code, but its runtime doesn't seem to be PIC compatible... Clarification needed.

@vicuna
Copy link
Author

vicuna commented May 5, 2015

Comment author: @damiendoligez

Let me ask for Xavier's opinion before we apply this patch to 4.02.

@vicuna
Copy link
Author

vicuna commented May 6, 2015

Comment author: @xavierleroy

The grel.patch and its use of LEA_VAR look correct to me. gcc -fPIC also generates a movq from a GOTPCREL relocation in this case (taking a pointer to an external function).

@vicuna
Copy link
Author

vicuna commented May 6, 2015

Comment author: @mshinwell

I also think the grel.patch bugfix is correct. I'm a bit rusty on these topics, but if I understand correctly, the visible semantics shouldn't have changed: originally (with @plt) we would have taken the address of a PLT trampoline that branched immediately through a particular GOT entry (which in turn would call the lazy resolution stub, or branch to the function itself if it had already been resolved); now, we obtain the address from the same GOT entry directly by using a %rip-relative load---the offset being calculated by the (static) linker when dealing with the GOTPCREL relocation.

We should probably test on more architectures to ensure the libasmrun_shared support works correctly, or is documented not to work. I may be able to help for SPARC and POWER, since I need to find suitable machines to test another patch.

I've applied the patch for the relocations to the 4.02 branch.

I spoke to Damien and he agreed that we can un-revert the main patch. Gabriel, are you able to do this? I looked at "svn diff -r 16071:16072" and it looks like some other changes might have got caught up in your reversion (maybe only in "Changes")?

@vicuna
Copy link
Author

vicuna commented May 6, 2015

Comment author: @gasche

I will unrevert, but unless this is very urgent I will wait for the next week-end to do it.

@vicuna
Copy link
Author

vicuna commented May 10, 2015

Comment author: @gasche

Merged in 4.02 again.

@vicuna
Copy link
Author

vicuna commented Jun 20, 2015

Comment author: Richard Jones

Just a note: I have enabled OCaml-based plugins in nbdkit using this work. Everything appears to work fine.

@vicuna
Copy link
Author

vicuna commented Jun 20, 2015

Comment author: @yallop

It works for me, too:

yallop/ocaml-ctypes-inverted-stubs-example@54e0748e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants