There are no .S intermediate files, so rework in terms of head-bin-objs.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>

I'm slightly -1 on this, because

  head-bin-objs := $(addprefix $(obj)/,$(head-bin-objs))

is substantial obfuscation which I'd prefer to bin.

Anthony: Why does dropping the targets += line interfere with incremental
builds?  With it gone, *.bin are regenerated unconditionally, but I can't see
what would cause that, nor why the normal dependencies on head.o don't work.
---
 xen/arch/x86/boot/Makefile | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 294ac2418583..527f3e393037 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -1,16 +1,17 @@
 obj-bin-y += head.o
-head-srcs := cmdline.S reloc.S
 
-nocov-y += $(head-srcs:.S=.o)
-noubsan-y += $(head-srcs:.S=.o)
-targets += $(head-srcs:.S=.o)
+head-bin-objs := cmdline.o reloc.o
 
-head-srcs := $(addprefix $(obj)/, $(head-srcs))
+nocov-y += $(head-bin-objs)
+noubsan-y += $(head-bin-objs)
+targets += $(head-bin-objs)
+
+head-bin-objs := $(addprefix $(obj)/,$(head-bin-objs))
 
 # For .incbin - add $(obj) to the include path and add the dependencies
 # manually as they're not included in .d
 $(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
-$(obj)/head.o: $(head-srcs:.S=.bin)
+$(obj)/head.o: $(head-bin-objs:.o=.bin)
 
 CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
 $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
@@ -18,8 +19,8 @@ CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float
 CFLAGS_x86_32 += -I$(srctree)/include
 
 # override for 32bit binaries
-$(head-srcs:.S=.o): CFLAGS_stack_boundary :=
-$(head-srcs:.S=.o): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
+$(head-bin-objs): CFLAGS_stack_boundary :=
+$(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
 
 %.bin: %.lnk
 	$(OBJCOPY) -j .text -O binary $< $@
@@ -27,4 +28,4 @@ $(head-srcs:.S=.o): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
 %.lnk: %.o $(src)/build32.lds
 	$(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) -N -T $(filter %.lds,$^) -o $@ $<
 
-clean-files := cmdline.S reloc.S *.lnk *.bin
+clean-files := *.lnk *.bin
-- 
2.11.0

On 14.04.2022 13:47, Andrew Cooper wrote:
> There are no .S intermediate files, so rework in terms of head-bin-objs.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Anthony PERARD <anthony.perard@citrix.com>
> 
> I'm slightly -1 on this, because
> 
>   head-bin-objs := $(addprefix $(obj)/,$(head-bin-objs))
> 
> is substantial obfuscation which I'd prefer to bin.

Would ...

> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -1,16 +1,17 @@
>  obj-bin-y += head.o
> -head-srcs := cmdline.S reloc.S
>  
> -nocov-y += $(head-srcs:.S=.o)
> -noubsan-y += $(head-srcs:.S=.o)
> -targets += $(head-srcs:.S=.o)
> +head-bin-objs := cmdline.o reloc.o

head-bin-objs := $(obj)/cmdline.o $(obj)/reloc.o

> -head-srcs := $(addprefix $(obj)/, $(head-srcs))
> +nocov-y += $(head-bin-objs)
> +noubsan-y += $(head-bin-objs)
> +targets += $(head-bin-objs)

nocov-y += $(notdir $(head-bin-objs))

etc perhaps be a little less obfuscation?

Jan

On Thu, Apr 14, 2022 at 12:47:08PM +0100, Andrew Cooper wrote:
> There are no .S intermediate files, so rework in terms of head-bin-objs.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

The patch looks fine.

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

> ---
> I'm slightly -1 on this, because
> 
>   head-bin-objs := $(addprefix $(obj)/,$(head-bin-objs))
> 
> is substantial obfuscation which I'd prefer to bin.

It might be possible to do something that Kbuild does, which would be to
teach the build system to look for "$(head-objs)" or maybe
"$(head-bin-objs)" when it want to build "head.o". That something that's
done in Kbuild I think to build a module from several source files.

> Anthony: Why does dropping the targets += line interfere with incremental
> builds?  With it gone, *.bin are regenerated unconditionally, but I can't see
> what would cause that, nor why the normal dependencies on head.o don't work.

Try to build with "make V=2", make will display why a target is been
rebuild (when this target is built with $(if_changed, )

$(targets) is used by Rules.mk to findout which dependencies files (the
.cmd) to load and only load them if the target exist. Then the
$(if_changed, ) macro rerun the command if prereq are newer than the
target or if the command as changed. Without the .cmd file loaded, the
macro would compare the new command to an empty value and so rebuild the
target.

Now, the *.bin files are regenerated because cmdline.o is been rebuild
mostly because make didn't load the record of the previous command run.

Thanks,

-- 
Anthony PERARD

On 14/04/2022 18:45, Anthony PERARD wrote:
> On Thu, Apr 14, 2022 at 12:47:08PM +0100, Andrew Cooper wrote:
>> There are no .S intermediate files, so rework in terms of head-bin-objs.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> The patch looks fine.
>
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
>
>> ---
>> I'm slightly -1 on this, because
>>
>>   head-bin-objs := $(addprefix $(obj)/,$(head-bin-objs))
>>
>> is substantial obfuscation which I'd prefer to bin.
> It might be possible to do something that Kbuild does, which would be to
> teach the build system to look for "$(head-objs)" or maybe
> "$(head-bin-objs)" when it want to build "head.o". That something that's
> done in Kbuild I think to build a module from several source files.
>
>> Anthony: Why does dropping the targets += line interfere with incremental
>> builds?  With it gone, *.bin are regenerated unconditionally, but I can't see
>> what would cause that, nor why the normal dependencies on head.o don't work.
> Try to build with "make V=2", make will display why a target is been
> rebuild (when this target is built with $(if_changed, )
>
> $(targets) is used by Rules.mk to findout which dependencies files (the
> .cmd) to load and only load them if the target exist. Then the
> $(if_changed, ) macro rerun the command if prereq are newer than the
> target or if the command as changed. Without the .cmd file loaded, the
> macro would compare the new command to an empty value and so rebuild the
> target.
>
> Now, the *.bin files are regenerated because cmdline.o is been rebuild
> mostly because make didn't load the record of the previous command run.

I'm not certain if this case is a match with Linux's module logic.  The
module logic is "compile each .c file, then link all the .o's together
into one .ko".

In this case, we're saying "to assemble head.S to head.o, you first need
to build {cmdline,reloc}.bin so the incbin doesn't explode".  I guess it
depends how generic the "$X depends on arbitrary $Y's" expression can be
made.

Between this patch an the previous one, I've clearly got mixed up with
what exactly the target+= and regular dependencies.

The comment specifically refers to the fact that the old #include
"cmdline.S" used to show up as a dep in .head.o.cmd, whereas .incbin
doesn't.  (Not surprising, because -M and friends are from the
preprocessor, not assembler, but it would be helpful if this limitation
didn't exist.)  As a consequence, the dependency needs adding back in
somehow.

From your description above, I assume that simply being listed as a dep
isn't good enough to trigger a recursive load of the .bin's .cmd file
(not that there is one), which is why they need adding specially to targets?

As I have simplified this to (almost) normal build runes, should we be
expressing it differently now to fit in with the new way of doing things?

~Andrew