Skip to content

Do not generate dummy code to force module linking#602

Merged
chambart merged 8 commits intoocaml:trunkfrom
chambart:required_globals
Jul 8, 2016
Merged

Do not generate dummy code to force module linking#602
chambart merged 8 commits intoocaml:trunkfrom
chambart:required_globals

Conversation

@chambart
Copy link
Copy Markdown
Contributor

@chambart chambart commented Jun 2, 2016

To ensure that a module is linked if only C externals are used from its interface, there is some dummy code generated referencing its top-level module.
This avoids it by propagating separately the required modules and registering it when needed.

This generated code is problematic for some other ongoing development.

~module_initializer:lam
in
let required_globals =
failwith "TODO: fill required_globals"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm guessing you're missing something here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oups...
Who cares about pack anywhay ? ;-)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In fact there is nothing to do in case of a pack

@chambart chambart force-pushed the required_globals branch 2 times, most recently from 785c615 to 39b95b9 Compare June 2, 2016 15:53
@garrigue
Copy link
Copy Markdown
Contributor

garrigue commented Jun 2, 2016

Good idea. This generated code was a hack anyway, I just could not find a simple way to do it.
(The number of modified files shows that this is not completely trivial.)
Have you seen whether this can be done for bytecode too?
But it's probably less problematic there.

@chambart
Copy link
Copy Markdown
Contributor Author

chambart commented Jun 3, 2016

The change is not so large, it's mainly interface propagation. And 8 files changes are test files.
I will take a look at bytecode and closure to see if it can be made a cleaner there too easily.
I have no idea what caused the appveyor failure, there are no log to help...

@chambart
Copy link
Copy Markdown
Contributor Author

chambart commented Jun 3, 2016

And here it is for bytecode and closure. The changes required for the bytecode version are a bit more invasive than the native one since there was already a field in cmx to record the information, but none for bytecode.

@chambart
Copy link
Copy Markdown
Contributor Author

chambart commented Jun 6, 2016

I forgot to bootstrap, hence the build failure.

@gasche In this case if we keep the blob out of the real change commit, there is no way to get a working intermediate state, hence breaking bisection. Would you prefer a squashed version of the patch containing both the change and the bootstrap here ?

@chambart chambart force-pushed the required_globals branch from 251f6f4 to 4b90065 Compare June 6, 2016 13:59
@gasche
Copy link
Copy Markdown
Member

gasche commented Jun 6, 2016

I think squashing would be fine if there is a well-defined commit in which to squash. Sometimes we do changes iteratively in ways that actually require doing several bootstraps in the intermediate states, and then it can be a bit cumbersome to completely preserve bisectability.

@bobot
Copy link
Copy Markdown
Contributor

bobot commented Jun 6, 2016

@chambart Could you indicate why you need to bootstrap in this particular case? Is it because the .cmi representation changed? I have the objective to require bootstrapping only if boot/ocamlc can't compile the compiler (eg. new language feature used in the compiler).

@chambart
Copy link
Copy Markdown
Contributor Author

chambart commented Jun 6, 2016

@bobot The cmo format changed. This should probably not require bootstraping.

The build without bootstrap is failing when building otherlibs which use the newly built compiler instead of the boot/ocamlc one, see: https://travis-ci.org/chambart/ocaml-1/jobs/134999150

@chambart
Copy link
Copy Markdown
Contributor Author

chambart commented Jun 8, 2016

Rebased to clean the history and make bisectable.

@chambart
Copy link
Copy Markdown
Contributor Author

chambart commented Jul 7, 2016

Rebased, there was some changes needed to merge with the recent location changes.

@garrigue, @xavierleroy, any objection ?

@garrigue
Copy link
Copy Markdown
Contributor

garrigue commented Jul 8, 2016

This looks ok to me.
The obvious difficulty is that this kind of things are hard to test, but I hope you've done enough checks.

chambart added 8 commits July 8, 2016 16:02
This also share the result type of transl_implementation_flambda
and transl_store_implementation
Adds the required_globals information to bytecode compilation units.

This patch also bootstrap ocamlc. The cmo format is changed by this
commit, there is no way around bootstraping here. Note that ocamldep and
ocamllex does not rely on the cmo format, so they are not present in
this commit.

Changes in tests:

* Update test/transprim/comparison_table.ml.reference:
    The (opaque (global List!)) expression is not present anymore

* Update tests/no-alias-deps/aliases.cmo.reference
    The output of objinfo changed
@chambart
Copy link
Copy Markdown
Contributor Author

chambart commented Jul 8, 2016

I tried some complex cases and built some subset of opam. In fact opam testing does not convince me that much, I don't think that exercise this a lot.

@chambart chambart force-pushed the required_globals branch from a8aa562 to a1c4cba Compare July 8, 2016 14:52
@chambart
Copy link
Copy Markdown
Contributor Author

chambart commented Jul 8, 2016

Rebased

@chambart chambart merged commit bd9d555 into ocaml:trunk Jul 8, 2016
camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017
Do not generate dummy code to force module linking
lukemaurer pushed a commit to lukemaurer/ocaml that referenced this pull request Jul 19, 2022
stedolan pushed a commit to stedolan/ocaml that referenced this pull request Sep 21, 2022
stedolan pushed a commit to stedolan/ocaml that referenced this pull request Sep 21, 2022
ce76e02 flambda-backend: Bugfix for type_application (ocaml#746)
44f3afb flambda-backend: PR580 for main branch (ocaml#743)
b851eaa flambda-backend: Backport first part of ocaml/ocaml PR10498 (ocaml#737)
fafb4bd flambda-backend: Fix return mode for eta-expanded function in type_argument (ocaml#735)
c31f6c3 flambda-backend: Fix treatment of functions called [@nontail] (ocaml#725)
847781e flambda-backend: Fix build_upstream post-PR703 (ocaml#712)
bfcbbf8 flambda-backend: Extend Pblock value kind to handle variants (ocaml#703)
b2cab95 flambda-backend: Merge ocaml-jst
a6d6e0e flambda-backend: Merge ocaml-jst
88a4f63 flambda-backend: Use Pmakearray for immutable arrays (ocaml#699)
eeaa44b flambda-backend: Install an ocamldoc binary (ocaml#695)
48d322b flambda-backend: Ensure that GC is not invoked from bounds check failures (ocaml#681)
4370fa1 flambda-backend: Review changes of term directory (ocaml#602)
65a4566 flambda-backend: Add code coverage using bisect_ppx (ocaml#352)
63ab65f flambda-backend: Bugfix for primitive inclusion (ocaml#662)
7e3e0c8 flambda-backend: Fix inclusion checks for primitives (ocaml#661)
96c68f9 flambda-backend: Speed up linking by changing cmxa format (ocaml#607)
1829150 flambda-backend: Bugfix for Translmod.all_idents (ocaml#659)

git-subtree-dir: ocaml
git-subtree-split: ce76e02
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
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.

5 participants