| Message ID | 20200331103102.1105674-11-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: Build system improvements | expand |
On 31.03.2020 12:30, Anthony PERARD wrote: > --- a/xen/Rules.mk > +++ b/xen/Rules.mk > @@ -130,15 +130,24 @@ include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk > c_flags += $(CFLAGS-y) > a_flags += $(CFLAGS-y) $(AFLAGS-y) > > -built_in.o: $(obj-y) $(extra-y) > -ifeq ($(obj-y),) > - $(CC) $(c_flags) -c -x c /dev/null -o $@ > -else > +quiet_cmd_ld_builtin = LD $@ > ifeq ($(CONFIG_LTO),y) > - $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$^) > +cmd_ld_builtin = \ > + $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) > else > - $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$^) > +cmd_ld_builtin = \ > + $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) > endif How about going yet one step further and doing away with the ifeq here altogether: cmd_ld_builtin-y = \ $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) cmd_ld_builtin-$(CONFIG_LTO) = \ $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) > +quiet_cmd_cc_builtin = LD $@ > +cmd_cc_builtin = \ > + $(CC) $(XEN_CFLAGS) -c -x c /dev/null -o $@ > + > +built_in.o: $(obj-y) $(extra-y) FORCE > +ifeq ($(obj-y),) > + $(call if_changed,cc_builtin) > +else > + $(call if_changed,ld_builtin) $(call if_changed,ld_builtin-y) ? As an aside (not something you introduce) this makes it even more prominent that the use of $(XEN_LDFLAGS) is asymmetric here, for whatever reason (if any). Of course there's other redundancy between the two commands which could be eliminated. Jan
On 08/04/2020 13:40, Jan Beulich wrote: > On 31.03.2020 12:30, Anthony PERARD wrote: >> --- a/xen/Rules.mk >> +++ b/xen/Rules.mk >> @@ -130,15 +130,24 @@ include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk >> c_flags += $(CFLAGS-y) >> a_flags += $(CFLAGS-y) $(AFLAGS-y) >> >> -built_in.o: $(obj-y) $(extra-y) >> -ifeq ($(obj-y),) >> - $(CC) $(c_flags) -c -x c /dev/null -o $@ >> -else >> +quiet_cmd_ld_builtin = LD $@ >> ifeq ($(CONFIG_LTO),y) >> - $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$^) >> +cmd_ld_builtin = \ >> + $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) >> else >> - $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$^) >> +cmd_ld_builtin = \ >> + $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) >> endif > How about going yet one step further and doing away with the > ifeq here altogether: > > cmd_ld_builtin-y = \ > $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) > cmd_ld_builtin-$(CONFIG_LTO) = \ > $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) Please don't. Logic like this is substantially harder to follow than a plain if/else construct, and clarity is of far higher importance than optimising the line count in the build system. This trick only works for trivial cases, and interferes with diff's when the logic inevitably becomes less trivial. LTO support in particular needs a complete overhaul, but there is no way I'm going to touch that with a barge pole until this series is in place. ~Andrew
On 08.04.2020 15:13, Andrew Cooper wrote: > On 08/04/2020 13:40, Jan Beulich wrote: >> On 31.03.2020 12:30, Anthony PERARD wrote: >>> --- a/xen/Rules.mk >>> +++ b/xen/Rules.mk >>> @@ -130,15 +130,24 @@ include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk >>> c_flags += $(CFLAGS-y) >>> a_flags += $(CFLAGS-y) $(AFLAGS-y) >>> >>> -built_in.o: $(obj-y) $(extra-y) >>> -ifeq ($(obj-y),) >>> - $(CC) $(c_flags) -c -x c /dev/null -o $@ >>> -else >>> +quiet_cmd_ld_builtin = LD $@ >>> ifeq ($(CONFIG_LTO),y) >>> - $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$^) >>> +cmd_ld_builtin = \ >>> + $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) >>> else >>> - $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$^) >>> +cmd_ld_builtin = \ >>> + $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) >>> endif >> How about going yet one step further and doing away with the >> ifeq here altogether: >> >> cmd_ld_builtin-y = \ >> $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) >> cmd_ld_builtin-$(CONFIG_LTO) = \ >> $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) > > Please don't. > > Logic like this is substantially harder to follow than a plain if/else > construct, and clarity is of far higher importance than optimising the > line count in the build system. I could maybe see the argument if the two definitions were far apart. This suggestion isn't about line count at all, but about clarity. In particular because of the need to use ifeq(,) rather than simple "if" constructs, I view this list model as the better alternative in all cases where it can be made use of. > This trick only works for trivial cases, and interferes with diff's when > the logic inevitably becomes less trivial. It may, but whether it actually will can't be known until such time as it would get touched. The suggested model may very well also be suitable then. Anyway, Anthony, I'm not going to insist. This is just another aspect where we would better generally settle on the preferred style to use. Jan
On Wed, Apr 08, 2020 at 03:35:17PM +0200, Jan Beulich wrote: > On 08.04.2020 15:13, Andrew Cooper wrote: > > On 08/04/2020 13:40, Jan Beulich wrote: > >> On 31.03.2020 12:30, Anthony PERARD wrote: > >>> --- a/xen/Rules.mk > >>> +++ b/xen/Rules.mk > >>> @@ -130,15 +130,24 @@ include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk > >>> c_flags += $(CFLAGS-y) > >>> a_flags += $(CFLAGS-y) $(AFLAGS-y) > >>> > >>> -built_in.o: $(obj-y) $(extra-y) > >>> -ifeq ($(obj-y),) > >>> - $(CC) $(c_flags) -c -x c /dev/null -o $@ > >>> -else > >>> +quiet_cmd_ld_builtin = LD $@ > >>> ifeq ($(CONFIG_LTO),y) > >>> - $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$^) > >>> +cmd_ld_builtin = \ > >>> + $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) > >>> else > >>> - $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$^) > >>> +cmd_ld_builtin = \ > >>> + $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) > >>> endif > >> How about going yet one step further and doing away with the > >> ifeq here altogether: > >> > >> cmd_ld_builtin-y = \ > >> $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) > >> cmd_ld_builtin-$(CONFIG_LTO) = \ > >> $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) > > > > Please don't. > > > > Logic like this is substantially harder to follow than a plain if/else > > construct, and clarity is of far higher importance than optimising the > > line count in the build system. > > I could maybe see the argument if the two definitions were far apart. > This suggestion isn't about line count at all, but about clarity. In > particular because of the need to use ifeq(,) rather than simple "if" > constructs, I view this list model as the better alternative in all > cases where it can be made use of. We could use "ifdef CONFIG_LTO" to avoid ifeq ;-). But I think you disliked that because CONFIG_LTO could be present in the environment with a value different than "y" and mess up the build system, just to annoy us. > > This trick only works for trivial cases, and interferes with diff's when > > the logic inevitably becomes less trivial. > > It may, but whether it actually will can't be known until such time > as it would get touched. The suggested model may very well also be > suitable then. > > Anyway, Anthony, I'm not going to insist. This is just another aspect > where we would better generally settle on the preferred style to use. I think if/else is better for alternatives. And we can keep "var-y" for lists with optional items. Thanks,
diff --git a/xen/Rules.mk b/xen/Rules.mk index 5e668f5ba0d8..c744175fd6f0 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -130,15 +130,24 @@ include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk c_flags += $(CFLAGS-y) a_flags += $(CFLAGS-y) $(AFLAGS-y) -built_in.o: $(obj-y) $(extra-y) -ifeq ($(obj-y),) - $(CC) $(c_flags) -c -x c /dev/null -o $@ -else +quiet_cmd_ld_builtin = LD $@ ifeq ($(CONFIG_LTO),y) - $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$^) +cmd_ld_builtin = \ + $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) else - $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$^) +cmd_ld_builtin = \ + $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) endif + +quiet_cmd_cc_builtin = LD $@ +cmd_cc_builtin = \ + $(CC) $(XEN_CFLAGS) -c -x c /dev/null -o $@ + +built_in.o: $(obj-y) $(extra-y) FORCE +ifeq ($(obj-y),) + $(call if_changed,cc_builtin) +else + $(call if_changed,ld_builtin) endif targets += built_in.o
In the case where $(obj-y) is empty, we also replace $(c_flags) by $(XEN_CFLAGS) to avoid generating an .%.d dependency file. This avoid make trying to include %.h file in the ld command if $(obj-y) isn't empty anymore on a second run. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Notes: v4: - Have cmd_ld_builtin depends on CONFIG_LTO, which simplify built_in.o rule. xen/Rules.mk | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)