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.

Sign up for GitHub

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

Jump to bottom

Protect link-time code generation placeholders with Sys.opaque_identity #3599

Merged
merged 1 commit into from
Jul 6, 2020
Merged

Protect link-time code generation placeholders with Sys.opaque_identity #3599

merged 1 commit into from
Jul 6, 2020

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Jul 1, 2020

This ensures that they are not inlined when using flambda. See discussion in #1930.

@emillon emillon requested a review from a user July 1, 2020 14:48
@emillon emillon force-pushed the ltcg-opaque-identity branch from 1293de3 to df10ba5 Compare July 1, 2020 14:50
@jberdine
Copy link
Contributor

jberdine commented Jul 1, 2020

I tested locally, and this leads to a correctly version-stamped executable. Note that warning 58 does still appear.

@jberdine jberdine mentioned this pull request Jul 1, 2020
Merged
@ghost
Copy link

ghost commented Jul 1, 2020

For warning 58, it could be a bug in flambda. Might be worth checking if we can reproduce this on a minimal example.

ghost
ghost suggested changes Jul 1, 2020
View reviewed changes
@emillon
Copy link
Collaborator Author

emillon commented Jul 1, 2020

In ocamlformat, Version.version gets compiled to an immediate string ("unknown"), so I suppose the Build_info_data module is somehow referenced but not linked because not necessary?

@ghost
Copy link

ghost commented Jul 1, 2020

@emillon looking at the log, warning 58 is emitted while compiling version.ml to version.cmx, so there is no linking involved at this point. One thing to check is that the cmi file for build_info_data is indeed compiled with -opaque. You can check that with the following code:

$ utop-full
# #require "compiler-libs";;
# (Cmi_format.read_cmi "/home/dim/.opam/default/lib/dune-build-info/build_info__Build_info_data.cmi").cmi_flags;;
- : Cmi_format.pers_flags list = [Cmi_format.Opaque; Cmi_format.Alerts <abstr>]

If it was compiled with -opaque, the constructor Cmi_format.Opaque should appear in the list.

@jberdine
Copy link
Contributor

jberdine commented Jul 1, 2020

It seems it was compiled with -opaque:

utop # (Cmi_format.read_cmi "/Users/jjb/ocamlformat/_opam/lib/dune-build-info/build_info__Build_info_data.cmi").cmi_flags;;
- : Cmi_format.pers_flags list = [Cmi_format.Opaque; Cmi_format.Alerts <abstr>]

@ghost
Copy link

ghost commented Jul 2, 2020

Alright, that seems like a bug in the compiler then. It should produce such a warning if the mli was indeed compiled with -opaque.

@emillon
Copy link
Collaborator Author

emillon commented Jul 3, 2020

I tried reproduce that on several compiler versions. Two interesting bits:

  • this only affects 4.10.0+flambda (4.09.1+flambda works as expected)
  • the fix is actually in dune, not dune-build-action, so if you use a new version of d-b-i and an old dune you get the bug

So I think we can just fix that whenever Sys.opaque_identity is available, that is for >= 4.03 and do nothing on 4.02.

@emillon emillon force-pushed the ltcg-opaque-identity branch from df10ba5 to 1879ee7 Compare July 3, 2020 12:08
@emillon emillon requested a review from a user July 3, 2020 12:09
@ghost
Copy link

ghost commented Jul 6, 2020

Agreed

ghost
ghost approved these changes Jul 6, 2020
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@emillon
Protect LTCG placeholders with Sys.opaque_identity
c15d651 
This ensures that they are not inlined when using flambda.
This problem is only present in OCaml 4.10.0, so it is fine not to patch
on 4.02 where `Sys.opaque_identity` is not available.
See discussion in ocaml#1930.

Signed-off-by: Etienne Millon <[email protected]>
@emillon emillon force-pushed the ltcg-opaque-identity branch from 1879ee7 to c15d651 Compare July 6, 2020 09:37
@emillon emillon merged commit 2d8398f into ocaml:master Jul 6, 2020
@emillon emillon deleted the ltcg-opaque-identity branch July 6, 2020 09:39
@emillon emillon mentioned this pull request Jul 8, 2020
Open
@ghost ghost mentioned this pull request Jul 27, 2020
Closed
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Aug 14, 2020
@rgrinberg
[new release] dune-build-info, dune, dune-configurator, dune-action-p…
545d8e0 
…lugin, dune-private-libs and dune-glob (2.7.0)

CHANGES:

- Write intermediate files in a `.mdx` folder for each `mdx` stanza
  to prevent the corresponding actions to be executed as part of the `@all`
  alias (ocaml/dune#3659, @NathanReb)

- Read Coq flags from `env` (ocaml/dune#3547 , fixes ocaml/dune#3486, @gares)

- Allow bisect_ppx to be enabled/disabled via dune-workspace. (ocaml/dune#3404,
  @stephanieyou)

- Formatting of dune files is now done in the executing dune process instead of
  in a separate process. (ocaml/dune#3536, @nojb)

- Add a `--debug-artifact-substution` flag to help debug problem with
  version not being captured by `dune-build-info` (ocaml/dune#3589,
  @jeremiedimino)

- Allow the use of the `context_name` variable in the `enabled_if` fields of
  executable(s) and install stanzas. (ocaml/dune#3568, fixes ocaml/dune#3566, @voodoos)

- Fix compatibility with OCaml 4.12.0 when compiling empty archives; no .a file
  is generated. (ocaml/dune#3576, @dra27)

- `$ dune utop` no longer tries to load optional libraries that are unavailable
  (ocaml/dune#3612, fixes ocaml/dune#3188, @anuragsoni)

- Fix dune-build-info on 4.10.0+flambda (ocaml/dune#3599, @emillon, @jeremiedimino).

- Allow multiple libraries with `inline_tests` to be defined in the same
  directory (ocaml/dune#3621, @rgrinberg)

- Run exit hooks in jsoo separate compilation mode (ocaml/dune#3626, fixes ocaml/dune#3622,
  @rgrinberg)

- Add (alias ...), (mode ...) fields to (copy_fields ...) stanza (ocaml/dune#3631, @nojb)

- (copy_files ...) now supports copying files from outside the workspace using
  absolute file names (ocaml/dune#3639, @nojb)

- Dune does not use `ocamlc` as an intermediary to call C compiler anymore.
  Configuration flags `ocamlc_cflags` and `ocamlc_cppflags` are always prepended
  to the compiler arguments. (ocaml/dune#3565, fixes ocaml/dune#3346, @voodoos)

- Revert the build optimization in ocaml/dune#2268. This optimization slows down building
  individual executables when they're part of an `executables` stanza group
  (ocaml/dune#3644, @rgrinberg)

- Use `{dev}` rather than `{pinned}` in the generated `.opam` file. (ocaml/dune#3647,
  @kit-ty-kate)

- Insert correct extension name when editing `dune-project` files. Previously,
  dune would just insert the stanza name. (ocaml/dune#3649, fixes ocaml/dune#3624, @rgrinberg)

- Fix crash when evaluating an `mdx` stanza that depends on unavailable
  packages. (ocaml/dune#3650, @craigfe)

- Fix typo in `cache-check-probablity` field in dune config files. This field
  now requires 2.7 as it wasn't usable before this version. (ocaml/dune#3652, @edwintorok)

- Add `"odoc" {with-doc}` to the dependencies in the generated `.opam` files.
  (ocaml/dune#3667, @kit-ty-kate)

- Do not allow user actions to capture dune's stdin (ocaml/dune#3677, fixes ocaml/dune#3672,
  @rgrinberg)

- `(subdir ...)` stanzas can now appear in dune files used via `(include ...)`.
  (ocaml/dune#3676, @nojb)
@rgrinberg rgrinberg mentioned this pull request Aug 14, 2020
Merged
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Aug 14, 2020
@rgrinberg
[new release] dune-build-info, dune, dune-configurator, dune-action-p…
fd24ec4 
…lugin, dune-private-libs and dune-glob (2.7.0)

CHANGES:

- Write intermediate files in a `.mdx` folder for each `mdx` stanza
  to prevent the corresponding actions to be executed as part of the `@all`
  alias (ocaml/dune#3659, @NathanReb)

- Read Coq flags from `env` (ocaml/dune#3547 , fixes ocaml/dune#3486, @gares)

- Add instrumentation framework to toggle instrumentation by `bisect_ppx`,
  `landmarks`, etc, via dune-workspace and/or the command-line. (ocaml/dune#3404, ocaml/dune#3526
  @stephanieyou, @nojb)

- Formatting of dune files is now done in the executing dune process instead of
  in a separate process. (ocaml/dune#3536, @nojb)

- Add a `--debug-artifact-substution` flag to help debug problem with
  version not being captured by `dune-build-info` (ocaml/dune#3589,
  @jeremiedimino)

- Allow the use of the `context_name` variable in the `enabled_if` fields of
  executable(s) and install stanzas. (ocaml/dune#3568, fixes ocaml/dune#3566, @voodoos)

- Fix compatibility with OCaml 4.12.0 when compiling empty archives; no .a file
  is generated. (ocaml/dune#3576, @dra27)

- `$ dune utop` no longer tries to load optional libraries that are unavailable
  (ocaml/dune#3612, fixes ocaml/dune#3188, @anuragsoni)

- Fix dune-build-info on 4.10.0+flambda (ocaml/dune#3599, @emillon, @jeremiedimino).

- Allow multiple libraries with `inline_tests` to be defined in the same
  directory (ocaml/dune#3621, @rgrinberg)

- Run exit hooks in jsoo separate compilation mode (ocaml/dune#3626, fixes ocaml/dune#3622,
  @rgrinberg)

- Add (alias ...), (mode ...) fields to (copy_fields ...) stanza (ocaml/dune#3631, @nojb)

- (copy_files ...) now supports copying files from outside the workspace using
  absolute file names (ocaml/dune#3639, @nojb)

- Dune does not use `ocamlc` as an intermediary to call C compiler anymore.
  Configuration flags `ocamlc_cflags` and `ocamlc_cppflags` are always prepended
  to the compiler arguments. (ocaml/dune#3565, fixes ocaml/dune#3346, @voodoos)

- Revert the build optimization in ocaml/dune#2268. This optimization slows down building
  individual executables when they're part of an `executables` stanza group
  (ocaml/dune#3644, @rgrinberg)

- Use `{dev}` rather than `{pinned}` in the generated `.opam` file. (ocaml/dune#3647,
  @kit-ty-kate)

- Insert correct extension name when editing `dune-project` files. Previously,
  dune would just insert the stanza name. (ocaml/dune#3649, fixes ocaml/dune#3624, @rgrinberg)

- Fix crash when evaluating an `mdx` stanza that depends on unavailable
  packages. (ocaml/dune#3650, @craigfe)

- Fix typo in `cache-check-probablity` field in dune config files. This field
  now requires 2.7 as it wasn't usable before this version. (ocaml/dune#3652, @edwintorok)

- Add `"odoc" {with-doc}` to the dependencies in the generated `.opam` files.
  (ocaml/dune#3667, @kit-ty-kate)

- Do not allow user actions to capture dune's stdin (ocaml/dune#3677, fixes ocaml/dune#3672,
  @rgrinberg)

- `(subdir ...)` stanzas can now appear in dune files used via `(include ...)`.
  (ocaml/dune#3676, @nojb)
emillon added a commit to emillon/ocamlformat that referenced this pull request Aug 25, 2020
@emillon
Use dune-build-info
fff0a61 
This uses dune-build-info to compute the version number.

It will be resolved in the following way:

- if (version) is set in (dune-project), it is used. This is what happens
  when using opam pins (through dune subst), or for released versions
  (through dune-release).
- otherwise, a description from [git describe] will be used. Caveat for
  this case: binaries under [_build/] will not have this information, but
  [dune install --prefix _install] will copy a valid binary under
  [_install/bin].

We require at least dune 2.7, since `dune-build-info` is broken on
flambda switches before this version.

See ocaml/dune#3599
emillon added a commit to emillon/ocamlformat that referenced this pull request Sep 9, 2020
@emillon
Use dune-build-info
5760f1f 
This uses dune-build-info to compute the version number.

It will be resolved in the following way:

- if (version) is set in (dune-project), it is used. This is what happens
  when using opam pins (through dune subst), or for released versions
  (through dune-release).
- otherwise, a description from [git describe] will be used. Caveat for
  this case: binaries under [_build/] will not have this information, but
  [dune install --prefix _install] will copy a valid binary under
  [_install/bin].

We require at least dune 2.7, since `dune-build-info` is broken on
flambda switches before this version.

See ocaml/dune#3599
emillon added a commit to ocaml-ppx/ocamlformat that referenced this pull request Sep 9, 2020
@emillon
Use dune-build-info (#1133)
66b6b3a 
This uses dune-build-info to compute the version number.

It will be resolved in the following way:

- if (version) is set in (dune-project), it is used. This is what happens
  when using opam pins (through dune subst), or for released versions
  (through dune-release).
- otherwise, a description from [git describe] will be used. Caveat for
  this case: binaries under [_build/] will not have this information, but
  [dune install --prefix _install] will copy a valid binary under
  [_install/bin].

We require at least dune 2.7, since `dune-build-info` is broken on
flambda switches before this version.

See ocaml/dune#3599
@jberdine jberdine mentioned this pull request Aug 16, 2022
Closed
jberdine added a commit to jberdine/dune that referenced this pull request Aug 20, 2022
@jberdine
fix(duild-info): reapply flambda fix from ocaml#3599 removed in ocaml…
542fdde 
…#5049

With flambda, Sys.opaque_identity is needed around the placeholder
string or else flambda is able to compile it away. This prevents the
rewriting done by e.g. `dune install` from working.
jberdine added a commit to jberdine/dune that referenced this pull request Aug 20, 2022
@jberdine
fix(duild-info): reapply flambda fix from ocaml#3599 removed in ocaml…
0bdea22 
…#5049

With flambda, Sys.opaque_identity is needed around the placeholder
string or else flambda is able to compile it away. This prevents the
rewriting done by e.g. `dune install` from working.

Signed-off-by: Josh Berdine <[email protected]>
jberdine added a commit to jberdine/dune that referenced this pull request Aug 20, 2022
@jberdine
fix(duild-info): reapply flambda fix from ocaml#3599 removed in ocaml…
cc114e8 
…#5049

With flambda, Sys.opaque_identity is needed around the placeholder
string or else flambda is able to compile it away. This prevents the
rewriting done by e.g. `dune install` from working.

Signed-off-by: Josh Berdine <[email protected]>
jberdine added a commit to jberdine/dune that referenced this pull request Aug 20, 2022
@jberdine
fix(duild-info): reapply flambda fix from ocaml#3599 removed in ocaml…
4a9d9a3 
…#5049

With flambda, Sys.opaque_identity is needed around the placeholder
string or else flambda is able to compile it away. This prevents the
rewriting done by e.g. `dune install` from working.

Signed-off-by: Josh Berdine <[email protected]>
rgrinberg pushed a commit that referenced this pull request Aug 20, 2022
@jberdine @rgrinberg
fix(duild-info): reapply flambda fix from #3599 removed in #5049
07fc48a 
With flambda, Sys.opaque_identity is needed around the placeholder
string or else flambda is able to compile it away. This prevents the
rewriting done by e.g. `dune install` from working.

Signed-off-by: Josh Berdine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jberdine jberdine jberdine left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@emillon @jberdine