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

Fix cinaps on arm32 #4081

Closed
wants to merge 1 commit into from
Closed

Fix cinaps on arm32 #4081

wants to merge 1 commit into from

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Jan 5, 2021

Closes #4069

When dynlink:false is passed, the ARM backend emits MOVW/MOVT instructions which have relocations incompatible with PIC code.

This is similar to #2527 (ocaml issue: ocaml/ocaml#8867).

Closes ocaml#4069

When `dynlink:false` is passed, the ARM backend emits MOVW/MOVT
instructions which have relocations incompatible with PIC code.
This is similar to ocaml#2527 (ocaml issue: ocaml/ocaml#8867).

Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Copy link
Collaborator Author

emillon commented Jan 5, 2021

The fix is just for cinaps but it looks like all places that pass dynlink:false to Compilation_context.create might be affected, for example inline tests.

#2527 has a discussion about whether to pass -nodynlink or not:

  • nodynlink prevents creation of PIE executables
  • most distributions are moving to PIE (for security reasons)
  • the optimisation to use dynlink only if necessary is "commented out" (true || ...) in normal executables:
    let dynlink =
    (* See https://github.com/ocaml/dune/issues/2527 *)
    true
    || Dune_file.Executables.Link_mode.Map.existsi exes.modes
    ~f:(fun mode _loc ->
    match mode with
    | Other { kind = Shared_object; _ } -> true
    | _ -> false)

So I think it's worth wondering if we should just remove this mechanism (and the ~dynlink argument) and never pass -nodynlink. One thing I'm not completely sure is how it interacts with the possibility of creating static executables, but I believe this works at the moment even with this bypassed optimisation. Thoughts @jeremiedimino ?

This part would be done in a separate PR of course, this one is enough to fix the cinaps issue.

@emillon emillon mentioned this pull request Jan 5, 2021
5 tasks
voodoos added a commit to voodoos/dune that referenced this pull request Jan 5, 2021
@emillon: See ocaml#4081 - this might not work on arm32 otherwise.

Co-authored-by: Etienne Millon <etienne.millon@gmail.com>
voodoos added a commit to voodoos/dune that referenced this pull request Jan 5, 2021
@emillon: See ocaml#4081 - this might not work on arm32 otherwise.

Co-authored-by: Etienne Millon <etienne.millon@gmail.com>
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@rgrinberg
Copy link
Member

Yes, let's just disable this option everywhere until it's fixed with PIE. If you want to future proof things, you can add this value for dynlink:

type dynlink =
  | Enable
  | Disable_if_possible

This way we'll just need to update Disable_if_possible whenever OCaml fixes this option.

@rgrinberg
Copy link
Member

Actually, I'm not sure if nodynlink is ever going to be fixed. So perhaps we should just get rid of it and re-evaluate later.

@emillon
Copy link
Collaborator Author

emillon commented Jan 6, 2021

@rgrinberg I've just opened #4085 which removes the -nodynlink handling. It supersedes this PR but in terms of release engineering we can merge this (#4081) first so that it is backported more easily to a point release on 2.7.x.

@ghost
Copy link

ghost commented Jan 11, 2021

I know that at Jane Street we have been using -nodynlink historically because it provided a noticeable speedup. I don't have up to date numbers so the speedup might be less now. Never passing -nodynlink by default seems fine, but it might be worth adding a workspace option to allow passing -nodynlink when possible (i.e. as we do now), for people who want the extra speedup.

@emillon
Copy link
Collaborator Author

emillon commented Jan 11, 2021

We don't do any optimization for normal executables (the ones produced by executable stanzas) at the moment - the piece of code linked in #4081 (comment) is equivalent to let dynlink = true. So removing that support does not change how dune links executables.

Do you mean adding a way to remove the true || part?

@ghost
Copy link

ghost commented Jan 11, 2021

Indeed, I forgot about this. Never-mind then. We might indeed want to allow this optimisation at Jane Street in the future, but we can cross that bridge when we come to it. In the meantime #4805 seems good.

@emillon
Copy link
Collaborator Author

emillon commented Jan 11, 2021

Thanks! I just merged it to master. This one might be useful if we cut a 2.7.x point release but I'm closing it in the meantime.

@rgrinberg
Copy link
Member

I created #4095 to track this.

voodoos added a commit to voodoos/dune that referenced this pull request Jan 18, 2021
@emillon: See ocaml#4081 - this might not work on arm32 otherwise.

Co-authored-by: Etienne Millon <etienne.millon@gmail.com>
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@emillon emillon deleted the cinaps-arm32 branch July 25, 2023 09:22
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.

Cinaps stanza 1.0 fails to build the cinaps executable under arm32
2 participants