Skip to content

Merge Flambda 2 .cmxa/.a files into new ocamloptcomp.cmxa/.a #141

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
Merged
merged 4 commits into from
Aug 5, 2021
Merged

Merge Flambda 2 .cmxa/.a files into new ocamloptcomp.cmxa/.a #141

merged 4 commits into from
Aug 5, 2021

Conversation

mshinwell
Copy link
Collaborator

This will avoid clients of compilerlibs having to explicitly link against the Flambda 2 archives. There is currently no tool for merging .cmxa files as far as I know, so I wrote one. This may need to be extended in the future to handle .cma files too.

@mshinwell mshinwell added the flambda2 Prerequisite for, or part of, flambda2 label Aug 5, 2021
@mshinwell mshinwell marked this pull request as draft August 5, 2021 11:30
@mshinwell mshinwell force-pushed the flambda2-artifact-installation branch from 1752159 to 86a7f51 Compare August 5, 2021 12:35
@mshinwell mshinwell force-pushed the flambda2-artifact-installation branch from 86a7f51 to 493da09 Compare August 5, 2021 13:34
@mshinwell mshinwell marked this pull request as ready for review August 5, 2021 14:34
@mshinwell mshinwell requested a review from xclerc August 5, 2021 14:45
@mshinwell mshinwell changed the title Merge Flambda 2 .cmxa files into new ocamloptcomp.cmxa Merge Flambda 2 .cmxa/.a files into new ocamloptcomp.cmxa/.a Aug 5, 2021
xclerc
xclerc reviewed Aug 5, 2021
View reviewed changes
xclerc
xclerc reviewed Aug 5, 2021
View reviewed changes
tools/merge_archives.ml
let magic =
really_input_string chan (String.length Config.cmxa_magic_number)
in
let (cmxa : Cmx_format.library_infos) = input_value chan in
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we should check the magic number here;
there is a consistency check in the function below, but
my fear is the unmarshaling of something with the wrong
shape (and the segfault that could follow).

Or is it written this way because you would like to be
compatible across versions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It deliberately doesn't check the magic number so it can be used across versions (it used to, and was problematic).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it has the potential of failing badly, but again this is for
power users so we can let them shoot themselves in the foot
since they are powerful enough to recover.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I see, because you mean that "library_infos" might have changed. I think this is probably ok for an internal build system tool.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that type or any one transitively used from it.
In the end I agree it is fine; the tool is simple enough so that
if we see an unexplained failure, it will come from the unmarshaling.

xclerc
xclerc reviewed Aug 5, 2021
View reviewed changes
xclerc
xclerc reviewed Aug 5, 2021
View reviewed changes
@xclerc
Copy link
Contributor

xclerc commented Aug 5, 2021

I understand this is an internal tool for power users, so I did not
pay attention to e.g. the order of ccobjs/ccopts, packs, and
other complications.

@mshinwell
Copy link
Collaborator Author

Regarding ordering: the .cmxa tool aims to preserve the ordering provided on the command line, in particular in case C compiler options are present where there is an ordering constraint. The script for merging .a files does preserve the order, which is important there.

@xclerc
Copy link
Contributor

xclerc commented Aug 5, 2021

I was a tiny bit worried about lib_ccobjs because it goes through a set,
but it also looks fine for the current use case.

@mshinwell
Copy link
Collaborator Author

lib_ccobjs deliberately uses a set, because it seems fine to dedup required object files, but it might not be fine to provide them duplicated to a linker (iirc). Although maybe if the user had provided all the .a files anyway (without using this tool) they would have ended up with the same error...

@mshinwell mshinwell force-pushed the flambda2-artifact-installation branch from ea24c50 to 77d2a80 Compare August 5, 2021 16:07
@mshinwell mshinwell merged commit 051aa43 into ocaml-flambda:main Aug 5, 2021
mshinwell added a commit that referenced this pull request Aug 6, 2021
@mshinwell
Merge Flambda 2 .cmxa/.a files into new ocamloptcomp.cmxa/.a (#141)
133e64f
ccasin added a commit that referenced this pull request Mar 24, 2023
@ccasin
Squashed 'ocaml/' changes from bcae5ff..ea89813
af4c6c2 
ea89813 Merge pull request #154 from ocaml-flambda/merge-flambda-backend
23cf5a5 Merge flambda-backend changes
b3af0c4 Unboxed types version 3 tests (#82)
1282d16 Functions with no clauses aren't local-returning (#149)
15d38c0 `make install` puts ocamlc at workspace root (#152)
b4928ee Remove -absname to improve build errors (#151)
f5b5e49 Remove `type_unpacks` (#150)
50d54db Remove arity-interrupting elaboration of module patterns (#146)
4382869 Remove the need to manually update the `tools/debug_printers` file (#148)
06a1d91 Add `promote-failed` targets to the Makefiles (#144)
d04eb58 Cleanup of comprehensions and immutable arrays (#127)
a45df79 Add a Module_strengthening extension (#142)
163c4b9 Add support for extensions in module types (#141)
74aa974 Some small patch-ups around matching on extensions (#140)
07127fe Remove raw_body from modular extensions setup (#137)
3f9bd64 Don't copy when resolving aliases in try_modtypes (#143)
aba6294 Immediacy rework (#122)
cf4eeef Add no-stack-allocation variant of some tests that print lambda (#133)
8f22438 Fully switch over Jane Street Merlin support to `.local-*` (#136)
5482a8d Remove `Lev_module_definition` from lambda (#135)
261e016 Change modular extensions to produce `AST_desc` types (#132)
0760c82 Disable module patterns in comprehensions (#131)
6acac80 Add Ctype global state debug printers (#130)
bc32037 Enable support for Jane Street's internal Merlin configuration (#64)
d1a8d03 Split out `Clflags.Extension` into a new file, `Language_extension` (#125)
435de6d Fix bootstrap and add legacy CI (#126)
7e5a626 Improve the API of language extensions to better support upstream compatibility (and also tooling) (#13)
c4e17b0 Replace var with local for faster mode checking (#53)
6d477d8 Merge pull request #124 from riaqn/merge-backend
d737533 minor fixes after merge
f1710d6 Merge flambda-backend changes
cc18534 Just run make boostrap (#123)

git-subtree-dir: ocaml
git-subtree-split: ea89813
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xclerc xclerc xclerc approved these changes

Assignees
No one assigned
Labels
flambda2 Prerequisite for, or part of, flambda2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mshinwell @xclerc