Skip to content

#13669: use compilation unit name in dependency check for the native linker#13673

Merged
Octachron merged 3 commits intoocaml:trunkfrom
Octachron:linkdeps-ignore-pack
Dec 17, 2024
Merged

#13669: use compilation unit name in dependency check for the native linker#13673
Octachron merged 3 commits intoocaml:trunkfrom
Octachron:linkdeps-ignore-pack

Conversation

@Octachron
Copy link
Copy Markdown
Member

When checking dependency at link time, the native linker is currently using the full path for computing provided compilation unit in presence of packed modules. This creates a bug when compiling fragment of packed module separatedly

ocamlopt -for-pack Pack a.ml -c -o a.cmx
ocamlopt -for-pack Pack main.ml -c -o main.cmx
ocamlopt a.cmx main.cmx -o main

because the a.cmx file recorded that it was providing Pack.A whereas Main was registering a dependency on A directly. Consequently, the linking check for dependency emits a mysterious error

Error: No implementation provided for the following modules:
         A referenced from Main (main.cmx)

in the scenario above. This scenario happens in coccinelle build system and me and @lthls agreed that it was better to not break coccinelle even if don't really support this use case.

In order to fix this issue, this PR homogenize the notion of provided and required units for the native linker by stating to the dependency checker that a.cmx provides the compilation unit A rather than the pack submodule Pack.A . Indeed, pack submodule cannot appear as dependency in other modules: pack siblings depends will depend directly on the non-qualified path, whereas external modules will depend only on the pack module itself.

Another option would be to update the dependency side, but the changes felt too intrusive to me for 5.3 .

@Octachron Octachron force-pushed the linkdeps-ignore-pack branch from 366224a to 30b33fe Compare December 13, 2024 17:18
Copy link
Copy Markdown
Contributor

@lthls lthls left a comment

Choose a reason for hiding this comment

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

I think a comment in the test that we only support that for backwards compatibility reasons would be nice, apart from that everything looks fine.

@dra27 dra27 closed this Dec 16, 2024
@dra27 dra27 reopened this Dec 16, 2024
@Octachron Octachron merged commit a9f9c84 into ocaml:trunk Dec 17, 2024
Octachron added a commit that referenced this pull request Dec 18, 2024
#13669: use compilation unit name in dependency check for the native linker

(cherry picked from commit a9f9c84)
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.

3 participants