[PATCH v2] tools/ocaml: Fix library generation

[PATCH v2] tools/ocaml: Fix library generation

[Vincent Bernardoff]

Fix the commands given to the OCaml compiler to make the OCaml
bindings to Xen usable outside the build environment.

Signed-off-by: Vincent Bernardoff <vincent.bernardoff@citrix.com>

---
Changed since v1:
	* tools/Rules.mk is not modified, changes are now in
          bottom-level Makefiles of OCaml libraries
---
 tools/ocaml/Makefile.rules         |    6 +++---
 tools/ocaml/libs/eventchn/Makefile |    2 +-
 tools/ocaml/libs/xc/Makefile       |    2 +-
 tools/ocaml/libs/xl/Makefile       |    2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/ocaml/Makefile.rules b/tools/ocaml/Makefile.rules
index ff19067..28b81a9 100644
--- a/tools/ocaml/Makefile.rules
+++ b/tools/ocaml/Makefile.rules
@@ -45,7 +45,7 @@ ALL_OCAML_OBJ_SOURCES=$(addsuffix .ml, $(ALL_OCAML_OBJS))
 	$(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,)
 
 clean: $(CLEAN_HOOKS)
-	$(Q)rm -f .*.d *.o *.so *.a *.cmo *.cmi *.cma *.cmx *.cmxa *.annot *.spot *.spit $(LIBS) $(PROGRAMS) $(GENERATED_FILES) .ocamldep.make
+	$(Q)rm -f .*.d *.o *.so *.a *.cmo *.cmi *.cma *.cmx *.cmxa *.annot *.spot *.spit $(LIBS) $(PROGRAMS) $(GENERATED_FILES) .ocamldep.make META
 
 quiet-command = $(if $(V),$1,@printf " %-8s %s\n" "$2" "$3" && $1)
 
@@ -54,7 +54,7 @@ mk-caml-lib-bytecode = $(call quiet-command, $(OCAMLC) $(OCAMLCFLAGS) -a -o $1 $
 
 mk-caml-stubs = $(call quiet-command, $(OCAMLMKLIB) -o `basename $1 .a` $2,MKLIB,$1)
 mk-caml-lib-stubs = \
-	$(call quiet-command, $(AR) rcs $1 $2 && $(OCAMLMKLIB) -o `basename $1 .a | sed -e 's/^lib//'` $2,MKLIB,$1)
+	$(call quiet-command, $(OCAMLMKLIB) -o `basename $1 .a | sed -e 's/^lib//'` $2 $3,MKLIB,$1)
 
 # define a library target <name>.cmxa and <name>.cma
 define OCAML_LIBRARY_template
@@ -65,7 +65,7 @@ define OCAML_LIBRARY_template
  $(1)_stubs.a: $(foreach obj,$$($(1)_C_OBJS),$(obj).o)
 	$(call mk-caml-stubs,$$@, $$+)
  lib$(1)_stubs.a: $(foreach obj,$($(1)_C_OBJS),$(obj).o)
-	$(call mk-caml-lib-stubs,$$@, $$+)
+	$(call mk-caml-lib-stubs,$$@, $$+, $(foreach lib,$(LIBS_$(1)),$(lib)))
 endef
 
 define OCAML_NOC_LIBRARY_template
diff --git a/tools/ocaml/libs/eventchn/Makefile b/tools/ocaml/libs/eventchn/Makefile
index 2d8d618..ddd2ace 100644
--- a/tools/ocaml/libs/eventchn/Makefile
+++ b/tools/ocaml/libs/eventchn/Makefile
@@ -8,7 +8,7 @@ OBJS = xeneventchn
 INTF = $(foreach obj, $(OBJS),$(obj).cmi)
 LIBS = xeneventchn.cma xeneventchn.cmxa
 
-LIBS_xeneventchn = $(LDLIBS_libxenctrl)
+LIBS_xeneventchn = -L$(XEN_LIBXC) -lxenctrl
 
 all: $(INTF) $(LIBS) $(PROGRAMS)
 
diff --git a/tools/ocaml/libs/xc/Makefile b/tools/ocaml/libs/xc/Makefile
index 239c187..2e55849 100644
--- a/tools/ocaml/libs/xc/Makefile
+++ b/tools/ocaml/libs/xc/Makefile
@@ -9,7 +9,7 @@ OBJS = xenctrl
 INTF = xenctrl.cmi
 LIBS = xenctrl.cma xenctrl.cmxa
 
-LIBS_xenctrl = $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest)
+LIBS_xenctrl = -L$(XEN_LIBXC) -lxenctrl -lxenguest
 
 xenctrl_OBJS = $(OBJS)
 xenctrl_C_OBJS = xenctrl_stubs
