[v3,03/12] tools/ocaml: Introduce xenctrl ABI build-time checks
diff mbox series

Message ID 20190910120207.10358-4-ian.jackson@eu.citrix.com
State New
Headers show
Series
  • ocaml abi fixes
Related show

Commit Message

Ian Jackson Sept. 10, 2019, 12:01 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>
Acked-by: Christian Lindig <christian.lindig@citrix.com>
---
 .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

Jan Beulich Sept. 10, 2019, 12:17 p.m. UTC | #1
On 10.09.2019 14:01, Ian Jackson wrote:
> 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>
> Acked-by: Christian Lindig <christian.lindig@citrix.com>

Just in case you want an extra ack for the tiny sysctl.h addition:
Acked-by: Jan Beulich <jbeulich@suse.com>

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

I don't particularly like such "max" values put in (public) headers
(as they require constant updating), but I can't see a good
alternative either.

Jan
Andrew Cooper Sept. 10, 2019, 12:18 p.m. UTC | #2
On 10/09/2019 13:17, Jan Beulich wrote:
> On 10.09.2019 14:01, Ian Jackson wrote:
>> 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>
>> Acked-by: Christian Lindig <christian.lindig@citrix.com>
> Just in case you want an extra ack for the tiny sysctl.h addition:
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
>> --- 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
> I don't particularly like such "max" values put in (public) headers
> (as they require constant updating), but I can't see a good
> alternative either.

This is the best I could come up with.  At least it is in a
tools-restricted header.

~Andrew

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;