[PATCH v2 0/5] tools/ocaml: build/compatibility fixes with OCaml 5.0 for Xen 4.17

[PATCH v2 0/5] tools/ocaml: build/compatibility fixes with OCaml 5.0 for Xen 4.17

[Edwin Török]

Changes to previous series:
* removed Dune patches from this series for now (that requires more work to work with osstest on Debian oldstable that won't be ready in time for 4.17)
* also updated xenctrl to work with no naked pointers mode (the only mode in OCaml 5.0)
* changed alloc_custom to use '0' and '1' instead of '1' and '128' for values that are singletons anyway

This can be tested with OCaml <5.0 (e.g. 4.13 or 4.14) with --enable-naked-pointer-checker
to find instances where naked pointers are used or by code review.
(Note that OCaml 5.0 won't have support for naked pointers at all, and thus
it doesn't have the checker either)

It would be good to get this included in Xen 4.17, especially that it
changes the internal ABI of xenctrl bindings.

Edwin Török (5):
  tools/ocaml/Makefile.rules: do not run ocamldep on distclean
  tools/ocaml/Makefile.rules: hide -include on *clean
  tools/ocaml/libs/eventchn: do not leak event channels and OCaml 5.0
    compat
  tools/ocaml/libs/xc: OCaml 5.0 compatibility
  tools/ocaml/libs/{xb, mmap}: use Data_abstract_val wrapper

 tools/ocaml/Makefile.rules                    |  4 +--
 tools/ocaml/libs/eventchn/xeneventchn_stubs.c | 29 +++++++++++++++++--
 tools/ocaml/libs/mmap/xenmmap_stubs.c         |  2 +-
 tools/ocaml/libs/xb/xs_ring_stubs.c           |  2 +-
 tools/ocaml/libs/xc/xenctrl_stubs.c           | 11 +++----
 5 files changed, 37 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH v2 1/5] tools/ocaml/Makefile.rules: do not run ocamldep on distclean

[Edwin Török]

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 tools/ocaml/Makefile.rules | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/ocaml/Makefile.rules b/tools/ocaml/Makefile.rules
index 0d3c6ac839..e0b9de34e4 100644
--- a/tools/ocaml/Makefile.rules
+++ b/tools/ocaml/Makefile.rules
@@ -44,7 +44,7 @@ META: META.in
 
 ALL_OCAML_OBJ_SOURCES=$(addsuffix .ml, $(ALL_OCAML_OBJS))
 
-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
-- 
2.34.1

[PATCH v2 2/5] tools/ocaml/Makefile.rules: hide -include on *clean

[Edwin Török]

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 tools/ocaml/Makefile.rules | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/ocaml/Makefile.rules b/tools/ocaml/Makefile.rules
index e0b9de34e4..39ac260a4d 100644
--- a/tools/ocaml/Makefile.rules
+++ b/tools/ocaml/Makefile.rules
@@ -44,10 +44,8 @@ META: META.in
 
 ALL_OCAML_OBJ_SOURCES=$(addsuffix .ml, $(ALL_OCAML_OBJS))
 
-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
 
 clean: $(CLEAN_HOOKS)
 	$(Q)rm -f .*.d *.o *.so *.a *.cmo *.cmi *.cma *.cmx *.cmxa *.annot *.spot *.spit $(LIBS) $(PROGRAMS) $(GENERATED_FILES) .ocamldep.make META
@@ -94,7 +92,9 @@ define C_PROGRAM_template
 	$(call quiet-command, $(CC) $(LDFLAGS) -o $$@ $$+,BIN,$$@)
 endef
 
+ifeq (,$(findstring clean,$(MAKECMDGOALS)))
 -include .ocamldep.make
+endif
 
 $(foreach lib,$(OCAML_LIBRARY),$(eval $(call OCAML_LIBRARY_template,$(lib))))
 $(foreach lib,$(OCAML_NOC_LIBRARY),$(eval $(call OCAML_NOC_LIBRARY_template,$(lib))))
-- 
2.34.1

[PATCH v2 3/5] tools/ocaml/libs/eventchn: do not leak event channels and OCaml 5.0 compat

[Edwin Török]

Add a finalizer on the event channel value, so that it calls
`xenevtchn_close` when the value would be GCed.

In practice oxenstored seems to be the only user of this,
and it creates a single global event channel only,
but freeing this could still be useful when run with OCAMLRUNPARAM=c

The code was previously casting a C pointer to an OCaml value,
which should be avoided: OCaml 5.0 won't support it.
(all "naked" C pointers must be wrapped inside an OCaml value,
 either an Abstract tag, or Nativeint, see the manual
 https://ocaml.org/manual/intfc.html#ss:c-outside-head)

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
Acked-by: Christian Lindig <christian.lindig@citrix.com>
---
 tools/ocaml/libs/eventchn/xeneventchn_stubs.c | 29 +++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
index f889a7a2e4..67af116377 100644
--- a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
+++ b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
@@ -33,7 +33,30 @@
 #include <caml/fail.h>
 #include <caml/signals.h>
 
-#define _H(__h) ((xenevtchn_handle *)(__h))
+/* We want to close the event channel when it is no longer in use,
+   which can only be done safely with a finalizer.
+   Event channels are typically long lived, so we don't need tighter control over resource deallocation.
+   Use a custom block
+*/
+
+/* Access the xenevtchn_t* part of the OCaml custom block */
+#define _H(__h) (*((xenevtchn_handle**)Data_custom_val(__h)))
+
+static void stub_evtchn_finalize(value v)
+{
+	/* docs say to not use any CAMLparam* macros here */
+	xenevtchn_close(_H(v));
+}
+
+static struct custom_operations xenevtchn_ops = {
+	"xenevtchn",
+	stub_evtchn_finalize,
+	custom_compare_default, /* raises Failure, cannot compare */
+	custom_hash_default, /* ignored */
+	custom_serialize_default, /* raises Failure, can't serialize */
+	custom_deserialize_default, /* raises Failure, can't deserialize */
+	custom_compare_ext_default /* raises Failure */
+};
 
 CAMLprim value stub_eventchn_init(void)
 {
@@ -48,7 +71,9 @@ CAMLprim value stub_eventchn_init(void)
 	if (xce == NULL)
 		caml_failwith("open failed");
 
-	result = (value)xce;
+	/* contains file descriptors, trigger full GC at least every 128 allocations */
+	result = caml_alloc_custom(&xenevtchn_ops, sizeof(xce), 0, 1);
+	_H(result) = xce;
 	CAMLreturn(result);
 }
 
-- 
2.34.1

[PATCH v2 4/5] tools/ocaml/libs/xc: OCaml 5.0 compatibility

[Edwin Török]

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(-)

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

[PATCH v2 5/5] tools/ocaml/libs/{xb, mmap}: use Data_abstract_val wrapper

[Edwin Török]

This is not strictly necessary since it is essentially a no-op
currently: a cast to void* and value*, even in OCaml 5.0.

However it does make it clearer that what we have here is not a regular
OCaml value, but one allocated with Abstract_tag or Custom_tag,
and follows the example from the manual more closely:
https://v2.ocaml.org/manual/intfc.html#ss:c-outside-head

It also makes it clearer that these modules have been reviewed for
compat with OCaml 5.0.

No functional change.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 tools/ocaml/libs/mmap/xenmmap_stubs.c | 2 +-
 tools/ocaml/libs/xb/xs_ring_stubs.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/ocaml/libs/mmap/xenmmap_stubs.c b/tools/ocaml/libs/mmap/xenmmap_stubs.c
index e2ce088e25..141dedb78c 100644
--- a/tools/ocaml/libs/mmap/xenmmap_stubs.c
+++ b/tools/ocaml/libs/mmap/xenmmap_stubs.c
@@ -28,7 +28,7 @@
 #include <caml/fail.h>
 #include <caml/callback.h>
 
-#define Intf_val(a) ((struct mmap_interface *) a)
+#define Intf_val(a) ((struct mmap_interface *) Data_abstract_val(a))
 
 static int mmap_interface_init(struct mmap_interface *intf,
                                int fd, int pflag, int mflag,
diff --git a/tools/ocaml/libs/xb/xs_ring_stubs.c b/tools/ocaml/libs/xb/xs_ring_stubs.c
index 7a91fdee75..cc9114029f 100644
--- a/tools/ocaml/libs/xb/xs_ring_stubs.c
+++ b/tools/ocaml/libs/xb/xs_ring_stubs.c
@@ -35,7 +35,7 @@
 #include <sys/mman.h>
 #include "mmap_stubs.h"
 
-#define GET_C_STRUCT(a) ((struct mmap_interface *) a)
+#define GET_C_STRUCT(a) ((struct mmap_interface *) Data_abstract_val(a))
 
 /*
  * Bytes_val has been introduced by Ocaml 4.06.1. So define our own version
-- 
2.34.1

Re: [PATCH v2 4/5] tools/ocaml/libs/xc: OCaml 5.0 compatibility

[Edwin Torok]

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&amp;data=05%7C01%7Cedvin.torok%40citrix.com%7C4bf28f7a32074a49cdf008daa079a807%7C335836de42ef43a2b145348c2ee9ca5b%7C0%7C0%7C637998741634627105%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=GU%2BbmB1ai0llQg0z5zWctzwW0ocUQUOHVlMxkeD3U0U%3D&amp;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
> 

Re: [PATCH v2 4/5] tools/ocaml/libs/xc: OCaml 5.0 compatibility

[Christian Lindig]

> 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

Re: [PATCH v2 4/5] tools/ocaml/libs/xc: OCaml 5.0 compatibility

[Edwin Torok]

> 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