[Qemu-devel] [PATCH] rules.mak: Force CFLAGS for all objects in DSO

[Qemu-devel] [PATCH] rules.mak: Force CFLAGS for all objects in DSO

[Fam Zheng]

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)
              $(eval modules: $($v:%.mo=%$(DSOSUF))),
              $(eval $(patsubst %-m,%-y,$v) += $(call expand-objs, $($v)))))
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH] rules.mak: Force CFLAGS for all objects in DSO

[Paolo Bonzini]

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)))))
>  
> 

Re: [Qemu-devel] [PATCH] rules.mak: Force CFLAGS for all objects in DSO

[Fam Zheng]

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

Re: [Qemu-devel] [PATCH] rules.mak: Force CFLAGS for all objects in DSO

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] rules.mak: Force CFLAGS for all objects in DSO

[Fam Zheng]

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

Re: [Qemu-devel] [PATCH] rules.mak: Force CFLAGS for all objects in DSO

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] rules.mak: Force CFLAGS for all objects in DSO

[Alexander Graf]

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