Protect link-time code generation placeholders with Sys.opaque_identity by emillon · Pull Request #3599 · ocaml/dune

emillon · 2020-07-01T14:48:21Z

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

jberdine · 2020-07-01T15:04:02Z

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

ghost · 2020-07-01T15:16:12Z

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

src/dune/link_time_code_gen.ml

emillon · 2020-07-01T15:17:59Z

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 · 2020-07-01T16:21:14Z

@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 · 2020-07-01T21:00:16Z

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 · 2020-07-02T09:14:08Z

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 · 2020-07-03T10:19:27Z

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.

ghost · 2020-07-06T09:05:26Z

Agreed

ghost

Looks good!

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]>

@NathanReb

…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)

@NathanReb

…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)

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

…#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.

…#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]>

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]>

emillon requested a review from a user July 1, 2020 14:48

emillon force-pushed the ltcg-opaque-identity branch from 1293de3 to df10ba5 Compare July 1, 2020 14:50

jberdine mentioned this pull request Jul 1, 2020

Use dune-build-info to compute version number ocaml-ppx/ocamlformat#1133

Merged

ghost suggested changes Jul 1, 2020

View reviewed changes

src/dune/link_time_code_gen.ml Outdated Show resolved Hide resolved

emillon force-pushed the ltcg-opaque-identity branch from df10ba5 to 1879ee7 Compare July 3, 2020 12:08

emillon requested a review from a user July 3, 2020 12:09

ghost approved these changes Jul 6, 2020

View reviewed changes

emillon force-pushed the ltcg-opaque-identity branch from 1879ee7 to c15d651 Compare July 6, 2020 09:37

emillon merged commit 2d8398f into ocaml:master Jul 6, 2020

emillon deleted the ltcg-opaque-identity branch July 6, 2020 09:39

emillon mentioned this pull request Jul 8, 2020

Test "github660" does not pass with flambda #3618

Open

ghost mentioned this pull request Jul 27, 2020

Dependency between generated dune-build-info file and clients #3277

Closed

rgrinberg mentioned this pull request Aug 14, 2020

[new release] dune-build-info, dune, dune-configurator, dune-action-plugin, dune-private-libs and dune-glob (2.7.0) ocaml/opam-repository#17005

Merged

jberdine mentioned this pull request Aug 16, 2022

Regression of dune-build-info with flambda #6075

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Protect link-time code generation placeholders with Sys.opaque_identity#3599
emillon merged 1 commit intoocaml:masterfrom
emillon:ltcg-opaque-identity

emillon commented Jul 1, 2020

Uh oh!

jberdine commented Jul 1, 2020

Uh oh!

ghost commented Jul 1, 2020

Uh oh!

Uh oh!

emillon commented Jul 1, 2020

Uh oh!

ghost commented Jul 1, 2020

Uh oh!

jberdine commented Jul 1, 2020

Uh oh!

ghost commented Jul 2, 2020

Uh oh!

emillon commented Jul 3, 2020

Uh oh!

ghost commented Jul 6, 2020

Uh oh!

ghost left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

emillon commented Jul 1, 2020

Uh oh!

jberdine commented Jul 1, 2020

Uh oh!

ghost commented Jul 1, 2020

Uh oh!

Uh oh!

emillon commented Jul 1, 2020

Uh oh!

ghost commented Jul 1, 2020

Uh oh!

jberdine commented Jul 1, 2020

Uh oh!

ghost commented Jul 2, 2020

Uh oh!

emillon commented Jul 3, 2020

Uh oh!

ghost commented Jul 6, 2020

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants