Skip to content
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

Module initialization not performed for extern-only lib (? segfault) #6956

Closed
vicuna opened this issue Aug 11, 2015 · 16 comments
Closed

Module initialization not performed for extern-only lib (? segfault) #6956

vicuna opened this issue Aug 11, 2015 · 16 comments
Assignees

Comments

@vicuna
Copy link

vicuna commented Aug 11, 2015

Original bug ID: 6956
Reporter: @Chris00
Assigned to: @garrigue
Status: resolved (set by @xavierleroy on 2015-11-22T10:44:32Z)
Resolution: fixed
Priority: low
Severity: minor
Platform: amd64
OS: Debian GNU/Linux
OS Version: testing 3.18.16
Version: 4.01.0
Fixed in version: 4.03.0+dev / +beta1
Category: ~DO NOT USE (was: OCaml general)
Duplicate of: #4166
Related to: #7371
Monitored by: @hcarty @Chris00

Bug description

When a module exposes extern functions and only those are used in a program, the module initialization code is not executed. This is a problem because this code may register exceptions that may be raised by the C functions.

Steps to reproduce

A minimal example is attached (use "make" to compile). The "good" example correctly raise the exception, be the exception is explicitly mentioned in the program (which triggers the module initialization code). In the "bad" example, the exception is no longer mentioned and the program segfaults. The reason is because caml_named_value returns NULL and the dereferencing fails.

File attachments

@vicuna
Copy link
Author

vicuna commented Aug 12, 2015

Comment author: @chambart

It is not exactly that the module does not perform initialisation. In fact dependencies to external exported in the cmi file does not add a dependency to the implementation.

The classical way to fix that kind of problem are to either:
Add some sort of dummy code to force linking.
Hide the fact that the functions are external through an .mli.
Explicitely add the cmx file to the link command.

I'm not certain we may want to consider externals as real code dependency, but if we do, that would require adding the information in the Primitive.description type the original compilation unit that should contain this function and before emitting the code, add the dependencies of every present externals.

@vicuna
Copy link
Author

vicuna commented Aug 12, 2015

Comment author: @Chris00

I'm not certain we may want to consider externals as real code dependency

If you decide not to, it should be prominently clear in the manual. I suppose I am not the only one thinking that exporting an "external" in a .mli file is like "val" with the additional possibility of inlining the C call. It is absolutely unexpected that it bypasses the module initialization code.

@vicuna
Copy link
Author

vicuna commented Aug 12, 2015

Comment author: @garrigue

This is a well-known problem (cf. #4166)
The correct behavior is not completely clear: an external declaration doesn't require any ML code, so you can actually include it in an interface-only module.
Note that, as I have mentioned in #4166, since module aliases also have this behavior when using -no-alias-deps, we might want to limit this behavior for externals in the same way. However, the approach used for module aliases is not directly applicable to externals for now (would create too much dummy code).

@vicuna
Copy link
Author

vicuna commented Aug 12, 2015

Comment author: @garrigue

By the way, since this is a long-standing issue, I wouldn't qualify the priority as "high".
This is just a case of undefined behavior (since the meaning of external is under-specified).

@vicuna
Copy link
Author

vicuna commented Aug 12, 2015

Comment author: @damiendoligez

If you use "val" in your .mli files instead of "external", ocamlopt will compile your calls in exactly the same way.

IMO the proper solution to this problem is to add a warning to the documentation to the effect that users should never use "external" in their .mli files.

@vicuna
Copy link
Author

vicuna commented Aug 12, 2015

Comment author: @Chris00

users should never use "external" in their .mli files

Then, why not add a warning to the compiler (turned on by default)?

@vicuna
Copy link
Author

vicuna commented Aug 14, 2015

Comment author: @garrigue

As an experiment, I've written a patch which warns on the user side: emit a warning whenever a primitive is called but the unit it belongs to is not referenced by the code.
Here is an example:
../boot/ocamlrun ../ocamlc -strict-sequence -w +33..39 -g -warn-error A -bin-annot -nostdlib -safe-string ./Compflags scanf.cmo -c scanf.ml
File "scanf.ml", line 334, characters 14-30:
Warning 52: this compilation unit uses the primitive Bytes.create,
but it doesn't require Bytes

As you can see, this happens in the standard library itself, so I'm not sure this is a good practical solution.
Disable it by default ? But then it may be completely ignored by users.
Rather warn about compilation units exporting externals ? This is going to happen if you don't write an mli.
Use my warning, but add an attribute to the exported definition, like for deprecated functions (but the other way round) ? Somehow it seems superfluous, as using external means that you want you export to be used without requiring the unit, but somehow it makes sense for the problems in this PR. The situation would be, if you use external without attribute you get warned if the unit is not required, but the provider can explicitly state that there is no need to link the unit by setting the attribute.

@vicuna
Copy link
Author

vicuna commented Aug 14, 2015

Comment author: @garrigue

After looking at the results of my warning on various libraries (in particular lablgtk), I'm having a few other ideas.

Conservative approach (avoiding breakage):
warn about external in interfaces (but of course not in implementations), and also use my user-side warning when the imported module was compiled was compiled with no interface.

Clever approach:
Only warn on the producer side. Detect whether a module has side-effects or not (i.e. toplevel function calls), and only warn if it both exports externals and has side-effects.
Note that one can avoid exporting an external in an implementation by immediately rebinding it:
external f : t1 -> t2 = "f"
let f = f

This said, I can see plenty of examples which break all of these, without any clear bug.

@vicuna
Copy link
Author

vicuna commented Aug 14, 2015

Comment author: @garrigue

Of course, this also seems to mean that actually the warning approach is not the right one, and we should rather introduce internal dependencies, and force the linking.

@vicuna
Copy link
Author

vicuna commented Aug 14, 2015

Comment author: @garrigue

Added a new patch, require-external.diff, which forces the linking of modules containing used external definitions.
I suppose this is the simplest solution (actually this is only a slight variant of the warning patch...)
A practical question is whether we need a flag to disable that, link -no-alias-deps for module aliases. The only reason I would see to do that is to allow "purely external modules", i.e. without implementation, but does it exist in practice?

@vicuna
Copy link
Author

vicuna commented Aug 18, 2015

Comment author: @xavierleroy

I'm coming late to this discussion, but the one true way to ensure that initialization code from an OCaml module is always executed is to use the "-linkall" flag when building the library that contains this module. Without -linkall, the compiler is free to discard the module if it sees no use of the code defined in this module. It's a fairly simple policy (-linkall = important side effect in module initialization; no -linkall = module can be discarded), and I'm not sure we need more complicated mechanisms or heuristics.

@vicuna
Copy link
Author

vicuna commented Aug 19, 2015

Comment author: @garrigue

Xavier, while your statement is certainly true at this point (in particular due to the external problem), I'm not sure we want things to stay that way, and I am under the impression that efforts have been made in order to ensure that the "requirement" semantics is clear in most cases.

One reason -linkall is not really an option is huge libraries like Core: do you really suggest to use -linkall for such libraries, whereas we have just introduced module aliases to avoid monolithic linking? I understand that -linkall only needs to be enable for side-effecting modules, but identifying them can be painful, and often one only wants the side-effect to occur if some functions in the module are used.

When we introduced module aliases we have been careful not to change the "requirement" semantics in the default case: modules that were required at link time should stay required.
Correct me if I'm wrong, but the simplifications at the lambda-code level are also careful not to remove user's code. And deadcode elimination and inlining in the native backend is done after computing the requirements. For bytecode, apparently there is some deadcode elimination occurring before computing the required symbols, which means in particular that code immediately after a raise statement does not generate dependencies, but I think this is the only case.
So, it seems that the requirements of a module are fairly predictable, and people do actually rely on it.
What I'm proposing is a very small patch that makes it also predictable for externals, using the same mechanism as for module aliases.

If you think that this should be left unspecified, and that the compiler should be free to aggressively remove dependencies, then this should be clearly stated, because people are depending on that.

@vicuna
Copy link
Author

vicuna commented Sep 9, 2015

Comment author: @garrigue

Looking back again at this problem (and 4166), I really think that we should do something about it, as it bites even experienced users.
And, in my opinion, fixing the semantics is the simplest approach (i.e. require-external.diff).

The only reasonable alternative I see would be to deprecate the use of external in interfaces.
However, even doing that would not be sufficient in practice, as the same problem happens if you don't write a mli.

@vicuna
Copy link
Author

vicuna commented Sep 9, 2015

Comment author: @alainfrisch

Specifying the "requirement" semantics seems a good idea to me.

Allowing externals in interfaces improves the bytecode, which could be especially important with js_of_ocaml (several low-level operations are exposed as externals, I'm not sure how clever is js_of_ocaml to detect them if they are not exposed as externals in the interface).

Would it be shocking to specify the "requirement" semantics based on identifier resolution, including non-value-like components? (i.e. referring to a type Foo.t would force linking in Foo) This would be quite predictable at least.

@vicuna
Copy link
Author

vicuna commented Sep 9, 2015

Comment author: @garrigue

It's a bit more complex for types, as people do rely on the fact using only types requires no implementation (cf. parsetree.mli).
And some even use it for mutually recursive type definitions!
(Of course that part is not officially supported)

@vicuna
Copy link
Author

vicuna commented Nov 22, 2015

Comment author: @xavierleroy

See #289. Fixed by commit 07b8eb3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants