| Message ID | 20220414114708.4788-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/build: Clean up 32bit boot objects | expand |
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,
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
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
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
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(-)