[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] tools/ocaml: Factor out compatiblity handling
On 28/08/2024 3:15 pm, Edwin Torok wrote: > On Wed, Aug 28, 2024 at 2:30 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > wrote: >> ... rather than having each library implement its own subset. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> >> --- >> CC: Christian Lindig <christian.lindig@xxxxxxxxxx> >> CC: David Scott <dave@xxxxxxxxxx> >> CC: Edwin Török <edwin.torok@xxxxxxxxx> >> CC: Rob Hoes <Rob.Hoes@xxxxxxxxxx> >> CC: Andrii Sultanov <andrii.sultanov@xxxxxxxxx> >> >> Broken out of a larger series, to help Andrii with his dynlib work. >> --- >> tools/ocaml/libs/xc/Makefile | 2 +- >> tools/ocaml/libs/xc/xenctrl_stubs.c | 13 +++---------- >> tools/ocaml/libs/xen-caml-compat.h | 23 +++++++++++++++++++++++ >> 3 files changed, 27 insertions(+), 11 deletions(-) >> create mode 100644 tools/ocaml/libs/xen-caml-compat.h >> >> diff --git a/tools/ocaml/libs/xc/Makefile b/tools/ocaml/libs/xc/Makefile >> index 1d9fecb06ef2..cdf4d01dac52 100644 >> --- a/tools/ocaml/libs/xc/Makefile >> +++ b/tools/ocaml/libs/xc/Makefile >> @@ -2,7 +2,7 @@ OCAML_TOPLEVEL=$(CURDIR)/../.. >> XEN_ROOT=$(OCAML_TOPLEVEL)/../.. >> include $(OCAML_TOPLEVEL)/common.make >> >> -CFLAGS += -I../mmap $(CFLAGS_libxenctrl) $(CFLAGS_libxenguest) >> +CFLAGS += -I../ -I../mmap $(CFLAGS_libxenctrl) $(CFLAGS_libxenguest) >> CFLAGS += $(APPEND_CFLAGS) >> OCAMLINCLUDE += -I ../mmap -I ../eventchn >> >> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c >> b/tools/ocaml/libs/xc/xenctrl_stubs.c >> index a52908012960..c78191f95abc 100644 >> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c >> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c >> @@ -25,6 +25,8 @@ >> #include <caml/fail.h> >> #include <caml/callback.h> >> >> +#include "xen-caml-compat.h" >> + >> #include <sys/mman.h> >> #include <stdint.h> >> #include <string.h> >> @@ -37,14 +39,6 @@ >> >> #include "mmap_stubs.h" >> >> -#ifndef Val_none >> -#define Val_none (Val_int(0)) >> -#endif >> - >> -#ifndef Tag_some >> -#define Tag_some 0 >> -#endif >> - >> static inline xc_interface *xch_of_val(value v) >> { >> xc_interface *xch = *(xc_interface **)Data_custom_val(v); >> @@ -744,8 +738,7 @@ CAMLprim value stub_xc_evtchn_status(value xch_val, >> value domid, value port) >> Store_field(result_status, 0, Val_int(status.vcpu)); >> Store_field(result_status, 1, stat); >> >> - result = caml_alloc_small(1, Tag_some); >> - Store_field(result, 0, result_status); >> + result = caml_alloc_some(result_status); >> >> CAMLreturn(result); >> } >> diff --git a/tools/ocaml/libs/xen-caml-compat.h >> b/tools/ocaml/libs/xen-caml-compat.h >> new file mode 100644 >> index 000000000000..b4a0ca4ce90c >> --- /dev/null >> +++ b/tools/ocaml/libs/xen-caml-compat.h >> @@ -0,0 +1,23 @@ >> +/* SPDX-License-Identifier: LGPL-2.1-only WITH OCaml-LGPL-linking-exception >> */ >> +#ifndef XEN_CAML_COMPAT_H >> +#define XEN_CAML_COMPAT_H >> + >> +#ifndef Val_none /* Option handling. Compat for Ocaml < 4.12 */ >> + >> +#define Val_none Val_int(0) >> +#define Tag_some 0 >> +#define Some_val(v) Field(v, 0) >> + >> +static inline value caml_alloc_some(value v) >> +{ >> + CAMLparam1(v); >> + >> + value some = caml_alloc_small(1, Tag_some); >> + Store_field(some, 0, v); > The compiler uses Field() rather than Store_field() here. > I think using Store_field here can potentially read uninitialized > data, because 'caml_alloc_small' gives you uninitialized memory > that you must immediately fill with valid values. > Looking at the implementation Store_field calls caml_modify which will > read the old value to figure out whether it was in minor or major > heap, > and doing that on uninitialized data is unpredictable. > > We should follow what the manual says and use Field() when > caml_alloc_small() is used, and use Store_field() when caml_alloc() is > used, > and not attempt to mix them: > See https://ocaml.org/manual/5.2/intfc.html#ss:c-low-level-gc-harmony Lovely, this got changed in Ocaml with no information or justification... https://github.com/ocaml/ocaml/pull/9819 I'll resync this locally, but I shaltn't repost. ~Andrew
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |