[kvm-unit-tests PATCH] Always compile the kvm-unit-tests with -fno-common

[kvm-unit-tests PATCH] Always compile the kvm-unit-tests with -fno-common

[Thomas Huth]

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
 COMMON_CFLAGS += -Wall -Wwrite-strings -Wempty-body -Wuninitialized
 COMMON_CFLAGS += -Wignored-qualifiers -Werror
 
-- 
2.18.1

Re: [kvm-unit-tests PATCH] Always compile the kvm-unit-tests with -fno-common

[Thomas Huth]

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

Re: [kvm-unit-tests PATCH] Always compile the kvm-unit-tests with -fno-common

[Thomas Huth]

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

Re: [kvm-unit-tests PATCH] Always compile the kvm-unit-tests with -fno-common

[Andrew Jones]

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

Re: [kvm-unit-tests PATCH] Always compile the kvm-unit-tests with -fno-common

[Paolo Bonzini]

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.