| Message ID | 7b88cbda20e068bbce1c5dfb0a18af3f4e4b6865.1664276827.git.edvin.torok@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/ocaml: build/compatibility fixes with OCaml 5.0 for Xen 4.17 | expand |
This accidentally broke compatibility with OCaml 4.02.3, only realized when I went back to my Dune based build system which can automatically compile with multiple compiler versions. See below for a patch for that. I've included this patch in the correct place (before the patch that breaks it) in the git repository at: https://github.com/edwintorok/xen/compare/private/edvint/public0 From 78a613cbb8db7033fe741488912f21b24eaaef56 Mon Sep 17 00:00:00 2001 Message-Id: <78a613cbb8db7033fe741488912f21b24eaaef56.1664295046.git.edvin.torok@citrix.com> From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= <edvin.torok@citrix.com> Date: Tue, 27 Sep 2022 17:06:57 +0100 Subject: [PATCH] tools/ocaml: fix compatibility with OCaml 4.02.3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Edwin Török <edvin.torok@citrix.com> --- tools/ocaml/libs/mmap/mmap_stubs.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tools/ocaml/libs/mmap/mmap_stubs.h b/tools/ocaml/libs/mmap/mmap_stubs.h index 65e4239890..5c65cc86fb 100644 --- a/tools/ocaml/libs/mmap/mmap_stubs.h +++ b/tools/ocaml/libs/mmap/mmap_stubs.h @@ -30,4 +30,9 @@ struct mmap_interface int len; }; +/* for compatibility with OCaml 4.02.3 */ +#ifndef Data_abstract_val +#define Data_abstract_val(v) ((void*) Op_val(v)) +#endif + #endif -- 2.34.1 > On 27 Sep 2022, at 12:15, Edwin Török <edvin.torok@citrix.com> wrote: > > Follow the manual to avoid naked pointers: > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fv2.ocaml.org%2Fmanual%2Fintfc.html%23ss%3Ac-outside-head&data=05%7C01%7Cedvin.torok%40citrix.com%7C4bf28f7a32074a49cdf008daa079a807%7C335836de42ef43a2b145348c2ee9ca5b%7C0%7C0%7C637998741634627105%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=GU%2BbmB1ai0llQg0z5zWctzwW0ocUQUOHVlMxkeD3U0U%3D&reserved=0 > > No functional change, except on OCaml 5.0 where it is a bugfix. > > Signed-off-by: Edwin Török <edvin.torok@citrix.com> > --- > tools/ocaml/libs/xc/xenctrl_stubs.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c > index 19335bdf45..7ff4e00314 100644 > --- a/tools/ocaml/libs/xc/xenctrl_stubs.c > +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c > @@ -37,7 +37,7 @@ > > #include "mmap_stubs.h" > > -#define _H(__h) ((xc_interface *)(__h)) > +#define _H(__h) *((xc_interface **) Data_abstract_val(__h)) > #define _D(__d) ((uint32_t)Int_val(__d)) > > #ifndef Val_none > @@ -70,14 +70,15 @@ static void Noreturn failwith_xc(xc_interface *xch) > CAMLprim value stub_xc_interface_open(void) > { > CAMLparam0(); > - xc_interface *xch; > + CAMLlocal1(result); > > + result = caml_alloc(1, Abstract_tag); > /* Don't assert XC_OPENFLAG_NON_REENTRANT because these bindings > * do not prevent re-entrancy to libxc */ > - xch = xc_interface_open(NULL, NULL, 0); > - if (xch == NULL) > + _H(result) = xc_interface_open(NULL, NULL, 0); > + if (_H(result) == NULL) > failwith_xc(NULL); > - CAMLreturn((value)xch); > + CAMLreturn(result); > } > > > -- > 2.34.1 >
> On 27 Sep 2022, at 17:13, Edwin Torok <edvin.torok@citrix.com> wrote: > > > See below for a patch for that. I've included this patch in the correct place (before the patch that breaks it) in the git repository at: > https://github.com/edwintorok/xen/compare/private/edvint/public0 > Acked-by: Christian Lindig <christian.lindig@citrix.com> I believe these changes are fine. We are now allocating the event channel dynamically and this requires using a finaliser to free that memory. > -ifneq ($(MAKECMDGOALS),clean) > +ifeq (,$(findstring clean,$(MAKECMDGOALS))) > .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile $(OCAML_TOPLEVEL)/Makefile.rules > $(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,) > endif Is this the right logic? Moving from ifneq to ifeq here. I am not so familiar with the Makfile rules. The gist seems to be: we depend on auto-generated Make dependencies that the Makefile in general depends on. But in a “make clean” (or other “*clean” it is wasteful to generate these only to later remove them. However, these kind of subtleties are obvious enough while we are working on this but over time accumulate to Makefiles that are hard to change. So I wonder if this kind of optimisation, while correct, is worth it, but fine going along with it. — C
> On 30 Sep 2022, at 15:59, Christian Lindig <christian.lindig@citrix.com> wrote: > > > >> On 27 Sep 2022, at 17:13, Edwin Torok <edvin.torok@citrix.com> wrote: >> >> >> See below for a patch for that. I've included this patch in the correct place (before the patch that breaks it) in the git repository at: >> https://github.com/edwintorok/xen/compare/private/edvint/public0 >> > > > Acked-by: Christian Lindig <christian.lindig@citrix.com> > > I believe these changes are fine. We are now allocating the event channel dynamically and this requires using a finaliser to free that memory. Thanks, > > >> -ifneq ($(MAKECMDGOALS),clean) >> +ifeq (,$(findstring clean,$(MAKECMDGOALS))) >> .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile $(OCAML_TOPLEVEL)/Makefile.rules >> $(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,) >> endif > > Is this the right logic? Moving from ifneq to ifeq here. > > I am not so familiar with the Makfile rules. The gist seems to be: we depend on auto-generated Make dependencies that the Makefile in general depends on. But in a “make clean” (or other “*clean” it is wasteful to generate these only to later remove them. However, these kind of subtleties are obvious enough while we are working on this but over time accumulate to Makefiles that are hard to change. So I wonder if this kind of optimisation, while correct, is worth it, but fine going along with it. > Makefile functions can be a bit confusing to read. "ifneq ($(MAKECMDGOALS), clean)" means $(MAKECMDGOALS) != "clean" "ifeq (,$(findstring clean,$(MAKECMDGOALS)))" means that "clean" in $(MAKECMDGOALS) == "" (the empty string), or i.o.w. "clean" not in $(MAKECMDGOALS), which is a bit more generic than the previous one, since we have all sorts of rules in the Makefile (especially around subdirs) where 'clean' is a substring. This is quite subtle and I had to reread this line many times too to check it is correct. The real solution here would be to have a single non-recursive Makefile (and there is some discussion/patches heading in that direction in xen-devel particularly from Anthony), and then evaluating the "clean" rules would be a lot less expensive, it'd only have to be done once. But there might be a while until we get there, and meanwhile these clean rules slow down the OCaml build too much (just running the "clean" takes a lot longer than building the entire OCaml libraries and oxenstored sequentially). Although I only need to use 'clean' when using the upstream Makefiles (where almost every incremental change requires a 'clean' inbetween because the Makefiles express the dependencies incorrectly), or when switching from upstream Makefile to 'dune' (a one-off event usually). Since I use 'Dune' for my daily work anyway (and the makefile is used in our internal build system) perhaps this Makefile patch is not needed at all, I can change 'Makefile.dune' to not call 'make clean' at all, and I'll know to remember to run it if things fail anyway (it'll be pretty obvious when Dune says you've got a build artifact in the wrong place). Best regards, --Edwin
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c index 19335bdf45..7ff4e00314 100644 --- a/tools/ocaml/libs/xc/xenctrl_stubs.c +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c @@ -37,7 +37,7 @@ #include "mmap_stubs.h" -#define _H(__h) ((xc_interface *)(__h)) +#define _H(__h) *((xc_interface **) Data_abstract_val(__h)) #define _D(__d) ((uint32_t)Int_val(__d)) #ifndef Val_none @@ -70,14 +70,15 @@ static void Noreturn failwith_xc(xc_interface *xch) CAMLprim value stub_xc_interface_open(void) { CAMLparam0(); - xc_interface *xch; + CAMLlocal1(result); + result = caml_alloc(1, Abstract_tag); /* Don't assert XC_OPENFLAG_NON_REENTRANT because these bindings * do not prevent re-entrancy to libxc */ - xch = xc_interface_open(NULL, NULL, 0); - if (xch == NULL) + _H(result) = xc_interface_open(NULL, NULL, 0); + if (_H(result) == NULL) failwith_xc(NULL); - CAMLreturn((value)xch); + CAMLreturn(result); }
Follow the manual to avoid naked pointers: https://v2.ocaml.org/manual/intfc.html#ss:c-outside-head No functional change, except on OCaml 5.0 where it is a bugfix. Signed-off-by: Edwin Török <edvin.torok@citrix.com> --- tools/ocaml/libs/xc/xenctrl_stubs.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)