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.

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

Cmti files: fix duplicate self-reference in imported cmi_crcs list. #744

Closed
wants to merge 1 commit into
base: trunk
from

Conversation

Projects
None yet
3 participants
@dbuenzli
Contributor

dbuenzli commented Aug 4, 2016

Currently cmti files store the reference to their own cmi twice in the cmi_crcs list:

> ocamlobjinfo $(opam config var lib)/ocaml/list.cmti
File /Users/dbuenzli/.opam/4.03.0/lib/ocaml/list.cmti
Unit name: List
Interfaces imported:
    ac5f6095cc0a546330ada0df0986a497    List
    999b28e3b7638771c87eebf5a8325e42    Pervasives
    ac5f6095cc0a546330ada0df0986a497    List
    9642e3ed163e46770985ca668738ed5f    CamlinternalFormatBasics
[...]

The reason is that the cmi_crcs are written with an imports value that already contains the crc for the module. However the output_cmi function assumes cmi_crcs doesn't contain the crc for the module itself.

This patch simply remove the crc of the module from imports to define the cmi_crcs field to output:

> _install/bin/ocamlobjinfo stdlib/list.cmti
File stdlib/list.cmti
Unit name: List
Interfaces imported:
    cd293869f2923980325b5f3354992c2d    List
    eef96f967b03d53aceb35ab9ee61e6fc    Pervasives
    cbd5f2d6b649925222e1e9fb63b89db6    CamlinternalFormatBasics
[...]
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 8, 2016

Member

It is not completely trivial to review, but the patch looks correct to me.

Member

gasche commented Aug 8, 2016

It is not completely trivial to review, but the patch looks correct to me.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 25, 2016

Contributor

The code in cmt_format that creates the cmi_infos record looks suspicious: the creation of the flags list has gotten out of sync with the one in Env.save_signature_with_imports. This does not affect the digest which does not take flags into account, but the flags info in the .cmti wrong will be wrong. One should share the code with Env.save_signature_with_imports (which requires passing the deprecated info).

Btw, shouldn't the digest take into account the flags?

Also, it's a bit confusing: Env.save_signature also calls Env.imports (as Cmt_format.save_cmt does); why is it not affected by the same bug? Answer: because it's only when one saves the the cmi file that the current unit is registered in the set returned by Env.imports. This looks a bit fragile. Perhaps a more robust fix would be to add a include_current_unit:bool argument to Env.imports to explicitly choose whether the current unit should be included or not (it would be included in the calls to that function from Emitcode and Compilenv, i.e. when one build an implementation; and explicitly removed when one calls that function to produce a cmi structure).

Contributor

alainfrisch commented Aug 25, 2016

The code in cmt_format that creates the cmi_infos record looks suspicious: the creation of the flags list has gotten out of sync with the one in Env.save_signature_with_imports. This does not affect the digest which does not take flags into account, but the flags info in the .cmti wrong will be wrong. One should share the code with Env.save_signature_with_imports (which requires passing the deprecated info).

Btw, shouldn't the digest take into account the flags?

Also, it's a bit confusing: Env.save_signature also calls Env.imports (as Cmt_format.save_cmt does); why is it not affected by the same bug? Answer: because it's only when one saves the the cmi file that the current unit is registered in the set returned by Env.imports. This looks a bit fragile. Perhaps a more robust fix would be to add a include_current_unit:bool argument to Env.imports to explicitly choose whether the current unit should be included or not (it would be included in the calls to that function from Emitcode and Compilenv, i.e. when one build an implementation; and explicitly removed when one calls that function to produce a cmi structure).

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Aug 25, 2016

Contributor

@alainfrisch If you have better ideas to improve the (frankly a bit too convoluted to my taste) code feel free to close this an provide a better patch.

Contributor

dbuenzli commented Aug 25, 2016

@alainfrisch If you have better ideas to improve the (frankly a bit too convoluted to my taste) code feel free to close this an provide a better patch.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 26, 2016

Contributor

On second thought, it seems simply better to pass the cmi_infos directly instead of the Types.signature to save_cmt.

Contributor

alainfrisch commented Aug 26, 2016

On second thought, it seems simply better to pass the cmi_infos directly instead of the Types.signature to save_cmt.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 26, 2016

Contributor

Will create another PR.

Contributor

alainfrisch commented Aug 26, 2016

Will create another PR.

alainfrisch added a commit to alainfrisch/ocaml that referenced this pull request Aug 26, 2016

Avoid rebuilding cmi_info record when creating .cmti files
Instead of rebuilding cmi_info in Cmt_format.save_cmt, the record
created in Env.save_signature is kept and passed to that function.  In
addition to simplifying the code, this avoids possible mismatch between
the two records, including:

  - Duplicated entry in cmi_crcs for the current unit as noted in #744.

  - Missing flags (Unsafe_string/Deprecated were not properly set in
    Cmt_format).

The interface is also stronger, since the signature passed to save_cmt
was supposed to be already mapped by Subst.for_saving but this was not
reflected in the API.
@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 26, 2016

Contributor

Superseded by #781.

Contributor

alainfrisch commented Aug 26, 2016

Superseded by #781.

alainfrisch added a commit to alainfrisch/ocaml that referenced this pull request Aug 30, 2016

Avoid rebuilding cmi_info record when creating .cmti files
Instead of rebuilding cmi_info in Cmt_format.save_cmt, the record
created in Env.save_signature is kept and passed to that function.  In
addition to simplifying the code, this avoids possible mismatch between
the two records, including:

  - Duplicated entry in cmi_crcs for the current unit as noted in #744.

  - Missing flags (Unsafe_string/Deprecated were not properly set in
    Cmt_format).

The interface is also stronger, since the signature passed to save_cmt
was supposed to be already mapped by Subst.for_saving but this was not
reflected in the API.

alainfrisch added a commit that referenced this pull request Aug 30, 2016

Avoid rebuilding cmi_info record when creating .cmti files (#781)
Instead of rebuilding cmi_info in Cmt_format.save_cmt, the record
created in Env.save_signature is kept and passed to that function.  In
addition to simplifying the code, this avoids possible mismatch between
the two records, including:

  - Duplicated entry in cmi_crcs for the current unit as noted in #744.

  - Missing flags (Unsafe_string/Deprecated were not properly set in
    Cmt_format).

The interface is also stronger, since the signature passed to save_cmt
was supposed to be already mapped by Subst.for_saving but this was not
reflected in the API.

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Avoid rebuilding cmi_info record when creating .cmti files (#781)
Instead of rebuilding cmi_info in Cmt_format.save_cmt, the record
created in Env.save_signature is kept and passed to that function.  In
addition to simplifying the code, this avoids possible mismatch between
the two records, including:

  - Duplicated entry in cmi_crcs for the current unit as noted in #744.

  - Missing flags (Unsafe_string/Deprecated were not properly set in
    Cmt_format).

The interface is also stronger, since the signature passed to save_cmt
was supposed to be already mapped by Subst.for_saving but this was not
reflected in the API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment