| Message ID | 1430920000-31229-1-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/05/2015 15:46, Fam Zheng wrote: > Because of the trick of process-archive-undefs, all .mo objects, even > with --enable-modules, are dependencies of executables. > > This breaks CFLAGS propogation because the compiling of module object > will happen too early before building for DSO. > > With GCC 5, the linking would fail because .o doesn't have -fPIC. Also, > BUILD_DSO will be missed. (module-common.o will have it, so the stamp > symbol was still liked in .so). > > Fix the problem by forcing the CFLAGS during unnest-vars. > > Reported-by: Alexander Graf <agraf@suse.de> > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > rules.mak | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/rules.mak b/rules.mak > index 3a05627..6c0caf3 100644 > --- a/rules.mak > +++ b/rules.mak > @@ -102,7 +102,6 @@ endif > %.o: %.dtrace > $(call quiet-command,dtrace -o $@ -G -s $<, " GEN $(TARGET_DIR)$@") > > -%$(DSOSUF): CFLAGS += -fPIC -DBUILD_DSO > %$(DSOSUF): LDFLAGS += $(LDFLAGS_SHARED) > %$(DSOSUF): %.mo > $(call LINK,$^) > @@ -353,6 +352,7 @@ define unnest-vars > $(foreach o,$($v), > $(eval $o: $($o-objs))) > $(eval $(patsubst %-m,%-y,$v) += $($v)) > + $(eval $($v:%.mo=%$(DSOSUF)) $($v) $(foreach o,$($v),$($o-objs)) .PHONY: CFLAGS += -fPIC -DBUILD_DSO) Can you explain the various parts to the left of the colon? Thanks, Paolo > $(eval modules: $($v:%.mo=%$(DSOSUF))), > $(eval $(patsubst %-m,%-y,$v) += $(call expand-objs, $($v))))) > >
On Wed, 05/06 16:07, Paolo Bonzini wrote: > > > On 06/05/2015 15:46, Fam Zheng wrote: > > Because of the trick of process-archive-undefs, all .mo objects, even > > with --enable-modules, are dependencies of executables. > > > > This breaks CFLAGS propogation because the compiling of module object > > will happen too early before building for DSO. > > > > With GCC 5, the linking would fail because .o doesn't have -fPIC. Also, > > BUILD_DSO will be missed. (module-common.o will have it, so the stamp > > symbol was still liked in .so). > > > > Fix the problem by forcing the CFLAGS during unnest-vars. > > > > Reported-by: Alexander Graf <agraf@suse.de> > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > rules.mak | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/rules.mak b/rules.mak > > index 3a05627..6c0caf3 100644 > > --- a/rules.mak > > +++ b/rules.mak > > @@ -102,7 +102,6 @@ endif > > %.o: %.dtrace > > $(call quiet-command,dtrace -o $@ -G -s $<, " GEN $(TARGET_DIR)$@") > > > > -%$(DSOSUF): CFLAGS += -fPIC -DBUILD_DSO > > %$(DSOSUF): LDFLAGS += $(LDFLAGS_SHARED) > > %$(DSOSUF): %.mo > > $(call LINK,$^) > > @@ -353,6 +352,7 @@ define unnest-vars > > $(foreach o,$($v), > > $(eval $o: $($o-objs))) > > $(eval $(patsubst %-m,%-y,$v) += $($v)) > > + $(eval $($v:%.mo=%$(DSOSUF)) $($v) $(foreach o,$($v),$($o-objs)) .PHONY: CFLAGS += -fPIC -DBUILD_DSO) ^ ^ ^ ^ | | | | | | | `- In case all others are empty. | | | | | `- Expansion of all %.mo-objs so it's a %.o list. | | For $v=block-obj-m, this will contain curl.o, iscsi.o, ... | | | `- %.mo list | For $v=block-obj-m, this will contain curl.mo, iscsi.mo, ... | `- %.so list For $v=block-obj-m, this will contain curl.so, iscsi.so, ... > > Can you explain the various parts to the left of the colon? Context: - The line is inside "$(if $(CONFIG_MODULES),...)" - $v is one of "block-job-m common-obj-m ..." - So, $($v) is the module object list "foo.mo bar.mo ..." Do you want a respin to include the ascii art? :) Fam
On 06/05/2015 16:23, Fam Zheng wrote: >>> > > + $(eval $($v:%.mo=%$(DSOSUF)) $($v) $(foreach o,$($v),$($o-objs)) .PHONY: CFLAGS += -fPIC -DBUILD_DSO) > ^ ^ ^ ^ > | | | | > | | | `- In case all others are empty. > | | | > | | `- Expansion of all %.mo-objs so it's a %.o list. > | | For $v=block-obj-m, this will contain curl.o, iscsi.o, ... > | | > | `- %.mo list > | For $v=block-obj-m, this will contain curl.mo, iscsi.mo, ... > | > `- %.so list > For $v=block-obj-m, this will contain curl.so, iscsi.so, . Great. :) You should have used Unicode drawing characters to be consistent with rules.mak! But hopefully the ASCII art is not needed at all. Do we need all of those? - foo.mo is a prerequisite of foo.so. You cannot use foo.so (this is the bug you are fixing) but foo.so doesn't use CFLAGS in its rule, and you should be able to remove it. - can you use foo.mo without hitting the original bug? If so, could you simply use: -%$(DSOSUF): CFLAGS += -fPIC -DBUILD_DSO +%.mo: CFLAGS += -fPIC -DBUILD_DSO (and if so move the rule down, above "%.mo:")? If you cannot use foo.mo, foo.mo also doesn't use CFLAGS in its rules, and you should be able to remove foo.mo too. - so the other possibility is to just use $(foreach o,$($v),$($o-objs)). In this case there's already a convenient $(foreach) just above, and you can just add another $(eval) inside it. - and if that is true, you do not need .PHONY either because you have $(foreach ... $(eval)) instead of $(eval $(foreach)). Maybe I'm wrong, in which case the patch is ok. :) Paolo
On Wed, 05/06 16:36, Paolo Bonzini wrote: > > > On 06/05/2015 16:23, Fam Zheng wrote: > >>> > > + $(eval $($v:%.mo=%$(DSOSUF)) $($v) $(foreach o,$($v),$($o-objs)) .PHONY: CFLAGS += -fPIC -DBUILD_DSO) > > ^ ^ ^ ^ > > | | | | > > | | | `- In case all others are empty. > > | | | > > | | `- Expansion of all %.mo-objs so it's a %.o list. > > | | For $v=block-obj-m, this will contain curl.o, iscsi.o, ... > > | | > > | `- %.mo list > > | For $v=block-obj-m, this will contain curl.mo, iscsi.mo, ... > > | > > `- %.so list > > For $v=block-obj-m, this will contain curl.so, iscsi.so, . > > Great. :) You should have used Unicode drawing characters to be > consistent with rules.mak! Well, I'd have to find and read the vim plugin doc to do that again! > > But hopefully the ASCII art is not needed at all. Do we need all of those? > > - foo.mo is a prerequisite of foo.so. You cannot use foo.so (this is > the bug you are fixing) but foo.so doesn't use CFLAGS in its rule, and > you should be able to remove it. > > - can you use foo.mo without hitting the original bug? If so, could you > simply use: > > -%$(DSOSUF): CFLAGS += -fPIC -DBUILD_DSO > +%.mo: CFLAGS += -fPIC -DBUILD_DSO I believe this will propagate the flags correctly. However that will affect non-module build, so I didn't want to do it unconditionally. > > (and if so move the rule down, above "%.mo:")? If you cannot use > foo.mo, foo.mo also doesn't use CFLAGS in its rules, and you should be > able to remove foo.mo too. > > - so the other possibility is to just use $(foreach o,$($v),$($o-objs)). > In this case there's already a convenient $(foreach) just above, and > you can just add another $(eval) inside it. > > - and if that is true, you do not need .PHONY either because you have > $(foreach ... $(eval)) instead of $(eval $(foreach)). > OK. It's been a long day for me, so you can see that I'm going defensive with the code I'm writing, but yes, your last "if" looks about right, except we still need the "$(if $(CONFIG_MODULES),...)". Sanity check: is -fPIC only ever needed in compiling, but not in (partial) linking, right? Fam
On 06/05/2015 17:01, Fam Zheng wrote: >> > -%$(DSOSUF): CFLAGS += -fPIC -DBUILD_DSO >> > +%.mo: CFLAGS += -fPIC -DBUILD_DSO > I believe this will propagate the flags correctly. However that will affect > non-module build, so I didn't want to do it unconditionally. You're right. You'd need something like $(call lif $(CONFIG_MODULES), -fPIC -DBUILD_DSO) instead. >> > (and if so move the rule down, above "%.mo:")? If you cannot use >> > foo.mo, foo.mo also doesn't use CFLAGS in its rules, and you should be >> > able to remove foo.mo too. >> > >> > - so the other possibility is to just use $(foreach o,$($v),$($o-objs)). >> > In this case there's already a convenient $(foreach) just above, and >> > you can just add another $(eval) inside it. >> > >> > - and if that is true, you do not need .PHONY either because you have >> > $(foreach ... $(eval)) instead of $(eval $(foreach)). >> > > OK. > > It's been a long day for me, so you can see that I'm going defensive with the > code I'm writing, but yes, your last "if" looks about right, except we still > need the "$(if $(CONFIG_MODULES),...)". Understood entirely. But there's no hurry, sleep over it and take your time to test it tomorrow. > Sanity check: is -fPIC only ever needed in compiling, but not in (partial) > linking, right? Yes. It's LDFLAGS_SHARED that is used in linking (and nothing in partial linking). Paolo
On 06.05.15 15:46, Fam Zheng wrote: > Because of the trick of process-archive-undefs, all .mo objects, even > with --enable-modules, are dependencies of executables. > > This breaks CFLAGS propogation because the compiling of module object > will happen too early before building for DSO. > > With GCC 5, the linking would fail because .o doesn't have -fPIC. Also, > BUILD_DSO will be missed. (module-common.o will have it, so the stamp > symbol was still liked in .so). > > Fix the problem by forcing the CFLAGS during unnest-vars. > > Reported-by: Alexander Graf <agraf@suse.de> > Signed-off-by: Fam Zheng <famz@redhat.com> As a heads-up I just verified that this patch does indeed fix compilation with gcc5 for me. However looking at the mail thread I assume there's a v2 coming, so I'll hold off my tested-by tag ;). For the final patch, we will probably also want to have it in the qemu-stable tree, so that people will be able to compile older versions of qemu (and modules) with newer compilers. Alex
diff --git a/rules.mak b/rules.mak index 3a05627..6c0caf3 100644 --- a/rules.mak +++ b/rules.mak @@ -102,7 +102,6 @@ endif %.o: %.dtrace $(call quiet-command,dtrace -o $@ -G -s $<, " GEN $(TARGET_DIR)$@") -%$(DSOSUF): CFLAGS += -fPIC -DBUILD_DSO %$(DSOSUF): LDFLAGS += $(LDFLAGS_SHARED) %$(DSOSUF): %.mo $(call LINK,$^) @@ -353,6 +352,7 @@ define unnest-vars $(foreach o,$($v), $(eval $o: $($o-objs))) $(eval $(patsubst %-m,%-y,$v) += $($v)) + $(eval $($v:%.mo=%$(DSOSUF)) $($v) $(foreach o,$($v),$($o-objs)) .PHONY: CFLAGS += -fPIC -DBUILD_DSO) $(eval modules: $($v:%.mo=%$(DSOSUF))), $(eval $(patsubst %-m,%-y,$v) += $(call expand-objs, $($v)))))
Because of the trick of process-archive-undefs, all .mo objects, even with --enable-modules, are dependencies of executables. This breaks CFLAGS propogation because the compiling of module object will happen too early before building for DSO. With GCC 5, the linking would fail because .o doesn't have -fPIC. Also, BUILD_DSO will be missed. (module-common.o will have it, so the stamp symbol was still liked in .so). Fix the problem by forcing the CFLAGS during unnest-vars. Reported-by: Alexander Graf <agraf@suse.de> Signed-off-by: Fam Zheng <famz@redhat.com> --- rules.mak | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)