Check link order when creating archive and when using ocamlopt

[hhugo]

follow up on #12082.
Opening as a draft as tests are still missing.

We currently only check link ordering when linking bytecode executables.

• A bad ordering of objects when linking native binaries would result in No implementations provided for the following modules.
• A bad ordering of objects when creating archives (cma, cmxa) would only be detected later when trying to link such archives.

The PR proposes to detect wrong ordering in the 2 cases mentioned above.
The implementation of the check is shared in utils/linkcheck.ml

In addition,

• The check for missing module/implementation was duplicated in bytelink/aslink and is now shared in linkcheck.ml
• The check for duplicated implementation was duplicated in bytelink/aslink and is now shared in linkcheck.ml
• Error messages for missing implementation and Wrong_link_order have been improved a bit by always mentioning the file an implementation comes from.

$ ./boot/ocamlrun ./ocamlc -I stdlib a.cmo c.cmo b.cmo a.cmo
File "_none_", line 1:
Error: Wrong link order:  A (a.cmo), C (c.cmo) depends on B (b.cmo)

$ ./boot/ocamlrun ./ocamlc -I stdlib a.cmo c.cmo 
File "_none_", line 1:
Error: No implementations provided for the following modules:
         B referenced from A (a.cmo), C (c.cmo)

[hhugo]

@shindere, you seemed to be interested by reviewing this PR.

[shindere]

hhugo (2023/03/29 05:50 -0700):

@shindere, you seemed to be interested by reviewing this PR.

Yes, and still am. Apologies for having failed to do so until now. I'll do my best to do it soon but if somebody else can do it's fine, too.

[hhugo]

I've just rebased this PR on trunk.

[hhugo]

@shindere, I've rebased this PR on top of the recently merged #12031.

Would you have time for some review ?

[shindere]

hhugo (2023/06/19 03:35 -0700):

@shindere, I've rebased this PR on top of the recently merged #12031.

Ah many thanks!

Would you have time for some review ?

Yes. I apologize for the delay. I'll make my best to review it promptly.

[shindere]

Here are a few rather shallow remarks:

The Changes entry for this PR needs to be moved from the OCaml 5.1.0
section to the Working version section of the Changes file.

In utils/linkcheck.ml and utils/linkcheck.mli, shouldn't the
copyright be adjusted? It's new code, isn't it?

In utils/linkcheck.mli, I'd suggest to put the documentation comments
after rather than before the type or value they describe, as is done
e.g. in the standard library.

I would find it helpfl if utils/linkcheck.mli could include some
documentation about that the t type represents.

Also, the code refers to modname and globals. Given all the
discussion that happened in #12031, how many of those are actually
compunit? If they are, could you please reflect it in the code?

[hhugo]

The Changes entry for this PR needs to be moved from the OCaml 5.1.0 section to the Working version section of the Changes file.

Moved

In utils/linkcheck.ml and utils/linkcheck.mli, shouldn't the copyright be adjusted? It's new code, isn't it?

Done

In utils/linkcheck.mli, I'd suggest to put the documentation comments after rather than before the type or value they describe, as is done e.g. in the standard library.

Done

I would find it helpfl if utils/linkcheck.mli could include some documentation about that the t type represents.

I've added some comments

Also, the code refers to modname and globals. Given all the discussion that happened in #12031, how many of those are actually compunit? If they are, could you please reflect it in the code?

I think they are compunits (and sub-units when using-pack ?).
I've changed the code to reflect that better. I'm however unable to reuse the compunit type from cmo_format due to circular deps. Also note that cmx_format doesn't use the compunit type yet, which makes reusing this type less useful.

[hhugo]

@Octachron, I've rebased the PR above your #12210. You're using Style.as_inline_code Location.print_filename to print filenames witch reads weird to me as filename is not code. How would you format the filename in the message above ? Should I change it to be from "Lib" ("lib.cmo") ?

[Octachron]

My precise semantic for the tag is : technical incise phrase that does not belong grammatically to its surrounding. In other words, since lib.cmo appears within an English sentence but is not supposed to be read as an English word, it should be quoted.

[hhugo]

@shindere, @gasche, @smuenzel, I would welcome another round of review if you have time.

[shindere]

Very sorry about the delay, @hhugo. Apologies. Overall: many thanks for the documentation you have written. It made it significantly easier for me to get into the code. It seems to me that the Copyright header as you have written it in the linkcheck module is not yet good (it includes Xavier's name, for instance). Perhpas you can take inspiration from e.g. `ocamltest/main.ml`? In an earlier comment, I have suggested to depart from the `globals` terminology in your code. There are still leftovers, in the fields of Linkcheck's t type. I think all these globals should be compunits. I was also curious about why you use lists rather than sets in the interface of linkcheck? And also, for those types which are lists of lists I think more precise comments would be helpful to give an insight about what exactly these losts of lists represent. I also had big name discussions with myself but the two of me couldn't come to an agreement. I think `finish` could be better named, perhaps validate or check. I understand that this second choice would repeat the module name, and then I wondered whether the module could be renamed. I wish `consistency` wouldn't be such a long word, because apart from its length, a variation on `link_consistency` would have looked good to me. Or perhaps, what about `linkdeps`? Aren't link dependencies what we are dealing with, after all? Then you could rename `finish` into `check` without any name clash or redundancy. I also wondered why you didn't use the newly introduced `Compunit` type. Is it because that would have contaminated the native backend and you wanted to avoid that? Finally (for this round): is it mandatory to add all the units before checking for consistency? Especially, in completemode, can't you check for consistency as compunits get added? Or is it because the addition has to be done in reverse topological order as you documented?

[smuenzel]

@shindere, @gasche, @smuenzel, I would welcome another round of review if you have time.

I looked over it and it seems good to me (after addressing @shindere's questions)

[shindere]

Am I correct that some reviews have not yet been taken into acocunt, or is
this PR waiting for areview?

[hhugo]

Am I correct that some reviews have not yet been taken into acocunt, or is this PR waiting for areview?

I still need to look at @shindere 's last comments

[shindere]

hhugo (2023/09/11 03:24 -0700):

> Am I correct that some reviews have not yet been taken into acocunt, or is this PR waiting for areview? I still need to look at @shindere 's last comments

OK no problem, I just wanted to make sure this was not waiting for me without me knowing.

[hhugo]

I was also curious about why you use lists rather than sets in the
interface of linkcheck?

I guess I\ve used list because input data uses list (e.g. cu_required_compunits, cu_reloc).

I also wondered why you didn't use the newly introduced Compunit type.
Is it because that would have contaminated the native backend and you
wanted to avoid that?

That's exactly the reason, I didn't want to deal with such an invasive change. Also, my initial patch predate the merge of your Compunit PR.

Finally (for this round): is it mandatory to add all the units before
checking for consistency? Especially, in completemode, can't you check
for consistency as compunits get added? Or is it because the addition
has to be done in reverse topological order as you documented?

Checking at the very end is more convenient to report all issues (and not just the first one) and deduplicated them.

I'll try think about naming in the coming days

[hhugo]

@shindere, I've renamed the new file based on your suggestion. The PR should be ready for another round of review

[shindere]

hhugo (2023/10/01 12:59 -0700):

Checking at the very end is more convenient to report all issues (and not just the first one) and duplicated them.

You meant deduplicate them, didn't you?

[hhugo]

hhugo (2023/10/01 12:59 -0700):
Checking at the very end is more convenient to report all issues (and not just the first one) and duplicated them.
You meant deduplicate them, didn't you?

Yes, I edited my answer on GitHub after the fact

[shindere]

hhugo (2023/10/02 07:55 -0700):

@shindere, I've renamed the new file based on your suggestion. The PR should be ready for another round of review

Thanks. I'll make sure to review as soon as posible.

[shindere]

This now needs a rebase because of #12586.

The conflict should not be hard to resolve, though: add the line

  linkdeps.mli linkdeps.ml \

to the definition of utils_SOURCES in the root Makefile,
right after the consistbl.mli consistbl.ml \ line.

There is also a conflict in Changes and while fixing it you could
update the list of reviewers. ;-)

In linkdeps.mli, there's a typo in the docstring for create: returns and empty state should be returns an empty state. You may wantto add square brackets around true` in the smae docstirng.

The docstring for add also has a typo: [provides] are unit and sub-units provided by [compunit]: units, plural

I'm not sure the vertical alignment in the difinition of
compunit_and_source is what we want but @gasche and @Octachron may know
these things better. Generally speaking, I am calling for their eyes on this
PR.

The docstring for check could be reworded to avoid the negation:
once there are no more compilation units to add could be once all the compilaiton units (to be linked?) have been added.

The copyright in your linkdeps module is still not right, neither in the
interface, nor in theimplemnetaiton. See for instance utils/binutils.mli
and do exactly likewise.

In linkdeps.ml: bouth missing_compunit and provided_compunit should
use plurals, I think, especially given that badly_ordered_deps rightfully
does.

In the definition of check: I think missings is not a correct name
and it should be called missing (no s).

In the error reporting code, re:the message No implementations provided for the following modules, I'd tend to use a singular for implementation, although I do think
both singular and plural make sense.

Shouldn't the constructor Multiple_definition be named
Multiple_definitions (plural)? And how about rewordking
"@ @[<hov>Files %a@ define a module named %a@]" to say Multiple definitions of module ... in files? (I think you can removed the named).

In all the files where it appears, how aobut renaming the Linkdeps_error
constructor to just Link_error, or maybe Linking_error?

I am wondering about let lc = Linkdeps.create now that linkcheck has
been renamed to linkdeps. Perhaps lc should be renamed, too?

In the testsuite, I think
testsuite/tests/badly-ordered-deps/main.compilers.referenceshould be namedtestsuite/tests/badly-ordered-deps/main.native.reference` since it is
specific to the native compilers.

In the testfile: Make sure ocamlc and ocamlopt prints badly ordered dependencies only once: print, no s.

Why did you delete tests/lib-dynlink-initializers/test4_main.ml? Was that
on purpose? Same question for the other files that have been deleted?

[gasche]

I would be happy to approve on @smuenzel's behalf if/when he himself feels good enough about the current state to approve it for merging -- maybe he already does.

[gasche]

(I am assuming that this PR is not breaking programs that would work before, it only makes errors clearer and sometimes fails earlier. Is this indeed the case?)

[damiendoligez]

@hhugo the consensus among core devs seems to be that it would be a good idea to merge this PR once you've addressed @shindere 's remarks.

[xavierleroy]

I am assuming that this PR is not breaking programs that would work before, it only makes errors clearer and sometimes fails earlier. Is this indeed the case?

Very probably not. Unreferenced modules part of library files are removed before linking, so wrong dependencies in unreferenced modules don't cause errors today. With the proposed change, they will cause errors at library creation time. I think it's fine, but of course this is going to break existing code.

[hhugo]

Why did you delete tests/lib-dynlink-initializers/test4_main.ml? Was that
on purpose? Same question for the other files that have been deleted?

tests/lib-dynlink-initializers/test4_main.ml was deleted because it tests at runtime the behavior of loading a badly ordered library.
We can no longer create such library so the test cannot really run as expected (it fails too early).

Here is a comment from the deleted file:

(* Check that a module in a shared library cannot refer to another
   module in the same shared library if it has not yet been loaded. *)

[hhugo]

@shindere, I think I've finally addressed all your reviews. I've rebased and squashed all commits.
Thanks.

[shindere]

Many thnaks for having taken into account most of the suggestions. I write "most" because I believe that hte copyright header you have added to `utils/ml{,i}` is still not correct: there is no company name, whihc is minor, and I believe that the year you have written is wrong, too. However, I think the work is really nice and valuable and the PR has been here for several months now so I am going to approve and merge as it is so that we can move on and if you (or somebody else) wants to fix the copyright header that can be done later. Thanks again for the very nice work!

[lthls]

The compilation of Coccinelle is broken by this PR. The reason is that Coccinelle uses packs, but in a non-standard way: The internal libraries are built for a Coccinelle_modules pack, but the pack is only built for outside users. The binaries that are part of the Coccinelle sources are built by linking with the non-packed libraries.
This linking now fails because most modules have non-packed names in their ui_imports_cmx field (which is normal for modules inside the pack), but since they are supposed to be packed later their ui_defines field is the packed name.

I see two main ways to fix this:

• On the coccinelle side, link all internal executables with the packed library instead of the unpacked one. This doesn't require any action on our side, but it's hard to estimate the impact on the Coccinelle side (I expect at least bigger binaries).
• On our side, revert to using ui_name for the ~provides parameter to Linkdeps.add. This is a bit dubious, but I think it shouldn't break any normal use case. I will check that it does actually fix the compilation of Coccinelle, and propose a PR if it works.

[Octachron]

I have the impression that we could remove the pack prefix only when linking -for-pack cmx into executable, or maybe consider that the cmx provides both Pack$Module and Module?

[lthls]

I have the impression that we could remove the pack prefix only when linking -for-pack cmx into executable, or maybe consider that the cmx provides both Pack$Module and Module?

I don't think it is possible for another module to depend on Pack$Module. It is possible, for modules defined inside the same pack, to depend on module.cmx which defined Pack$Module, but in this case I believe the dependency is registered on Module, not Pack$Module (maybe we need to fix that ? Although there are some subtle interactions with -opaque).

Ideally I would like to make it clear that Coccinelle's use case is not supported, but it's hard to justify breaking something that has been working for so long.

[Octachron]

I can at least confirm that Octachron@1b9a4ef fixes the compilation of Coccinelle.

[lthls]

I don't think your patch goes into the right direction (I don't even understand why you only pass the omitted_pack parameter in Asmlink; it should be passed consistently to all the calls, in my opinion).
The issue here is a mismatch between what we declare as a dependency (a module name, basically) and what we provide (it used to be ui_name, the module name, but is now ui_defines, the list of compilation units packed in the given object). I would prefer a fix that either restores module names as keys for the consistency check (so pass [ui.ui_name] as the ~provides parameter), or switches to registering dependencies on compilation units (which would be the most consistent fix, but requires more work).

[Octachron]

My reasoning for my test was mostly that I wanted to minimally fix cocinelle use case without allowing to store pack fragments inside a cmxa and I was worried to handle the case of packed module that "provides" multiple modules.

However, since dependency doesn't traverse pack, I agree that using just [ui.ui_name] would be better: since pack siblings refer to each other without the pack prefix and the user of the pack will only depend on the pack itself.

Thus I agree that the real patch should just pass [ui.ui_name].

[Octachron]

@lthls , do you prefer to open or review the fix PR?

[lthls]

I would prefer reviewing.