| Message ID | 20200512095546.25602-1-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] Always compile the kvm-unit-tests with -fno-common | expand |
On 12/05/2020 11.55, Thomas Huth wrote: > The new GCC v10 uses -fno-common by default. To avoid that we commit > code that declares global variables twice and thus fails to link with > the latest version, we should also compile with -fno-common when using > older versions of the compiler. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index 754ed65..3ff2f91 100644 > --- a/Makefile > +++ b/Makefile > @@ -49,7 +49,7 @@ include $(SRCDIR)/$(TEST_DIR)/Makefile > cc-option = $(shell if $(CC) -Werror $(1) -S -o /dev/null -xc /dev/null \ > > /dev/null 2>&1; then echo "$(1)"; else echo "$(2)"; fi ;) > > -COMMON_CFLAGS += -g $(autodepend-flags) -fno-strict-aliasing > +COMMON_CFLAGS += -g $(autodepend-flags) -fno-strict-aliasing -fno-common Oh, wait, this breaks the non-x86 builds due to "extern-less" struct auxinfo auxinfo in libauxinfo.h ! Drew, why isn't this declared in auxinfo.c instead? Thomas
On 13/05/2020 12.05, Thomas Huth wrote: > On 12/05/2020 11.55, Thomas Huth wrote: >> The new GCC v10 uses -fno-common by default. To avoid that we commit >> code that declares global variables twice and thus fails to link with >> the latest version, we should also compile with -fno-common when using >> older versions of the compiler. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> Makefile | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/Makefile b/Makefile >> index 754ed65..3ff2f91 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -49,7 +49,7 @@ include $(SRCDIR)/$(TEST_DIR)/Makefile >> cc-option = $(shell if $(CC) -Werror $(1) -S -o /dev/null -xc /dev/null \ >> > /dev/null 2>&1; then echo "$(1)"; else echo "$(2)"; fi ;) >> >> -COMMON_CFLAGS += -g $(autodepend-flags) -fno-strict-aliasing >> +COMMON_CFLAGS += -g $(autodepend-flags) -fno-strict-aliasing -fno-common > > Oh, wait, this breaks the non-x86 builds due to "extern-less" struct > auxinfo auxinfo in libauxinfo.h ! > Drew, why isn't this declared in auxinfo.c instead? Oh well, it's there ... so we're playing tricks with the linker here? I guess adding a "__attribute__((common, weak))" to auxinfo.h will be ok to fix this issue? Thomas
On Wed, May 13, 2020 at 12:11:39PM +0200, Thomas Huth wrote: > On 13/05/2020 12.05, Thomas Huth wrote: > > On 12/05/2020 11.55, Thomas Huth wrote: > >> The new GCC v10 uses -fno-common by default. To avoid that we commit > >> code that declares global variables twice and thus fails to link with > >> the latest version, we should also compile with -fno-common when using > >> older versions of the compiler. > >> > >> Signed-off-by: Thomas Huth <thuth@redhat.com> > >> --- > >> Makefile | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/Makefile b/Makefile > >> index 754ed65..3ff2f91 100644 > >> --- a/Makefile > >> +++ b/Makefile > >> @@ -49,7 +49,7 @@ include $(SRCDIR)/$(TEST_DIR)/Makefile > >> cc-option = $(shell if $(CC) -Werror $(1) -S -o /dev/null -xc /dev/null \ > >> > /dev/null 2>&1; then echo "$(1)"; else echo "$(2)"; fi ;) > >> > >> -COMMON_CFLAGS += -g $(autodepend-flags) -fno-strict-aliasing > >> +COMMON_CFLAGS += -g $(autodepend-flags) -fno-strict-aliasing -fno-common > > > > Oh, wait, this breaks the non-x86 builds due to "extern-less" struct > > auxinfo auxinfo in libauxinfo.h ! > > Drew, why isn't this declared in auxinfo.c instead? > > Oh well, it's there ... so we're playing tricks with the linker here? I > guess adding a "__attribute__((common, weak))" to auxinfo.h will be ok > to fix this issue? Right. In lib/auxinfo.h we have /* No extern! Define a common symbol. */ struct auxinfo auxinfo; Despite git-blame giving me credit for the 'No extern' comment (and the missing 'extern'), I think Paolo made those changes when applying the patch. I presume he did so to fix compilation on x86, for which I presume the problem was that lib/argv.c references auxinfo, and that resulted in an undefined symbol, since x86 doesn't link to auxinfo.o. Unfortunately making the symbol weak won't work because if we add it to the definition in auxinfo.h, then the linker prefers using its own zero-initialized, global symbol. And, if we add the attribute to the definition in auxinfo.c, then we still get the multiple definition error. So I'm not really sure what the best thing to do is. Maybe we should just do this diff --git a/lib/auxinfo.h b/lib/auxinfo.h index 08b96f8ece4c..a46a1e6f6a62 100644 --- a/lib/auxinfo.h +++ b/lib/auxinfo.h @@ -13,7 +13,6 @@ struct auxinfo { unsigned long flags; }; -/* No extern! Define a common symbol. */ -struct auxinfo auxinfo; +extern struct auxinfo auxinfo; #endif #endif diff --git a/x86/Makefile.common b/x86/Makefile.common index ab67ca0a9fda..2ea9c9f5bbcc 100644 --- a/x86/Makefile.common +++ b/x86/Makefile.common @@ -5,6 +5,7 @@ all: directories test_cases cflatobjs += lib/pci.o cflatobjs += lib/pci-edu.o cflatobjs += lib/alloc.o +cflatobjs += lib/auxinfo.o cflatobjs += lib/vmalloc.o cflatobjs += lib/alloc_page.o cflatobjs += lib/alloc_phys.o Thanks, drew
On 14/05/20 09:33, Andrew Jones wrote: > diff --git a/lib/auxinfo.h b/lib/auxinfo.h > index 08b96f8ece4c..a46a1e6f6a62 100644 > --- a/lib/auxinfo.h > +++ b/lib/auxinfo.h > @@ -13,7 +13,6 @@ struct auxinfo { > unsigned long flags; > }; > > -/* No extern! Define a common symbol. */ > -struct auxinfo auxinfo; > +extern struct auxinfo auxinfo; > #endif > #endif > diff --git a/x86/Makefile.common b/x86/Makefile.common > index ab67ca0a9fda..2ea9c9f5bbcc 100644 > --- a/x86/Makefile.common > +++ b/x86/Makefile.common > @@ -5,6 +5,7 @@ all: directories test_cases > cflatobjs += lib/pci.o > cflatobjs += lib/pci-edu.o > cflatobjs += lib/alloc.o > +cflatobjs += lib/auxinfo.o > cflatobjs += lib/vmalloc.o > cflatobjs += lib/alloc_page.o > cflatobjs += lib/alloc_phys.o > > > Thanks, > drew > Yes, this sounds good.
diff --git a/Makefile b/Makefile index 754ed65..3ff2f91 100644 --- a/Makefile +++ b/Makefile @@ -49,7 +49,7 @@ include $(SRCDIR)/$(TEST_DIR)/Makefile cc-option = $(shell if $(CC) -Werror $(1) -S -o /dev/null -xc /dev/null \ > /dev/null 2>&1; then echo "$(1)"; else echo "$(2)"; fi ;) -COMMON_CFLAGS += -g $(autodepend-flags) -fno-strict-aliasing +COMMON_CFLAGS += -g $(autodepend-flags) -fno-strict-aliasing -fno-common COMMON_CFLAGS += -Wall -Wwrite-strings -Wempty-body -Wuninitialized COMMON_CFLAGS += -Wignored-qualifiers -Werror
The new GCC v10 uses -fno-common by default. To avoid that we commit code that declares global variables twice and thus fails to link with the latest version, we should also compile with -fno-common when using older versions of the compiler. Signed-off-by: Thomas Huth <thuth@redhat.com> --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)