diff --git a/tools/ocaml/libs/xl/Makefile b/tools/ocaml/libs/xl/Makefile
index c9e5274..897921c 100644
--- a/tools/ocaml/libs/xl/Makefile
+++ b/tools/ocaml/libs/xl/Makefile
@@ -10,7 +10,7 @@ OBJS = xenlight
 INTF = xenlight.cmi
 LIBS = xenlight.cma xenlight.cmxa
 
-LIBS_xenlight = $(LDLIBS_libxenlight)
+LIBS_xenlight = -L$(XEN_XENLIGHT) -lxenlight
 
 xenlight_OBJS = $(OBJS)
 xenlight_C_OBJS = xenlight_stubs
-- 
1.7.10.4

Re: [PATCH v2] tools/ocaml: Fix library generation

[Ian Campbell]

On Mon, 2013-04-15 at 11:09 +0100, Vincent Bernardoff wrote:
> Fix the commands given to the OCaml compiler to make the OCaml
> bindings to Xen usable outside the build environment.
> 
> Signed-off-by: Vincent Bernardoff <vincent.bernardoff@citrix.com>
> 
> ---
> Changed since v1:
> 	* tools/Rules.mk is not modified, changes are now in
>           bottom-level Makefiles of OCaml libraries

How does this relate to the patch which Andy Cooper posted in
<fe2d14f39de68ab01ac2.1365012176@andrewcoop.uk.xensource.com> ?

> diff --git a/tools/ocaml/libs/eventchn/Makefile b/tools/ocaml/libs/eventchn/Makefile
> index 2d8d618..ddd2ace 100644
> --- a/tools/ocaml/libs/eventchn/Makefile
> +++ b/tools/ocaml/libs/eventchn/Makefile
> @@ -8,7 +8,7 @@ OBJS = xeneventchn
>  INTF = $(foreach obj, $(OBJS),$(obj).cmi)
>  LIBS = xeneventchn.cma xeneventchn.cmxa
>  
> -LIBS_xeneventchn = $(LDLIBS_libxenctrl)
> +LIBS_xeneventchn = -L$(XEN_LIBXC) -lxenctrl

The problem with this is that it seems to reintroduce a form of the
problem solved by b7ee8d2f432f, that is accidental linking against
libraries in /usr/lib (or elsewhere) instead of the freshly built ones
in the source tree.

Andy's version of the patch seems to have solved that issue, I was just
hoping for a brief explanation of how (per
<1365607338.27868.87.camel@zakaz.uk.xensource.com>) before I through it
in the tree.

Ian.

Re: [PATCH v2] tools/ocaml: Fix library generation

[Andrew Cooper]

On 15/04/13 11:21, Ian Campbell wrote:
> On Mon, 2013-04-15 at 11:09 +0100, Vincent Bernardoff wrote:
>> Fix the commands given to the OCaml compiler to make the OCaml
>> bindings to Xen usable outside the build environment.
>>
>> Signed-off-by: Vincent Bernardoff <vincent.bernardoff@citrix.com>
>>
>> ---
>> Changed since v1:
>> 	* tools/Rules.mk is not modified, changes are now in
>>           bottom-level Makefiles of OCaml libraries
> How does this relate to the patch which Andy Cooper posted in
> <fe2d14f39de68ab01ac2.1365012176@andrewcoop.uk.xensource.com> ?

As stated somewhere on one of the two threads, this patch from Vincent
supersedes mine.

>
>> diff --git a/tools/ocaml/libs/eventchn/Makefile b/tools/ocaml/libs/eventchn/Makefile
>> index 2d8d618..ddd2ace 100644
>> --- a/tools/ocaml/libs/eventchn/Makefile
>> +++ b/tools/ocaml/libs/eventchn/Makefile
>> @@ -8,7 +8,7 @@ OBJS = xeneventchn
>>  INTF = $(foreach obj, $(OBJS),$(obj).cmi)
>>  LIBS = xeneventchn.cma xeneventchn.cmxa
>>  
>> -LIBS_xeneventchn = $(LDLIBS_libxenctrl)
>> +LIBS_xeneventchn = -L$(XEN_LIBXC) -lxenctrl
> The problem with this is that it seems to reintroduce a form of the
> problem solved by b7ee8d2f432f, that is accidental linking against
> libraries in /usr/lib (or elsewhere) instead of the freshly built ones
> in the source tree.
>
> Andy's version of the patch seems to have solved that issue, I was just
> hoping for a brief explanation of how (per
> <1365607338.27868.87.camel@zakaz.uk.xensource.com>) before I through it
> in the tree.
>
> Ian.
>

"My patch" was simply an upstreaming of JonL's patch to make XCP build
against Debian.  I have no particular knowledge of the Ocaml build
gubbins.  It unfortunatly did not fix the problem in general.

~Andrew

Re: [PATCH v2] tools/ocaml: Fix library generation

[Ian Campbell]

On Mon, 2013-04-15 at 11:27 +0100, Andrew Cooper wrote:
> On 15/04/13 11:21, Ian Campbell wrote:
> > On Mon, 2013-04-15 at 11:09 +0100, Vincent Bernardoff wrote:
> >> Fix the commands given to the OCaml compiler to make the OCaml
> >> bindings to Xen usable outside the build environment.
> >>
> >> Signed-off-by: Vincent Bernardoff <vincent.bernardoff@citrix.com>
> >>
> >> ---
> >> Changed since v1:
> >> 	* tools/Rules.mk is not modified, changes are now in
> >>           bottom-level Makefiles of OCaml libraries
> > How does this relate to the patch which Andy Cooper posted in
> > <fe2d14f39de68ab01ac2.1365012176@andrewcoop.uk.xensource.com> ?
> 
> As stated somewhere on one of the two threads, this patch from Vincent
> supersedes mine.

Ah, I'd missed one of the threads (despite replying to it at one point!)

> >
> >> diff --git a/tools/ocaml/libs/eventchn/Makefile b/tools/ocaml/libs/eventchn/Makefile
> >> index 2d8d618..ddd2ace 100644
> >> --- a/tools/ocaml/libs/eventchn/Makefile
> >> +++ b/tools/ocaml/libs/eventchn/Makefile
> >> @@ -8,7 +8,7 @@ OBJS = xeneventchn
> >>  INTF = $(foreach obj, $(OBJS),$(obj).cmi)
> >>  LIBS = xeneventchn.cma xeneventchn.cmxa
> >>  
> >> -LIBS_xeneventchn = $(LDLIBS_libxenctrl)
> >> +LIBS_xeneventchn = -L$(XEN_LIBXC) -lxenctrl
> > The problem with this is that it seems to reintroduce a form of the
> > problem solved by b7ee8d2f432f, that is accidental linking against
> > libraries in /usr/lib (or elsewhere) instead of the freshly built ones
> > in the source tree.
> >
> > Andy's version of the patch seems to have solved that issue, I was just
> > hoping for a brief explanation of how (per
> > <1365607338.27868.87.camel@zakaz.uk.xensource.com>) before I through it
> > in the tree.
> >
> > Ian.
> >
> 
> "My patch" was simply an upstreaming of JonL's patch to make XCP build
> against Debian.  I have no particular knowledge of the Ocaml build
> gubbins.  It unfortunatly did not fix the problem in general.

Adding some CCs, please can we try and retain these for any subsequent
threads/postings of this patch.

So what is the correct fix? How bad are the shortcomings of your
proposed fix?

What we need to avoid is either linking a new set of bindings against
stale libraries present on the system outside of the build directory or
the bindings subsequently linking against libraries other than the ones
they were built/linked against. Doing either is likely to result in
crashes for the user.

This suggests that whatever is baked into the ocaml module needs to
include at least a sufficient portion of the SONAME to ensure that the
library which gets used is actually compatible with the one the bindings
were build against. This requires that we don't embed "-lfoo" into the
module since that translates to libfoo.so not libfoo.so.VERSION (or
whichever scheme is used).

If it isn't possible to separate out the command for linking the module
from the one for using it then some hacks might be needed.

Ian.

Re: [PATCH v2] tools/ocaml: Fix library generation

[Andrew Cooper]

On 15/04/13 12:04, Ian Campbell wrote:
> On Mon, 2013-04-15 at 11:27 +0100, Andrew Cooper wrote:
>> On 15/04/13 11:21, Ian Campbell wrote:
>>> On Mon, 2013-04-15 at 11:09 +0100, Vincent Bernardoff wrote:
>>>> Fix the commands given to the OCaml compiler to make the OCaml
>>>> bindings to Xen usable outside the build environment.
>>>>
>>>> Signed-off-by: Vincent Bernardoff <vincent.bernardoff@citrix.com>
>>>>
>>>> ---
>>>> Changed since v1:
>>>> 	* tools/Rules.mk is not modified, changes are now in
>>>>           bottom-level Makefiles of OCaml libraries
>>> How does this relate to the patch which Andy Cooper posted in
>>> <fe2d14f39de68ab01ac2.1365012176@andrewcoop.uk.xensource.com> ?
>> As stated somewhere on one of the two threads, this patch from Vincent
>> supersedes mine.
> Ah, I'd missed one of the threads (despite replying to it at one point!)
>
>>>> diff --git a/tools/ocaml/libs/eventchn/Makefile b/tools/ocaml/libs/eventchn/Makefile
>>>> index 2d8d618..ddd2ace 100644
>>>> --- a/tools/ocaml/libs/eventchn/Makefile
>>>> +++ b/tools/ocaml/libs/eventchn/Makefile
>>>> @@ -8,7 +8,7 @@ OBJS = xeneventchn
>>>>  INTF = $(foreach obj, $(OBJS),$(obj).cmi)
>>>>  LIBS = xeneventchn.cma xeneventchn.cmxa
>>>>  
>>>> -LIBS_xeneventchn = $(LDLIBS_libxenctrl)
>>>> +LIBS_xeneventchn = -L$(XEN_LIBXC) -lxenctrl
>>> The problem with this is that it seems to reintroduce a form of the
>>> problem solved by b7ee8d2f432f, that is accidental linking against
>>> libraries in /usr/lib (or elsewhere) instead of the freshly built ones
>>> in the source tree.
>>>
>>> Andy's version of the patch seems to have solved that issue, I was just
>>> hoping for a brief explanation of how (per
>>> <1365607338.27868.87.camel@zakaz.uk.xensource.com>) before I through it
>>> in the tree.
>>>
>>> Ian.
>>>
>> "My patch" was simply an upstreaming of JonL's patch to make XCP build
>> against Debian.  I have no particular knowledge of the Ocaml build
>> gubbins.  It unfortunatly did not fix the problem in general.
> Adding some CCs, please can we try and retain these for any subsequent
> threads/postings of this patch.
>
> So what is the correct fix? How bad are the shortcomings of your
> proposed fix?

The shortcomings of the patch sent by me is that it completely breaks
the bytecode libraries.  As XCP was only needing to get the native
compile working, that's all that got accounted for.

