Skip to content

Fix lack of link error when missing modules providing externals#837

Merged
garrigue merged 4 commits intoocaml:4.04from
chambart:fix_bytecode_required_modules
Oct 11, 2016
Merged

Fix lack of link error when missing modules providing externals#837
garrigue merged 4 commits intoocaml:4.04from
chambart:fix_bytecode_required_modules

Conversation

@chambart
Copy link
Copy Markdown
Contributor

@chambart chambart commented Oct 4, 2016

Since 4.03 when a module provide an external and the external is used, the module must be linked.
This was implemented by generating dummy code, in the pr #602 I changed that. It now records in the cmo file that some module is required. This correctly made module provided by cma files to be linked, but nothing checked that this was not empty at the end.

This patch add the check.

I don't have time right now to test it correclty, so I will let the CI check that it does not break any other compilation. Sorry for that, and the bug.

@chambart chambart added this to the 4.04 milestone Oct 4, 2016
@chambart chambart added the bug label Oct 4, 2016
@damiendoligez
Copy link
Copy Markdown
Member

@garrigue could you review?

@chambart
Copy link
Copy Markdown
Contributor Author

chambart commented Oct 4, 2016

The problem seems to come from pack.
dynlinkaux.cmo is linked into a binary, and requires (among others Arg_helpers, which is provided by the pack). It is a pack made out of module not compiled with -for-pack (I think) (which is possible in bytecode), I don't know if this has to do with that.

@chambart
Copy link
Copy Markdown
Contributor Author

chambart commented Oct 4, 2016

No, I just checked, there is nothing removing required modules provided by the pack.

@chambart
Copy link
Copy Markdown
Contributor Author

chambart commented Oct 4, 2016

Here it is, as previous, not really tested yet...

@garrigue
Copy link
Copy Markdown
Contributor

garrigue commented Oct 5, 2016

OK, I'm waiting for a complete fix before reviewing.
Note that it would not be too bad if this doesn't go into 4.04, as the behavior is correct when using a library, which was the main goal. It is also a bit surprising that the behavior with ocamlc and ocamlopt are different.

@damiendoligez
Copy link
Copy Markdown
Member

I'm keeping the 4.04 milestone for now, we'll change it if this becomes the last PR to hold up the release.

@garrigue
Copy link
Copy Markdown
Contributor

garrigue commented Oct 6, 2016

It seems almost ready, except for this strange failure on w55.opt-backend using ocamlopt with flambda.
The strange part is that it is listed as passed in the log, but then reported in the failed test section.

@garrigue
Copy link
Copy Markdown
Contributor

garrigue commented Oct 6, 2016

By the way, your branch doesn't seem to include my addition of testsuite/tests/manual-intf-c. It would help checking.

@gasche
Copy link
Copy Markdown
Member

gasche commented Oct 6, 2016

@garrigue this test was disabled in the 4.04 branch by Damien in 38055ef, because it breaks on the testing machine as it seems to depend on the presence of a curses library. See the commit page ( 38055ef ), with a (small) discussion on how this could be made to work.

@mshinwell
Copy link
Copy Markdown
Contributor

@chambart It looks like some of the "bound_name" changes are mixed up in this patch by accident, maybe?

@damiendoligez
Copy link
Copy Markdown
Member

It's not 'w55.opt-backend' that fails, but 'w55.opt-backend' with ocamlopt a few lines down. The reporting is slightly misleading but correct.

@chambart
Copy link
Copy Markdown
Contributor Author

chambart commented Oct 6, 2016

@mshinwell effectively, I appently started that on the wrong branch. I'll rebase on 4.04 removing that.
I didn't look at it yet, but this error seems highly unrelated.

@chambart chambart force-pushed the fix_bytecode_required_modules branch from 8ac38af to d0f0586 Compare October 6, 2016 14:43
@garrigue
Copy link
Copy Markdown
Contributor

Looking at your code, it seems that it would be easy to turn the error into a link-time warning.
I wonder whether this would not be better, for backward compatibility (even though 4.03 had a hard error in that case).
However, to be coherent, this would also have to be a warning in native code.
Do you think this could be done easily?

@garrigue garrigue merged commit 6c4023d into ocaml:4.04 Oct 11, 2016
garrigue added a commit that referenced this pull request Oct 11, 2016
@chambart
Copy link
Copy Markdown
Contributor Author

@garrigue In byte code it is effectively quite simple to do a warning instead of an error. In native code, that might be a bit trickier as this is done by adding a fake dependency (loading the cmx file). We would have to distinguish it from normal dependencies. Not that much effort, but I don't know if the benefit is really worth it. Do you know any example in the wild where this might matter ?

dra27 pushed a commit to dra27/ocaml that referenced this pull request Dec 10, 2016
…l#837, PR#7371)

* Check that all the required modules are provided
   Also remove provided cmo from the required globals
* Remove required globals from packs when provided
* Remove provided globals after adding required ones
   Pack modules can require globals they provide.
* Dumpobj tool can print relocation information
dra27 pushed a commit to dra27/ocaml that referenced this pull request Dec 10, 2016
dra27 pushed a commit to dra27/ocaml that referenced this pull request Dec 10, 2016
…l#837, PR#7371)

* Check that all the required modules are provided
   Also remove provided cmo from the required globals
* Remove required globals from packs when provided
* Remove provided globals after adding required ones
   Pack modules can require globals they provide.
* Dumpobj tool can print relocation information
dra27 pushed a commit to dra27/ocaml that referenced this pull request Dec 10, 2016
dra27 pushed a commit to dra27/ocaml that referenced this pull request Dec 11, 2016
…l#837, PR#7371)

* Check that all the required modules are provided
   Also remove provided cmo from the required globals
* Remove required globals from packs when provided
* Remove provided globals after adding required ones
   Pack modules can require globals they provide.
* Dumpobj tool can print relocation information
dra27 pushed a commit to dra27/ocaml that referenced this pull request Dec 11, 2016
dra27 pushed a commit to dra27/ocaml that referenced this pull request Dec 11, 2016
…l#837, PR#7371)

* Check that all the required modules are provided
   Also remove provided cmo from the required globals
* Remove required globals from packs when provided
* Remove provided globals after adding required ones
   Pack modules can require globals they provide.
* Dumpobj tool can print relocation information
dra27 pushed a commit to dra27/ocaml that referenced this pull request Dec 11, 2016
dra27 pushed a commit to dra27/ocaml that referenced this pull request Dec 12, 2016
…l#837, PR#7371)

* Check that all the required modules are provided
   Also remove provided cmo from the required globals
* Remove required globals from packs when provided
* Remove provided globals after adding required ones
   Pack modules can require globals they provide.
* Dumpobj tool can print relocation information
dra27 pushed a commit to dra27/ocaml that referenced this pull request Dec 12, 2016
dra27 pushed a commit to dra27/ocaml that referenced this pull request Dec 14, 2016
…l#837, PR#7371)

* Check that all the required modules are provided
   Also remove provided cmo from the required globals
* Remove required globals from packs when provided
* Remove provided globals after adding required ones
   Pack modules can require globals they provide.
* Dumpobj tool can print relocation information
dra27 pushed a commit to dra27/ocaml that referenced this pull request Dec 14, 2016
dra27 pushed a commit that referenced this pull request Dec 14, 2016
… PR#7371)

* Check that all the required modules are provided
   Also remove provided cmo from the required globals
* Remove required globals from packs when provided
* Remove provided globals after adding required ones
   Pack modules can require globals they provide.
* Dumpobj tool can print relocation information
dra27 pushed a commit that referenced this pull request Dec 14, 2016
camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017
…l#837, PR#7371)

* Check that all the required modules are provided
   Also remove provided cmo from the required globals
* Remove required globals from packs when provided
* Remove provided globals after adding required ones
   Pack modules can require globals they provide.
* Dumpobj tool can print relocation information
camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017
stedolan pushed a commit to stedolan/ocaml that referenced this pull request Sep 21, 2022
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
Co-authored-by: Cuihtlauac ALVARADO <cuihtmlauac@tarides.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants