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

[Paolo Bonzini]

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.

Paolo