| Message ID | 1409567710-26217-1-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 01/09/2014 12:35, Fam Zheng ha scritto: > This fixes an issue with module build system. block/iscsi.so is > currently broken: > > $ ~/build/last/qemu-img > Failed to open module: /home/fam/build/master/block-iscsi.so: > undefined symbol: qmp_query_uuid > qemu-img: Not enough arguments > Try 'qemu-img --help' for more information > > To fix this, we should (at least) let qemu-img link qmp_query_uuid from > libqemustub.a. (There are a few other symbols missing, as well.) > > This patch changes the linking rules to: > > 1) Build ".mo" with "ld -r -o $@ $^" for each ".so", and later build .so > with it. > > 2) Always build all the .mo before linking the executables. This is > achieved by adding those .mo files to the executables' "-y" > variables. > > 3) When linking an executable, those .mo files in its "-y" variables are > filtered out, and replaced by one or more -Wl,-u,$symbol flags. This > is done in the added macro "process-archive-undefs". > > These "-Wl,-u,$symbol" flags will force ld to pull in the function > definition from the archives when linking. > > Note that the .mo objects, that are actually meant to be linked in > the executables, are already expanded in unnest-vars, before the > linking command. So we are safe to simply filter out .mo for the > purpose of pulling undefined symbols. > > process-archive-undefs works as this: For each ".mo", find all the > undefined symbols in it, filter ones that are defined in the > archives. For each of these symbols, generate a "-Wl,-u,$symbol" in > the link command, and put them before archive names in the command > line. > > Suggested-by: H.J. Lu <hjl.tools@gmail.com> > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > rules.mak | 43 ++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 40 insertions(+), 3 deletions(-) > > diff --git a/rules.mak b/rules.mak > index ba2f4c1..1d73293 100644 > --- a/rules.mak > +++ b/rules.mak > @@ -22,6 +22,32 @@ QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(*D)/$(*F).d > # Same as -I$(SRC_PATH) -I., but for the nested source/object directories > QEMU_INCLUDES += -I$(<D) -I$(@D) > > +WL_U := -Wl,-u, > +find-symbols = $(if $1, $(sort $(shell nm -P -g $1 | $2))) > +defined-symbols = $(call find-symbols,$1,awk '$$2!="U"{print $$1}') > +undefined-symbols = $(call find-symbols,$1,awk '$$2=="U"{print $$1}') > + > +# All the .mo objects in -m variables are also added into corresponding -y > +# variable in unnest-vars, but filtered out here, when LINK is called. > +# > +# The .mo objects are supposed to be linked as a DSO, for module build. So here > +# they are only used as a placeholders to generate those "archive undefined" > +# symbol options (-Wl,-u,$symbol_name), which are the archive functions > +# referenced by the code in the DSO. > +# > +# Also the presence in -y variables will also guarantee they are built before > +# linking executables that will load them. So we can look up symbol reference > +# in LINK. > +# > +# This is necessary because the exectuable itself may not use the function, in > +# which case the function would not be linked in. Then the DSO loading will > +# fail because of the missing symbol. > +process-archive-undefs = $(filter-out %.a %.mo,$1) \ > + $(addprefix $(WL_U), \ > + $(filter $(call defined-symbols,$(filter %.a, $1)), \ > + $(call undefined-symbols,$(filter %.mo,$1)))) \ > + $(filter %.a,$1) > + > extract-libs = $(strip $(foreach o,$1,$($o-libs))) > expand-objs = $(strip $(sort $(filter %.o,$1)) \ > $(foreach o,$(filter %.mo,$1),$($o-objs)) \ > @@ -38,7 +64,8 @@ LINKPROG = $(or $(CXX),$(CC)) > > ifeq ($(LIBTOOL),) > LINK = $(call quiet-command, $(LINKPROG) $(QEMU_CFLAGS) $(CFLAGS) $(LDFLAGS) -o $@ \ > - $1 $(version-obj-y) $(call extract-libs,$1) $(LIBS)," LINK $(TARGET_DIR)$@") > + $(call process-archive-undefs, $1) \ > + $(version-obj-y) $(call extract-libs,$1) $(LIBS)," LINK $(TARGET_DIR)$@") > else > LIBTOOL += $(if $(V),,--quiet) > %.lo: %.c > @@ -50,7 +77,8 @@ LIBTOOL += $(if $(V),,--quiet) > > LINK = $(call quiet-command,\ > $(if $(filter %.lo %.la,$1),$(LIBTOOL) --mode=link --tag=CC \ > - )$(LINKPROG) $(QEMU_CFLAGS) $(CFLAGS) $(LDFLAGS) -o $@ $1 \ > + )$(LINKPROG) $(QEMU_CFLAGS) $(CFLAGS) $(LDFLAGS) -o $@ \ > + $(call process-archive-undefs, $1)\ > $(if $(filter %.lo %.la,$1),$(version-lobj-y),$(version-obj-y)) \ > $(if $(filter %.lo %.la,$1),$(LIBTOOLFLAGS)) \ > $(call extract-libs,$(1:.lo=.o)) $(LIBS),$(if $(filter %.lo %.la,$1),"lt LINK ", " LINK ")"$(TARGET_DIR)$@") > @@ -76,11 +104,17 @@ endif > > %$(DSOSUF): CFLAGS += -fPIC -DBUILD_DSO > %$(DSOSUF): LDFLAGS += $(LDFLAGS_SHARED) > -%$(DSOSUF): > +%$(DSOSUF): %.mo > $(call LINK,$^) > @# Copy to build root so modules can be loaded when program started without install > $(if $(findstring /,$@),$(call quiet-command,cp $@ $(subst /,-,$@), " CP $(subst /,-,$@)")) > > + > +LD_REL := $(CC) -nostdlib -Wl,-r > + > +%.mo: > + $(call quiet-command,$(LD_REL) -o $@ $^," LD -r $(TARGET_DIR)$@") > + > .PHONY: modules > modules: > > @@ -306,6 +340,9 @@ define unnest-vars > # For module build, build shared libraries during "make modules" > # For non-module build, add -m to -y > $(if $(CONFIG_MODULES), > + $(foreach o,$($v), > + $(eval $o: $($o-objs))) > + $(eval $(patsubst %-m,%-y,$v) += $($v)) > $(eval modules: $($v:%.mo=%$(DSOSUF))), > $(eval $(patsubst %-m,%-y,$v) += $(call expand-objs, $($v))))) > > Thanks, Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
On 1 September 2014 11:35, Fam Zheng <famz@redhat.com> wrote: > This fixes an issue with module build system. block/iscsi.so is > currently broken: > > $ ~/build/last/qemu-img > Failed to open module: /home/fam/build/master/block-iscsi.so: > undefined symbol: qmp_query_uuid > qemu-img: Not enough arguments > Try 'qemu-img --help' for more information > > To fix this, we should (at least) let qemu-img link qmp_query_uuid from > libqemustub.a. (There are a few other symbols missing, as well.) > > This patch changes the linking rules to: > > 1) Build ".mo" with "ld -r -o $@ $^" for each ".so", and later build .so > with it. > > 2) Always build all the .mo before linking the executables. This is > achieved by adding those .mo files to the executables' "-y" > variables. > > 3) When linking an executable, those .mo files in its "-y" variables are > filtered out, and replaced by one or more -Wl,-u,$symbol flags. This > is done in the added macro "process-archive-undefs". > > These "-Wl,-u,$symbol" flags will force ld to pull in the function > definition from the archives when linking. > > Note that the .mo objects, that are actually meant to be linked in > the executables, are already expanded in unnest-vars, before the > linking command. So we are safe to simply filter out .mo for the > purpose of pulling undefined symbols. > > process-archive-undefs works as this: For each ".mo", find all the > undefined symbols in it, filter ones that are defined in the > archives. For each of these symbols, generate a "-Wl,-u,$symbol" in > the link command, and put them before archive names in the command > line. > > Suggested-by: H.J. Lu <hjl.tools@gmail.com> > Signed-off-by: Fam Zheng <famz@redhat.com> This patch causes the build on MacOSX to spit out a bunch of warnings every time we do a link: warning: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/nm: no name list warning: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/nm: no name list warning: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/nm: no name list LINK qemu-nbd > +find-symbols = $(if $1, $(sort $(shell nm -P -g $1 | $2))) > +defined-symbols = $(call find-symbols,$1,awk '$$2!="U"{print $$1}') > +undefined-symbols = $(call find-symbols,$1,awk '$$2=="U"{print $$1}') Are you sure your nm invocations are portable? thanks -- PMM
Il 01/09/2014 12:59, Peter Maydell ha scritto: > > warning: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/nm: > no name list > warning: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/nm: > no name list > warning: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/nm: > no name list > LINK qemu-nbd Is this with --enable-modules or without? Can you find out which invocations are printing the error? It could be one of "nm -P -g block/FILENAME.mo" or "nm -P -g libqemuutil.a" or "nm -P -g libqemustub.a". I found the error in http://www.opensource.apple.com/source/cctools/cctools-855/misc/nm.c if(st == NULL || st->nsyms == 0){ warning("no name list"); return; } I guess this could match the first part of this paragraph in the POSIX spec: "If no symbolic information is available for a valid input file, the nm utility shall report that fact, but not consider it an error condition". I guess we could use "2>&1" to shut it up, but it would be nice to know what the problem is. >> > +find-symbols = $(if $1, $(sort $(shell nm -P -g $1 | $2))) >> > +defined-symbols = $(call find-symbols,$1,awk '$$2!="U"{print $$1}') >> > +undefined-symbols = $(call find-symbols,$1,awk '$$2=="U"{print $$1}') > Are you sure your nm invocations are portable? Yes, http://pubs.opengroup.org/onlinepubs/9699919799/utilities/nm.html nm [-APv] [-g|-u] [-t format] file... -g Write only external (global) symbol information -P Write information in a portable output format, as specified in the STDOUT section. Paolo
On 1 September 2014 12:28, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 01/09/2014 12:59, Peter Maydell ha scritto: >> >> warning: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/nm: >> no name list >> warning: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/nm: >> no name list >> warning: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/nm: >> no name list >> LINK qemu-nbd > > Is this with --enable-modules or without? Both with and without. > Can you find out which invocations are printing the error? It could be > one of "nm -P -g block/FILENAME.mo" or "nm -P -g libqemuutil.a" or "nm > -P -g libqemustub.a". It produces the warnings on libqemuutil.a but not the others. > I found the error in > http://www.opensource.apple.com/source/cctools/cctools-855/misc/nm.c > > if(st == NULL || st->nsyms == 0){ > warning("no name list"); > return; > } > > I guess this could match the first part of this paragraph in the POSIX > spec: "If no symbolic information is available for a valid input file, > the nm utility shall report that fact, but not consider it an error > condition". I guess we could use "2>&1" to shut it up, but it would be > nice to know what the problem is. I narrowed down the warning to some of the individual .o files that end up in the .a: manooth$ nm util/host-utils.o warning: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/nm: no name list manooth$ nm util/getauxval.o warning: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/nm: no name list manooth$ nm trace/generated-tracers.o warning: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/nm: no name list I think what it doesn't like is when we have a .c file that compiles into an effectively empty object file because all the functions etc in it have been #ifdefed out, as happens with host-utils.c when CONFIG_INT128 is not defined, and with getauxval.c when CONFIG_GETAUXVAL is not defined, and with generated-tracers.c when there aren't any defined tracepoints. ranlib doesn't like that either (this one's a warning we've had for a long time): AR libqemuutil.a /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: libqemuutil.a(generated-tracers.o) has no symbols /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: libqemuutil.a(host-utils.o) has no symbols /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: libqemuutil.a(getauxval.o) has no symbols So we could fix this by not compiling empty files... thanks -- PMM
Il 01/09/2014 13:46, Peter Maydell ha scritto: > So we could fix this by not compiling empty files... Easy for int128, even easier for getauxval (it's not a fastpath, so we can move the "always return 0" version from include/qemu/osdep.h to util/getauxval.c). I'm not sure how to detect that there generated-tracers.c is empty. Stefan, is it used by anything except $(CONFIG_TRACE_SIMPLE)? > ranlib doesn't like that either (this one's a warning we've had for > a long time): Given this, do you consider th a blocker for this patch? Paolo
On 1 September 2014 13:04, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 01/09/2014 13:46, Peter Maydell ha scritto: >> So we could fix this by not compiling empty files... > > Easy for int128, even easier for getauxval (it's not a fastpath, so we > can move the "always return 0" version from include/qemu/osdep.h to > util/getauxval.c). > > I'm not sure how to detect that there generated-tracers.c is empty. > Stefan, is it used by anything except $(CONFIG_TRACE_SIMPLE)? You could always put in a dummy function that isn't actually used by anything. >> ranlib doesn't like that either (this one's a warning we've had for >> a long time): > > Given this, do you consider th a blocker for this patch? The ranlib warnings are comparatively clear and only happen when we link the .a file; the warnings we get out of nm are rather uglier and more confusing and produced at every final binary link. I'd certainly prefer it if we could fix them, but I guess I won't absolutely insist on it. -- PMM
On Mon, 09/01 13:07, Peter Maydell wrote: > On 1 September 2014 13:04, Paolo Bonzini <pbonzini@redhat.com> wrote: > > Il 01/09/2014 13:46, Peter Maydell ha scritto: > >> So we could fix this by not compiling empty files... > > > > Easy for int128, even easier for getauxval (it's not a fastpath, so we > > can move the "always return 0" version from include/qemu/osdep.h to > > util/getauxval.c). > > > > I'm not sure how to detect that there generated-tracers.c is empty. > > Stefan, is it used by anything except $(CONFIG_TRACE_SIMPLE)? > > You could always put in a dummy function that isn't actually > used by anything. > > >> ranlib doesn't like that either (this one's a warning we've had for > >> a long time): > > > > Given this, do you consider th a blocker for this patch? > > The ranlib warnings are comparatively clear and only happen > when we link the .a file; the warnings we get out of nm are > rather uglier and more confusing and produced at every final > binary link. I'd certainly prefer it if we could fix them, but I > guess I won't absolutely insist on it. > Thanks both of you for tracking down the problem. Let's add 2>/dev/null to suppress the warning. Fam
Il 02/09/2014 03:19, Fam Zheng ha scritto: > Thanks both of you for tracking down the problem. Let's add 2>/dev/null to > suppress the warning. Actually I think the warning would complicate tracking down real problems. We can fix the source of the warnings, especially since they already occur with ranlib. I've queued v2 of the patch, but I'll wait a bit before submitting it for inclusion. Paolo
On Tue, 09/02 14:07, Paolo Bonzini wrote: > Il 02/09/2014 03:19, Fam Zheng ha scritto: > > Thanks both of you for tracking down the problem. Let's add 2>/dev/null to > > suppress the warning. > > Actually I think the warning would complicate tracking down real > problems. We can fix the source of the warnings, especially since they > already occur with ranlib. I've queued v2 of the patch, but I'll wait a > bit before submitting it for inclusion. > OK, thanks. I'll post some patches to exclude the empty objects in the util-obj-y. Fam
diff --git a/rules.mak b/rules.mak index ba2f4c1..1d73293 100644 --- a/rules.mak +++ b/rules.mak @@ -22,6 +22,32 @@ QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(*D)/$(*F).d # Same as -I$(SRC_PATH) -I., but for the nested source/object directories QEMU_INCLUDES += -I$(<D) -I$(@D) +WL_U := -Wl,-u, +find-symbols = $(if $1, $(sort $(shell nm -P -g $1 | $2))) +defined-symbols = $(call find-symbols,$1,awk '$$2!="U"{print $$1}') +undefined-symbols = $(call find-symbols,$1,awk '$$2=="U"{print $$1}') + +# All the .mo objects in -m variables are also added into corresponding -y +# variable in unnest-vars, but filtered out here, when LINK is called. +# +# The .mo objects are supposed to be linked as a DSO, for module build. So here +# they are only used as a placeholders to generate those "archive undefined" +# symbol options (-Wl,-u,$symbol_name), which are the archive functions +# referenced by the code in the DSO. +# +# Also the presence in -y variables will also guarantee they are built before +# linking executables that will load them. So we can look up symbol reference +# in LINK. +# +# This is necessary because the exectuable itself may not use the function, in +# which case the function would not be linked in. Then the DSO loading will +# fail because of the missing symbol. +process-archive-undefs = $(filter-out %.a %.mo,$1) \ + $(addprefix $(WL_U), \ + $(filter $(call defined-symbols,$(filter %.a, $1)), \ + $(call undefined-symbols,$(filter %.mo,$1)))) \ + $(filter %.a,$1) + extract-libs = $(strip $(foreach o,$1,$($o-libs))) expand-objs = $(strip $(sort $(filter %.o,$1)) \ $(foreach o,$(filter %.mo,$1),$($o-objs)) \ @@ -38,7 +64,8 @@ LINKPROG = $(or $(CXX),$(CC)) ifeq ($(LIBTOOL),) LINK = $(call quiet-command, $(LINKPROG) $(QEMU_CFLAGS) $(CFLAGS) $(LDFLAGS) -o $@ \ - $1 $(version-obj-y) $(call extract-libs,$1) $(LIBS)," LINK $(TARGET_DIR)$@") + $(call process-archive-undefs, $1) \ + $(version-obj-y) $(call extract-libs,$1) $(LIBS)," LINK $(TARGET_DIR)$@") else LIBTOOL += $(if $(V),,--quiet) %.lo: %.c @@ -50,7 +77,8 @@ LIBTOOL += $(if $(V),,--quiet) LINK = $(call quiet-command,\ $(if $(filter %.lo %.la,$1),$(LIBTOOL) --mode=link --tag=CC \ - )$(LINKPROG) $(QEMU_CFLAGS) $(CFLAGS) $(LDFLAGS) -o $@ $1 \ + )$(LINKPROG) $(QEMU_CFLAGS) $(CFLAGS) $(LDFLAGS) -o $@ \ + $(call process-archive-undefs, $1)\ $(if $(filter %.lo %.la,$1),$(version-lobj-y),$(version-obj-y)) \ $(if $(filter %.lo %.la,$1),$(LIBTOOLFLAGS)) \ $(call extract-libs,$(1:.lo=.o)) $(LIBS),$(if $(filter %.lo %.la,$1),"lt LINK ", " LINK ")"$(TARGET_DIR)$@") @@ -76,11 +104,17 @@ endif %$(DSOSUF): CFLAGS += -fPIC -DBUILD_DSO %$(DSOSUF): LDFLAGS += $(LDFLAGS_SHARED) -%$(DSOSUF): +%$(DSOSUF): %.mo $(call LINK,$^) @# Copy to build root so modules can be loaded when program started without install $(if $(findstring /,$@),$(call quiet-command,cp $@ $(subst /,-,$@), " CP $(subst /,-,$@)")) + +LD_REL := $(CC) -nostdlib -Wl,-r + +%.mo: + $(call quiet-command,$(LD_REL) -o $@ $^," LD -r $(TARGET_DIR)$@") + .PHONY: modules modules: @@ -306,6 +340,9 @@ define unnest-vars # For module build, build shared libraries during "make modules" # For non-module build, add -m to -y $(if $(CONFIG_MODULES), + $(foreach o,$($v), + $(eval $o: $($o-objs))) + $(eval $(patsubst %-m,%-y,$v) += $($v)) $(eval modules: $($v:%.mo=%$(DSOSUF))), $(eval $(patsubst %-m,%-y,$v) += $(call expand-objs, $($v)))))
This fixes an issue with module build system. block/iscsi.so is currently broken: $ ~/build/last/qemu-img Failed to open module: /home/fam/build/master/block-iscsi.so: undefined symbol: qmp_query_uuid qemu-img: Not enough arguments Try 'qemu-img --help' for more information To fix this, we should (at least) let qemu-img link qmp_query_uuid from libqemustub.a. (There are a few other symbols missing, as well.) This patch changes the linking rules to: 1) Build ".mo" with "ld -r -o $@ $^" for each ".so", and later build .so with it. 2) Always build all the .mo before linking the executables. This is achieved by adding those .mo files to the executables' "-y" variables. 3) When linking an executable, those .mo files in its "-y" variables are filtered out, and replaced by one or more -Wl,-u,$symbol flags. This is done in the added macro "process-archive-undefs". These "-Wl,-u,$symbol" flags will force ld to pull in the function definition from the archives when linking. Note that the .mo objects, that are actually meant to be linked in the executables, are already expanded in unnest-vars, before the linking command. So we are safe to simply filter out .mo for the purpose of pulling undefined symbols. process-archive-undefs works as this: For each ".mo", find all the undefined symbols in it, filter ones that are defined in the archives. For each of these symbols, generate a "-Wl,-u,$symbol" in the link command, and put them before archive names in the command line. Suggested-by: H.J. Lu <hjl.tools@gmail.com> Signed-off-by: Fam Zheng <famz@redhat.com> --- rules.mak | 43 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-)