Message ID | fd09742f7c2191be3cdfafbd4c7cccb10eb0e3a2.1698240589.git.edwin.torok@cloud.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | tools/ocaml/Makefile.rules: use correct C flags when compiling OCaml C stubs | expand |
On Wed, Oct 25, 2023 at 2:58 PM Edwin Török <edwin.torok@cloud.com> wrote: > [...] > Signed-off-by: Edwin Török <edwin.torok@cloud.com> Sorry about the duplicate emails to the list, my 'git send-email' isn't working quite right.
> On 25 Oct 2023, at 14:52, Edwin Török <edvin.torok@citrix.com> wrote: > > From: Edwin Török <edwin.torok@cloud.com> > > The code currently uses GCC to compile OCaml C stubs directly, > and although in most cases this works, it is not entirely correct. > > This will fail if the OCaml runtime has been recompiled to use and link with ASAN for example > (or other situations where a flag needs to be used consistently in everything that is linked into the same binary). > > Use the OCaml compiler instead, which knows how to invoke the correct C compiler with the correct flags, > and append the Xen specific CFLAGS to that instead. > > Drop the explicit -fPIC and -I$(ocamlc -where): these will now be provided by the compiler as needed. > > Use -verbose so we see the actuall full C compiler command line invocation done by the OCaml compiler. > > Signed-off-by: Edwin Török <edwin.torok@cloud.com> Acked-by: Christian Lindig <christian.lindig@cloud.com> I like using the OCaml compiler to compile stubs as it knows how to handle C files and will invoke the C compiler with the correct flags. However, this is the kind of change that would be good to have tested on all supported platforms. I therefore invite comments from those who maintain the build process. — C
On 25.10.2023 15:52, Edwin Török wrote: > --- a/tools/ocaml/Makefile.rules > +++ b/tools/ocaml/Makefile.rules > @@ -37,7 +37,7 @@ ALL_OCAML_OBJS ?= $(OBJS) > $(call quiet-command, $(OCAMLYACC) -q $<,MLYACC,$@) > > %.o: %.c > - $(call quiet-command, $(CC) $(CFLAGS) -c -o $@ $<,CC,$@) > + $(call quiet-command, $(OCAMLOPT) -verbose $(addprefix -ccopt ,$(CFLAGS)) -c -o $@ $<,CC,$@) Wouldn't -verbose better be passed only if the build isn't a quiet one? Jan
> On 25 Oct 2023, at 15:04, Jan Beulich <jbeulich@suse.com> wrote: > > On 25.10.2023 15:52, Edwin Török wrote: >> --- a/tools/ocaml/Makefile.rules >> +++ b/tools/ocaml/Makefile.rules >> @@ -37,7 +37,7 @@ ALL_OCAML_OBJS ?= $(OBJS) >> $(call quiet-command, $(OCAMLYACC) -q $<,MLYACC,$@) >> >> %.o: %.c >> - $(call quiet-command, $(CC) $(CFLAGS) -c -o $@ $<,CC,$@) >> + $(call quiet-command, $(OCAMLOPT) -verbose $(addprefix -ccopt ,$(CFLAGS)) -c -o $@ $<,CC,$@) > > Wouldn't -verbose better be passed only if the build isn't a quiet one? Only the OCaml files (and the hypervisor itself) are compiled in quiet mode. It looks like tools/ and the C files in tools/ocaml were not, so the patch as is preserves the existing behaviour. I think there were some patches to switch to a non-recursive make, I hope that'll make quiet mode more consistent throughout the tree, until then I'd keep it as is instead of complicating the macro more. Thanks, --Edwin
> On 25 Oct 2023, at 15:02, Christian Lindig <christian.lindig@cloud.com> wrote: > > > >> On 25 Oct 2023, at 14:52, Edwin Török <edvin.torok@citrix.com> wrote: >> >> From: Edwin Török <edwin.torok@cloud.com> >> >> The code currently uses GCC to compile OCaml C stubs directly, >> and although in most cases this works, it is not entirely correct. >> >> This will fail if the OCaml runtime has been recompiled to use and link with ASAN for example >> (or other situations where a flag needs to be used consistently in everything that is linked into the same binary). >> >> Use the OCaml compiler instead, which knows how to invoke the correct C compiler with the correct flags, >> and append the Xen specific CFLAGS to that instead. >> >> Drop the explicit -fPIC and -I$(ocamlc -where): these will now be provided by the compiler as needed. >> >> Use -verbose so we see the actuall full C compiler command line invocation done by the OCaml compiler. >> >> Signed-off-by: Edwin Török <edwin.torok@cloud.com> > > Acked-by: Christian Lindig <christian.lindig@cloud.com> > > I like using the OCaml compiler to compile stubs as it knows how to handle C files and will invoke the C compiler with the correct flags. However, this is the kind of change that would be good to have tested on all supported platforms. I therefore invite comments from those who maintain the build process. There was a CI failure. I've pushed an updated version of this patch here: https://gitlab.com/xen-project/people/edwintorok/xen/-/commit/137ffad324eb82884e7ac6f46f618459d365693e If it passes the CI this time I'll send out a V2, looks like the -fPIC flag is needed on some platforms and is not automatically added by the OCaml compiler. Best regards, --Edwin
On 26.10.2023 19:38, Edwin Torok wrote: >> On 25 Oct 2023, at 15:04, Jan Beulich <jbeulich@suse.com> wrote: >> On 25.10.2023 15:52, Edwin Török wrote: >>> --- a/tools/ocaml/Makefile.rules >>> +++ b/tools/ocaml/Makefile.rules >>> @@ -37,7 +37,7 @@ ALL_OCAML_OBJS ?= $(OBJS) >>> $(call quiet-command, $(OCAMLYACC) -q $<,MLYACC,$@) >>> >>> %.o: %.c >>> - $(call quiet-command, $(CC) $(CFLAGS) -c -o $@ $<,CC,$@) >>> + $(call quiet-command, $(OCAMLOPT) -verbose $(addprefix -ccopt ,$(CFLAGS)) -c -o $@ $<,CC,$@) >> >> Wouldn't -verbose better be passed only if the build isn't a quiet one? > > Only the OCaml files (and the hypervisor itself) are compiled in quiet mode. It looks like tools/ and the C files in tools/ocaml were not, > so the patch as is preserves the existing behaviour. Yes and no. There's also make's -s flag, which I consider being wrongly overridden by passing -verbose here. But anyway ... Jan
diff --git a/tools/ocaml/Makefile.rules b/tools/ocaml/Makefile.rules index 0d3c6ac839..74856e2282 100644 --- a/tools/ocaml/Makefile.rules +++ b/tools/ocaml/Makefile.rules @@ -37,7 +37,7 @@ ALL_OCAML_OBJS ?= $(OBJS) $(call quiet-command, $(OCAMLYACC) -q $<,MLYACC,$@) %.o: %.c - $(call quiet-command, $(CC) $(CFLAGS) -c -o $@ $<,CC,$@) + $(call quiet-command, $(OCAMLOPT) -verbose $(addprefix -ccopt ,$(CFLAGS)) -c -o $@ $<,CC,$@) META: META.in sed 's/@VERSION@/$(VERSION)/g' < $< $o diff --git a/tools/ocaml/common.make b/tools/ocaml/common.make index 0c8a597d5b..629e4b3e66 100644 --- a/tools/ocaml/common.make +++ b/tools/ocaml/common.make @@ -9,8 +9,6 @@ OCAMLLEX ?= ocamllex OCAMLYACC ?= ocamlyacc OCAMLFIND ?= ocamlfind -CFLAGS += -fPIC -I$(shell ocamlc -where) - OCAMLOPTFLAG_G := $(shell $(OCAMLOPT) -h 2>&1 | sed -n 's/^ *\(-g\) .*/\1/p') OCAMLOPTFLAGS = $(OCAMLOPTFLAG_G) -ccopt "$(LDFLAGS)" -dtypes $(OCAMLINCLUDE) -cc $(CC) -w F -warn-error F OCAMLCFLAGS += -g $(OCAMLINCLUDE) -w F -warn-error F