>
> What we need to avoid is either linking a new set of bindings against
> stale libraries present on the system outside of the build directory or
> the bindings subsequently linking against libraries other than the ones
> they were built/linked against. Doing either is likely to result in
> crashes for the user.
>
> This suggests that whatever is baked into the ocaml module needs to
> include at least a sufficient portion of the SONAME to ensure that the
> library which gets used is actually compatible with the one the bindings
> were build against. This requires that we don't embed "-lfoo" into the
> module since that translates to libfoo.so not libfoo.so.VERSION (or
> whichever scheme is used).
>
> If it isn't possible to separate out the command for linking the module
> from the one for using it then some hacks might be needed.
>
> Ian.
>

To the best of my understanding:

Ocaml .cma/.cmxa libraries can ether be loaded at runtime, or
subsequently linked against later to create complete programs.

To facilitate that, gcc command line options are embedded in the
cma/cmxa so the correct .so's can be found.

The problem we have is that oxenstored is complied and linked completely
to an executable in the build system, which means it needs the build
system .so locations, while at the same time, the cma/cmxa's need to
have the runtime system library locations so they can be included/linked
against later.

~Andrew

Re: [PATCH v2] tools/ocaml: Fix library generation

[Ian Campbell]

On Mon, 2013-04-15 at 13:19 +0100, Andrew Cooper wrote:

> To the best of my understanding:
> 
> Ocaml .cma/.cmxa libraries can ether be loaded at runtime, or
> subsequently linked against later to create complete programs.
> 
> To facilitate that, gcc command line options are embedded in the
> cma/cmxa so the correct .so's can be found.

This matches my understanding.

One thing which isn't clear to me is if the command line options which
are embedded are also used as part of building the module itself or if
they are only used when the module is linked against.

> The problem we have is that oxenstored is complied and linked completely
> to an executable in the build system, which means it needs the build
> system .so locations, while at the same time, the cma/cmxa's need to
> have the runtime system library locations so they can be included/linked
> against later.

Right, and in this latter case when linking against the ocaml module we
needs to be sure we are picking up a version of the library which is
consistent with what the module was built against (including headers
etc). i.e. we need the right ABI.

This means that -lfoo and /path/to/libfoo.so are both equally wrong, it
needs to be the thing which libfoo.so links to, or the intermediate
"MAJOR" version SONAME variant iff that declares sufficient ABI
compatibility. e.g.

libxenctrl.so -> libxenctrl.so.4.2
libxenctrl.so.4.2 -> libxenctrl.so.4.2.0
libxenctrl.so.4.2.0

So specifying libxenctrl.so is wrong (as is -lxenctrl, which is
~equivalent). Either or libxenctrl.so.4.2 or libxenctrl.so.4.2.0 would
be OK. A C application ends up with a NEEDED value of libxenctrl.so.4.2
so that's probably the one to use here too.

How do the dllFOO_stubs.so libraries:
        $ find tools/ocaml/ -name dll\*stub\*so
        tools/ocaml/libs/xb/dllxenbus_stubs.so
        tools/ocaml/libs/eventchn/dllxeneventchn_stubs.so
        tools/ocaml/libs/xl/dllxenlight_stubs.so
        tools/ocaml/libs/xc/dllxenctrl_stubs.so
        tools/ocaml/libs/mmap/dllxenmmap_stubs.so
        tools/ocaml/xenstored/dllsyslog_stubs.so
fit in? They do not appear to have a suitable ELF NEEDED entry for the
library which they are stubbing for -- could that be the actual root
cause of this issue? I'm not entirely sure how those are built or used
but given that dllxenctrl_stubs.so uses symbols from libxenctrl.so I'd
have said it ought to have a NEEDED libxenctrl.so.4.2 in it.

However I'm not sure how these stub libraries get used, oxenstored
appears to dynamically link against libxenctrls.so.4.2 and not the
dllxenctrl_stubs.so -- so I guess it gets embedded at link time? So I'm
not at all sure if a NEEDED header in dllxentrl_stubs.so would get
propagated to the final application binary (arguably it should).

Ian.