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

OCAMLPARAM Support for Disabling Position-Independent Code (PIC) Generation #6167

Closed
vicuna opened this issue Sep 10, 2013 · 8 comments
Closed

Comments

@vicuna
Copy link

vicuna commented Sep 10, 2013

Original bug ID: 6167
Reporter: pgj
Assigned to: @gasche
Status: closed (set by @xavierleroy on 2016-12-07T10:47:09Z)
Resolution: fixed
Priority: normal
Severity: feature
Version: 4.01.0+beta/+rc
Fixed in version: 4.03.0+dev / +beta1
Category: back end (clambda to assembly)
Tags: patch, junior_job
Monitored by: @gasche tgazagna @avsm

Bug description

Add support for disabling Position-Independent Code (PIC) generation through the OCAMLPARAM environment variable. This change would make it possible to specify a "nopic" token which, when set, instructs the native code generator to emit position-dependent code independently of what was requested at the command line.

The rationale of this option is similar to the other flags already supported by OCAMLPARAM: Allow the user to force generation of position-dependent code without the need for modifying build system files or sources.

This patch was directly motivated by the need for making cross-compilation possible of the Mirage unikernel [1] to the "kFreeBSD" backend. This backend basically compiles Mirage (OCaml) programs into FreeBSD kernel modules, where certain expectations must be satisfied. One of those expectations is that the generated code must not be position-independent as the FreeBSD kernel dynamic loader (kld(4)) does not support ELF relocations of that type. Hence PIC generation has to be disabled globally for all the compiled code, regardless what the original upstream packages were instructed for. The "nodynlink" flag is also used in this case.

The attached patch implements this functionality by moving the pic_code variable to the Clflags module for both the AMD64 and the ARM native code generator backends, while it retains the default settings for both. I have tested it on FreeBSD/amd64, but it should work on the ARM platform and on other operating systems.

[1] http://openmirage.org/

File attachments

@vicuna
Copy link
Author

vicuna commented Apr 2, 2014

Comment author: @damiendoligez

One small remark: if we add a "nopic" flag we should also add a "pic" flag for symmetry (and because all the hard work is already done).

@vicuna
Copy link
Author

vicuna commented Dec 21, 2014

Comment author: @whitequark

@gasche, can you please take a look at the attached patch?

@vicuna
Copy link
Author

vicuna commented Dec 21, 2014

Comment author: @gasche

It would be more natural, I think, to use pic=0 and pic=1 to disable or disable position-independent code. Given the fact that the key-value-handling logic is already present in the OCAMLPARAM code, the current patch allows nopic=1 and nopic=0 to respectively disable and enable position-independent code (with "nopic" being a shortcut for "nopic=1"), which is a rather obscure interface. If people prefer using "nopic" to "pic=0", we could at least support both pic and nopic as keys as Damien suggests.

The other issue I have with the patch is the default-setting logic:

+let pic_code = ref (String.compare "amd64" Config.architecture == 0)

  • (* -fPIC (only true by default on amd64) *)

if you allow me to nitpick (no pun intended), this code would not scale to the addition of other architectures. We should pattern-match on Config.architecture, with at least a case for each architecture on which the option makes sense (having a catch-all pattern with the default value for the others is reasonable).

Is anyone willing to improve the patch on these two aspects? Given the non-stellar reaction time, I would assume that the original contributor pgj may not be willing to do the extra work. If nobody comes forward by the time of the release, I could do it myself.

@vicuna
Copy link
Author

vicuna commented Dec 21, 2014

Comment author: pgj

Sorry, folks, I was busy with other stuff and I did not come to respond to the notes. I think you are right, and yes, I think I could adapt the patch according to the recommendations.

@vicuna
Copy link
Author

vicuna commented Dec 21, 2014

Comment author: @gasche

Nice! I certainly implied no criticism of you, I find it natural to could drift a bit after 6 months with no reply. You can take your time to prepare a patch.

@vicuna
Copy link
Author

vicuna commented Dec 21, 2014

Comment author: pgj

Sure, no problem. Thanks for reminding me :-) I did not have time for the original project either (for which the patch was originally written) so it simply got slipped, indeed.

@vicuna
Copy link
Author

vicuna commented Jan 17, 2015

Comment author: pgj

Please find a revisited version attached. It is now adapted to the trunk version as of today and theoretically has also the requested changes. I tested it on FreeBSD/amd64, it builds and works for me. Sorry for the delay :-)

@vicuna
Copy link
Author

vicuna commented Jan 24, 2015

Comment author: @gasche

Merged in trunk, thanks!

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