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

patch of byterun/unix.c to bring native runtime inline with bytecode in regards to plugin reloading #7893

Closed
vicuna opened this issue Jan 5, 2019 · 6 comments

Comments

@vicuna
Copy link

vicuna commented Jan 5, 2019

Original bug ID: 7893
Reporter: progman
Status: new
Resolution: open
Priority: normal
Severity: minor
Platform: rpi3
OS: raspbian
OS Version: debian 18
Version: 4.07.1
Category: dynlink and natdynlink
Monitored by: @nojb @hcarty

Bug description

*** unix.c.dlltest	2019-01-05 07:26:44.459916955 +0000
--- unix.c	2019-01-05 08:22:05.729915688 +0000
***************
*** 273,278 ****
--- 273,282 ----
  
  void * caml_dlopen(char * libname, int for_execution, int global)
  {
+   //check whether already loaded
+   void* handle= dlopen(libname, RTLD_NOW | RTLD_NOLOAD);
+   if(handle)//it is so unload first as code is not otherwise actually loaded again
+     if(dlclose(handle) || dlclose(handle)) return NULL;
    return dlopen(libname, RTLD_NOW | (global ? RTLD_GLOBAL : RTLD_LOCAL));
    /* Could use RTLD_LAZY if for_execution == 0, but needs testing */
  }
@vicuna
Copy link
Author

vicuna commented Jan 5, 2019

Comment author: progman

bytecode plugins can be edited, re-compiled and re-loaded into a running program.
with native the operation appears successful but does not in fact reload the
code.
the patch checks for presence of said plugin and unloads it to ensure
a code refresh.
two dlclose()'s are required (docs say 1 close each open, and RTLD_NOLOAD still
(in practice) increments the plugin reference count, hence two)

@vicuna vicuna added the dynlink label Mar 14, 2019
@houndofsound
Copy link

Makefile:
go: dmods
ocamlc -c dyn.ml
ocamlc -o main dyn.cmo dynlink.cma main.ml
(sleep 1 ;mv dmod2.cmo dmod.cmo; echo new dmod...)&
./main

dmods: dmod.ml dmod2.ml
ocamlc -c dmod.ml
ocamlc -c dmod2.ml

#doesn't work! dynlink successful but code not changed.
#patched byterun/unix.c:caml_dlopen to unload previous load
opt:
ocamlopt -c dyn.ml
ocamlopt -o dmod.cmxs -shared dmod.ml
ocamlopt -o dmod2.cmxs -shared dmod2.ml
ocamlopt -o main dyn.cmx dynlink.cmxa main.ml
(sleep 1 ;mv dmod2.cmx dmod.cmx; mv dmod2.cmxs dmod.cmxs; mv dmod2.o dmod.o; echo new dmod...)&
./main

dyn.ml:
open Printf
type dynfuns={
mutable f1:unit->unit
}

let deffun()=
printf "default dyn function\n"

let dynfuns={f1=deffun}

main.ml:
(*
load a unit, invoke one of it's function, wait for keypress,....
then go and change the unit ...
press key, and reload unit, invoke same function
*)

open Printf
open Dyn

open Dynlink

let prompt()=
printf "press enter...";
let _=read_line() in
()

let go()=
let file= adapt_filename "dmod.cmo" in
dynfuns.f1();
loadfile file; (*it registers in the above record *)
dynfuns.f1();
prompt();
loadfile file;
dynfuns.f1();
printf "done\n"

let _=go()

dmod.ml:
open Dyn
open Printf
let myfun()=
printf "dynamically loaded fun...\n"

let _=
dynfuns.f1<-myfun

dmod2.ml:
open Dyn
open Printf

type t=INT of int
let i=INT 99

let myfun()=
printf "dynamically loaded fun 2nd!!!!...\n"

let _=
dynfuns.f1<-myfun

@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label Jul 13, 2020
@progman1
Copy link

still reproducible.

@lthls
Copy link
Contributor

lthls commented Jul 27, 2020

Unloading of modules in native dynlink is not supposed to work, and your example doesn't need to be modified much to show why.
If you replace the contents of the go function in main.ml by:

let file= adapt_filename "dmod.cmo" in
let f_before = dynfuns.f1 in
loadfile file; (*it registers in the above record *)
let f_between = dynfuns.f1 in
prompt();
loadfile file;
let f_after = dynfuns.f1 in
f_before ();
f_between ();
f_after ();
printf "done\n"

You should get an error in the call to f_between (with your patch) as its code has been unloaded. I'm not quite sure why it is fine to allow it in bytecode (I hope that at least there's a check that they share the same interface), but in native mode due to symbol conflicts you can't allow two compilation units with the same name to be loaded together (I wonder why the compiler is silently ignoring the second load instead of complaining, though). And since you can't unload code because you may still have closures around that point to it, that means you're stuck with only one dynamic load for each name.
For cases like yours where the modules are only loaded for their side effects, there's no reason why you could not just keep distinct file names. If you're actually depending on overriding the same module name (for instance if you are loading another module that depends on Dmod), then even in bytecode I don't think it was supposed to work.

@github-actions github-actions bot removed the Stale label Jul 29, 2020
@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

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

4 participants