Add an early error when compiling different modules with mismatching -for-pack #1391
Conversation
asmcomp/cmx_format.mli
Outdated
| @@ -44,7 +44,8 @@ type unit_infos = | |||
| mutable ui_apply_fun: int list; (* Apply functions needed *) | |||
| mutable ui_send_fun: int list; (* Send functions needed *) | |||
| mutable ui_export_info: export_info; | |||
| mutable ui_force_link: bool } (* Always linked *) | |||
| mutable ui_force_link: bool; (* Always linked *) | |||
| mutable ui_for_pack: string option } (* was it built with -for-pack ? *) | |||
There was a problem hiding this comment.
This needs a magic number change, I think.
asmcomp/compilenv.ml
Outdated
| @@ -205,6 +208,11 @@ let get_global_info global_ident = ( | |||
| let (ui, crc) = read_unit_info filename in | |||
| if ui.ui_name <> modname then | |||
| raise(Error(Illegal_renaming(modname, ui.ui_name, filename))); | |||
| (match ui.ui_for_pack, current_unit.ui_for_pack with | |||
There was a problem hiding this comment.
Can you add a comment explaining why this check isn't symmetric (between [ui.ui_for_pack] and [current_unit.ui_for_pack])?
asmcomp/compilenv.ml
Outdated
| fprintf ppf "%a@ was built with -for-pack %s, but this module isn't" | ||
| Location.print_filename filename pack_1 | ||
| | Mismatching_for_pack(filename, pack_1, Some pack_2) -> | ||
| fprintf ppf "%a@ was built with -for-pack %s, but this module is built\ |
There was a problem hiding this comment.
Is a space needed before the backslash?
asmcomp/compilenv.ml
Outdated
| @@ -443,6 +451,13 @@ let report_error ppf = function | |||
| fprintf ppf "%a@ contains the description for unit\ | |||
| @ %s when %s was expected" | |||
| Location.print_filename filename name modname | |||
| | Mismatching_for_pack(filename, pack_1, None) -> | |||
| fprintf ppf "%a@ was built with -for-pack %s, but this module isn't" | |||
There was a problem hiding this comment.
Is "this module" the correct terminology? I'm wondering if it should be more explicit that it corresponds to the current compilation unit.
Also, I would write "is not" instead of "isn't" here.
|
It won't fix this situation, but it will produce a proper error. |
|
Right, by "fix" I meant "produce a proper error", so that's good. |
chambart
force-pushed
the
mrp_7649_3922
branch
from
f686770
to
e88e3fb
Compare
|
Rebased against trunk, add Changes. |
|
I've removed the magic number changes, since these are now done centrally, and merged this with trunk. Assuming this passes CI this can be merged. |
|
Hmm, I have a feeling the failures now are because this is catching a problem with one of the dynlink test cases, which I think I fixed in my big dynlink-refactoring patch that hasn't been reviewed yet... |
|
One year later, is this PR dead? or could we bring it to a merge? |
|
I will discuss this with @chambart , as I've done some work in this area recently. |
|
No change since september 2019, so closing. @chambart or @mshinwell, feel free to reopen if you wish to revisit the issue. |
|
@lthls rebased it. |
chambart
force-pushed
the
mrp_7649_3922
branch
from
3a00289
to
a2bd9ec
Compare
…-for-pack If module A is build with -for-pack X and module B with -for-pack Y and A uses B, this will trigger when building A (with ocamlopt) rather than when linking, or when building some further module using both A and B.
chambart
force-pushed
the
mrp_7649_3922
branch
from
a2bd9ec
to
ae50f1d
Compare
|
A quick note on the deleted test: I could have fixed it to refer to the pack through the However, looking at that test file I see that we do produce a |
|
Nobody has complained, so I'm merging this. |
Building different modules with different -for-pack options can lead to particularly inscrutable error messages. (See https://caml.inria.fr/mantis/view.php?id=7645)
This patch forbid it with ocamlopt. This is a breaking change, but I think that it can only uncover hidden build system bugs. This triggers for instance on the lib-dynlink-native test (The source of the travis failure). Which I consider to be such a build system bug (or quite strange test).
Such an early test is not possible in bytecode as cmo files are only used at link time and -for-pack is ignored.