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>
---
 tools/ocaml/Makefile.rules | 2 +-
 tools/ocaml/common.make    | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

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

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

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