[3/3] tools/ocaml: Introduce xenctrl ABI build-time checks
diff mbox series

Message ID 20190909171206.25585-3-ian.jackson@eu.citrix.com
State New
Headers show
Series
  • [1/3] tools/ocaml: Add missing X86_EMU_VPCI
Related show

Commit Message

Ian Jackson Sept. 9, 2019, 5:12 p.m. UTC
c/s f089fddd941 broke the Ocaml ABI by renumering
XEN_SYSCTL_PHYSCAP_directio without adjusting the Ocaml
physinfo_cap_flag enumeration.

Add build machinery which will check the ABI correspondence.

This will result in a compile time failure whenever constants get
renumbered/added without a compatible adjustment to the Ocaml ABI.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
v2: Script with ad-hoc parsers rather than handwritten BUILD_BUG_ON
    list (which was out of date even in v1 of this patch!)
---
 .gitignore                          |  1 +
 tools/ocaml/libs/xc/Makefile        |  5 +++
 tools/ocaml/libs/xc/abi-check       | 84 +++++++++++++++++++++++++++++++++++++
 tools/ocaml/libs/xc/xenctrl_stubs.c | 69 +++++++++++++++++++++---------
 xen/include/public/sysctl.h         |  4 ++
 5 files changed, 143 insertions(+), 20 deletions(-)
 create mode 100755 tools/ocaml/libs/xc/abi-check

Comments

Christian Lindig Sept. 10, 2019, 8:29 a.m. UTC | #1
> On 9 Sep 2019, at 18:12, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
> 
> 
> Add build machinery which will check the ABI correspondence.
> 
> This will result in a compile time failure whenever constants get
> renumbered/added without a compatible adjustment to the Ocaml ABI.

I understand the desire to automate this but would have kept the original proposal for these reasons: changes are rare enough, it is obvious how to extend the scheme, the approach stayed well within the respective languages. Adding parsers and code generators to the build system will make it more difficult to improve it, which at least for the OCaml part is very desirable. However, I’m not going to object to the patch.

Acked-by: Christian Lindig <christian.lindig@citrix.com>

— Christian
Ian Jackson Sept. 10, 2019, 9:57 a.m. UTC | #2
Christian Lindig writes ("Re: [PATCH 3/3] tools/ocaml: Introduce xenctrl ABI build-time checks"):
> I understand the desire to automate this but would have kept the
> original proposal for these reasons: changes are rare enough, it is
> obvious how to extend the scheme, the approach stayed well within
> the respective languages. Adding parsers and code generators to the
> build system will make it more difficult to improve it, which at
> least for the OCaml part is very desirable. However, I’m not going
> to object to the patch.

I would love to have this all done in a more "proper" way.

However, in the meantime I do think it is essential to have a check
which actually ties the ocaml code right back to the Xen headers (the
latter being the canonical ABI definition).  Indeed the v1 patch in
this very thread had a mismatch between the ocaml and the BUILD_BUG_ON
list, which I only discovered when my checker threw an unexpected
error.

If there are other language bindings which have similar issues we
should check them too.

Ultimately I would prefer it if the Xen ABI were provided in a way
that was useable for automatically generating bindings.

> Acked-by: Christian Lindig <christian.lindig@citrix.com>

Thanks,
Ian.

Patch
diff mbox series

diff --git a/.gitignore b/.gitignore
index 3c947ac948..3ada0c4f0b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -392,6 +392,7 @@  tools/ocaml/libs/xentoollog/_xtl_levels.*
 tools/ocaml/libs/xentoollog/xentoollog.ml
 tools/ocaml/libs/xentoollog/xentoollog.mli
 tools/ocaml/libs/xs/paths.ml
+tools/ocaml/libs/xc/xenctrl_abi_check.h
 tools/ocaml/xenstored/_paths.h
 tools/ocaml/xenstored/oxenstored
 tools/ocaml/xenstored/oxenstored.conf
diff --git a/tools/ocaml/libs/xc/Makefile b/tools/ocaml/libs/xc/Makefile
index d24b0144d0..ac780627d2 100644
--- a/tools/ocaml/libs/xc/Makefile
+++ b/tools/ocaml/libs/xc/Makefile
@@ -31,4 +31,9 @@  install: $(LIBS) META
 uninstall:
 	$(OCAMLFIND) remove -destdir $(OCAMLDESTDIR) xenctrl
 
