Re: [Qemu-devel] [RFC PATCH v2 3/6] Makefile: introduce common-obj-m and block-obj-m for DSO

[Fam Zheng]

On Fri, 09/06 14:11, Paolo Bonzini wrote:
> Il 06/09/2013 13:47, Fam Zheng ha scritto:
> > On Fri, 09/06 12:09, Paolo Bonzini wrote:
> >> Il 06/09/2013 09:28, Fam Zheng ha scritto:
> >>> Add necessary rules and flags for shared object generation.
> >>> $(common-obj-m) will include $(block-obj-m), like $(common-obj-y) does
> >>> for $(block-obj-y). The new rules introduced here are:
> >>>
> >>> 0) For all %.so compiling:
> >>>
> >>>     QEMU_CFLAGS += -shared -fPIC
> >>>
> >>> 1) %.o in $(common-obj-m) is compiled to %.o, with "QEMU_CFLAGS +=
> >>> -shared -fPIC". Then linked to %.so.
> >>>
> >>> 2) %.mo in $(common-obj-m) is the placeholder for %.so for pattern
> >>> matching in Makefile. It's linked to "-shared" with all its dependencies
> >>> (multiple *.o) as input. Which means the list of depended objects must
> >>> be ruled out in each sub-Makefile.objs with an variable:
> >>>
> >>>     $(obj)/foo.mo-obj := $(addprefix $(obj)/,bar.o baz.o qux.o)
> >>>
> >>> Notice that $(obj)/ is required for both target and dependency in the
> >>> rule. DSO suffix (.so) is configure variable (.dll for Windows).
> >>
> >> Some kinks to iron, but this is really well-architected.  You took all
> >> the best parts of mjt's patches and fixed almost all the ugly ones.  Kudos!
> >>
> >>> Signed-off-by: Fam Zheng <address@hidden>
> >>> ---
> >>>  Makefile      | 32 +++++++++++++++++++++++++++++---
> >>>  Makefile.objs | 14 +++++++++++++-
> >>>  configure     |  3 +++
> >>>  rules.mak     | 10 ++++++++++
> >>>  4 files changed, 55 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/Makefile b/Makefile
> >>> index 806946e..cf47ea9 100644
> >>> --- a/Makefile
> >>> +++ b/Makefile
> >>> @@ -56,7 +56,7 @@ Makefile: ;
> >>>  configure: ;
> >>>  
> >>>  .PHONY: all clean cscope distclean dvi html info install install-doc \
> >>> - pdf recurse-all speed test dist
> >>> + pdf recurse-all speed test dist modules
> >>>  
> >>>  $(call set-vpath, $(SRC_PATH))
> >>>  
> >>> @@ -121,7 +121,30 @@ ifeq ($(CONFIG_SMARTCARD_NSS),y)
> >>>  include $(SRC_PATH)/libcacard/Makefile
> >>>  endif
> >>>  
> >>> -all: $(DOCS) $(TOOLS) $(HELPERS-y) recurse-all
> >>> +all: $(DOCS) $(TOOLS) $(HELPERS-y) recurse-all modules
> >>> +
> >>> +mod-obj-m = $(patsubst %.o,%$(DSOSUF),$(filter %.o,$(common-obj-m))) \
> >>> +            $(patsubst %.mo,%$(DSOSUF),$(filter %.mo,$(common-obj-m)))
> >>
> >>
> >> Why common-obj-m only, what about block-obj-m?
> >>
> > Because common-obj-m = $(block-obj-m), in Makefile.objs below.
> > 
> >> Perhaps adding to mod-obj-m (better name: modules-m) can be done in
> >> unnest-vars?
> > 
> > It looks more straightforward for me to do here, and it's used very locally.
> 
> But it doesn't scale too well, does it?  There's no particular reason 
> why all -m variables would be included in common-obj-m.  In fact, block-
> obj-y/block-obj-m are the only remaining case of one variable including 
> another.  In another review I even proposed eliminating this exception.
> 
> >>> +# Generate rules for single file modules (%.so: %.o).
> >>> +$(foreach o,$(filter %.o,$(common-obj-m)),$(eval \
> >>> + $(patsubst %.o,%.so,$o): $o ))
> >>
> >> If you subst %.o to %.mo, you can use the same set of rules for both cases.
> >>
> >>> +# For multi file modules, dependencies should be listed explicitly in
> >>> +# Makefile.objs as
> >>> +#     $(obj)/foo.mo-obj := $(obj)/bar.o $(obj)/biz.o
> >>
> >> Why not
> >>
> >> $(obj)/foo.mo: $(obj)/bar.o $(obj)/biz.o
> >>
> >> ?
> >>
> > Because I don't know how to expand foo.mo to bar.o and biz.o: I expand 
> > "foo.mo"
> > to "bar.o biz.o" in the link command line, to skip creating to foo.mo, or
> > libfoo.a, if it's to be linked static:
> > 
> >     block-obj-y += foo.mo
> > 
> > That's for avoiding ar and ln -r.
> 
> Ah, I see now.  See below for an idea (if it works).
> 
> >>> +$(foreach o,$(filter %.mo,$(mod-obj-m)),$(eval \
> >>> + $o: $($o-obj)))
> >>> +
> >>> +%.mo:
> >>> + $(if $(BUILD_DYNAMIC), \
> >>> +         $(call quiet-command,$(CC) $(sort $^) -shared -o $@,"  LD[M] 
> >>> $(TARGET_DIR)$@"), \
> >>> +         $(call quiet-command,$(AR) rcs $@ $(sort $^),"  AR    
> >>> $(TARGET_DIR)$@"))
> >>
> >> I think we can always build modules dynamically.  If the
> >> module/no-module configure option decides whether an object moves
> >> between *-obj-y and *-obj-m, statically-linked modules will just go on
> >> the linker command line with no need for $(AR) or
> >> --whole-archive/--no-whole-archive.
> >>
> > I don't understand, do you mean a "cc -shared" output can be statically 
> > linked
> > into a executable?
> 
> So if I understand correctly the .mo file is never used for a 
> statically-linked module?  So the "ar" doesn't matter as things stand
> 
> Perhaps you could just use .mo file as a placeholder that holds the
> module's list of dependencies, no matter if it is shared or static:
> 
> %.mo:
>       $(call quiet-command,echo $(sort $^) > $@,"  GEN    $(TARGET_DIR_$@"))
> 
> In the LINK rule include the list from all .mo files:
> 
>       $(sort $(filter %.o, $1)) \
>       $(shell cat $(filter %.mo,$^)) \
>       $(filter-out %.o %.mo,$^))
> 
> And the rule for shared objects would be simply:
> 
> %.$(DSOSUF): QEMU_LDFLAGS += -shared
> %.$(DSOSUF): %.mo
>       $(call LINK,$^)
> 
> This would remove the need for the $i-obj variables.
> 
Actually I would like a try to keep $i-{obj,cflags,libs}:

    foo-obj := bar.o biz.o
    foo-libs := $(FOO_LIBS)
    foo-cflags := $(FOO_CFLAGS)

getting rid of all the $(obj)/'s and expand them in the toplevel unnest magic
(because we know foo.mo is contained in local $(block-obj-y) so it's all
trackable).

If this idea can't work out, I'll use your suggested way.

Thanks,

Fam