+xenctrl_stubs.o: xenctrl_abi_check.h
+
+xenctrl_abi_check.h: abi-check xenctrl_stubs.c xenctrl.ml
+	$(PERL) -w $^ >$@.tmp && mv -f $@.tmp $@
+
 include $(TOPLEVEL)/Makefile.rules
diff --git a/tools/ocaml/libs/xc/abi-check b/tools/ocaml/libs/xc/abi-check
new file mode 100755
index 0000000000..c987cd8454
--- /dev/null
+++ b/tools/ocaml/libs/xc/abi-check
@@ -0,0 +1,84 @@ 
+#!/usr/bin/perl -w
+
+use strict;
+use Data::Dumper;
+
+our %enums;
+
+@ARGV == 2 or die;
+our ($c, $o) = @ARGV;
+
+open STDIN, "<", $c or die $!;
+
+our $cline = -1;
+our $ei;
+
+while (<>) {
+    if ($cline == -1) {
+        if (m/c_bitmap_to_ocaml_list/) {
+            $cline = 0;
+            $ei = { };
+        }
+    } else {
+        $cline++;
+        m{^\s+/\* \s+ ! \s+ (.*?) \s* \*/\s*$}x or die "$cline $_ ?";
+        my @vals = split /\s+/, $1;
+        if ($cline == 1 && !@vals) {
+            $cline = -1;
+        } elsif ($cline == 1 && @vals == 3) {
+            $ei->{$_} = shift @vals foreach qw(OType OPrefix Mangle);
+        } elsif ($cline == 2 && @vals == 3) {
+            $ei->{$_} = shift @vals foreach qw(CPrefix CFinal CFinalHow);
+            die if $enums{ $ei->{OType} };
+            $enums{ $ei->{OType} } = $ei;
+            $cline = -1;
+        } else {
+            die "$_ ?";
+        }
+    }
+}
+
+sub expect ($$) {
+    printf "BUILD_BUG_ON( %-30s != %-10s );\n", @_ or die $!;
+}
+
+open STDIN, "<", $o or die $!;
+my $cval;
+$ei = undef;
+my $bitnum = 0;
+while (<>) {
+    if (!$ei) {
+        if (m{^type \s+ (\w+) \s* \= \s* $/}x && $enums{$1}) {
+            $ei = $enums{$1};
+            $cval = '';
+            $bitnum = 0;
+        }
+    } else {
+        if (m{^\s+ \| \s* $ei->{OPrefix} (\w+) \s*$}x) {
+            $cval = $1;
+            if ($ei->{Mangle} eq 'lc') {
+                $cval = lc $cval;
+            } elsif ($ei->{Mangle} eq 'none') {
+            } else {
+                die;
+            }
+            $cval = $ei->{CPrefix}.$cval;
+            expect($cval, "(1u << $bitnum)");
+            $bitnum++;
+        } elsif (m/^\w|\{/) {
+            if ($ei->{CFinalHow} eq 'max') {
+                expect($ei->{CFinal}, "(1u << ".($bitnum-1).")");
+            } elsif ($ei->{CFinalHow} eq 'all') {
+                expect($ei->{CFinal}, "(1u << $bitnum)-1u");
+            } else {
+                die Dumper($ei)." ?";
+            }
+            $ei = undef;
+        } elsif (!m{\S}) {
+        } else {
+            die "$_ ?";
+        }
+    }
+}
+
+close STDOUT or die $!;
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 2e1b29ce33..352a6bd2d6 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -32,6 +32,7 @@ 
 
 #define XC_WANT_COMPAT_MAP_FOREIGN_API
 #include <xenctrl.h>
+#include <xen-tools/libs.h>
 
 #include "mmap_stubs.h"
 
@@ -119,6 +120,39 @@  static void domain_handle_of_uuid_string(xen_domain_handle_t h,
 #undef X
 }
 
+/*
+ * Various fields which are a bitmap in the C ABI are converted to lists of
+ * integers in the Ocaml ABI for more idiomatic handling.
+ */
+static value c_bitmap_to_ocaml_list
+             /* ! */
+             /*
+	      * All calls to this function must be in a form suitable
+	      * for xenctrl_abi_check.  The parsing there is ad-hoc.
+	      */
+             (unsigned int bitmap)
+{
+	CAMLparam0();
+	CAMLlocal2(list, tmp);
+
+#include "xenctrl_abi_check.h"
+
+	list = tmp = Val_emptylist;
+
+	for ( unsigned int i = 0; bitmap; i++, bitmap >>= 1 )
+	{
+		if ( !(bitmap & 1) )
+			continue;
+
+		tmp = caml_alloc_small(2, Tag_cons);
+		Field(tmp, 0) = Val_int(i);
+		Field(tmp, 1) = list;
+		list = tmp;
+	}
+
+	CAMLreturn(list);
+}
+
 CAMLprim value stub_xc_domain_create(value xch, value config)
 {
 	CAMLparam2(xch, config);
@@ -315,16 +349,13 @@  static value alloc_domaininfo(xc_domaininfo_t * info)
 	Store_field(result, 15, tmp);
 
 #if defined(__i386__) || defined(__x86_64__)
-	/* emulation_flags: x86_arch_emulation_flags list; */
-	tmp = emul_list = Val_emptylist;
-	for (i = 0; i < 10; i++) {
-		if ((info->arch_config.emulation_flags >> i) & 1) {
-			tmp = caml_alloc_small(2, Tag_cons);
-			Field(tmp, 0) = Val_int(i);
-			Field(tmp, 1) = emul_list;
-			emul_list = tmp;
-		}
-	}
+	/*
+	 * emulation_flags: x86_arch_emulation_flags list;
+	 */
+	emul_list = c_bitmap_to_ocaml_list
+		/* ! x86_arch_emulation_flags X86_EMU_ none */
+		/* ! XEN_X86_EMU_ XEN_X86_EMU_ALL all */
+		(info->arch_config.emulation_flags);
 
 	/* xen_x86_arch_domainconfig */
 	x86_arch_config = caml_alloc_tuple(1);
@@ -635,7 +666,7 @@  CAMLprim value stub_xc_send_debug_keys(value xch, value keys)
 CAMLprim value stub_xc_physinfo(value xch)
 {
 	CAMLparam1(xch);
-	CAMLlocal3(physinfo, cap_list, tmp);
+	CAMLlocal2(physinfo, cap_list);
 	xc_physinfo_t c_physinfo;
 	int r;
 
@@ -646,15 +677,13 @@  CAMLprim value stub_xc_physinfo(value xch)
 	if (r)
 		failwith_xc(_H(xch));
 
-	tmp = cap_list = Val_emptylist;
-	for (r = 0; r < 2; r++) {
-		if ((c_physinfo.capabilities >> r) & 1) {
-			tmp = caml_alloc_small(2, Tag_cons);
-			Field(tmp, 0) = Val_int(r);
-			Field(tmp, 1) = cap_list;
-			cap_list = tmp;
-		}
-	}
+	/*
+	 * capabilities: physinfo_cap_flag list;
+	 */
+	cap_list = c_bitmap_to_ocaml_list
+		/* ! physinfo_cap_flag CAP_ lc */
+		/* ! XEN_SYSCTL_PHYSCAP_ XEN_SYSCTL_PHYSCAP_MAX max */
+		(c_physinfo.capabilities);
 
 	physinfo = caml_alloc_tuple(10);
 	Store_field(physinfo, 0, Val_int(c_physinfo.threads_per_core));
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 36b3f8c429..5401f9c2fe 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -90,6 +90,10 @@  struct xen_sysctl_tbuf_op {
  /* The platform supports direct access to I/O devices with IOMMU. */
 #define _XEN_SYSCTL_PHYSCAP_directio     2
 #define XEN_SYSCTL_PHYSCAP_directio  (1u<<_XEN_SYSCTL_PHYSCAP_directio)
+
+/* Max XEN_SYSCTL_PHYSCAP_* constant.  Used for ABI checking. */
+#define XEN_SYSCTL_PHYSCAP_MAX XEN_SYSCTL_PHYSCAP_directio
+
 struct xen_sysctl_physinfo {
     uint32_t threads_per_core;
     uint32_t cores_per_socket;