Patches for clang compilation
Subsystems (3)
cover Patches for clang compilation
These three patches fixes some compilation issues that clang's encountering. I ran the tests and there are no regressions with gcc.
Bill Wendling (3):
x86: emulator: use "SSE2" for the target
pci: use uint64_t for unsigned long values
Makefile: use "-Werror" in cc-option
Makefile | 19 +++++++++++-------- lib/pci.c | 18 +++++++++--------- lib/pci.h | 4 ++-- x86/emulator.c | 2 +- 4 files changed, 23 insertions(+), 20 deletions(-)
Signature · 1 line
2.23.0.700.g56cf767bdb-goog
PATCH 1/3 x86: emulator: use "SSE2" for the target
The movdqu and movapd instructions are SSE2 instructions. Clang interprets the __attribute__((target("sse"))) as allowing SSE only instructions. Using SSE2 instructions cause an error.
--- x86/emulator.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
x86/emulator.c +1 -1
@@ -657,7 +657,7 @@ static bool sseeq(sse_union *v1, sse_union *v2)return ok; } -static __attribute__((target ("sse" )))void test_sse (sse_union *mem) +static __attribute__((target ("sse2" )))void test_sse (sse_union *mem) {sse_union v;
Signature · 1 line
2.23.0.700.g56cf767bdb-goog
PATCH 2/3 pci: use uint64_t for unsigned long values
In reply to Bill WendlingThe "pci_bar_*" functions work with unsigned long values, but were using uint32_t for the data types. Clang complains about this. So we bump up the type to uint64_t.
lib/pci.c:110:3: error: implicit conversion from 'unsigned long' to 'uint32_t' (aka 'unsigned int') changes value from 18446744073709551612 to 4294967292 [-Werror,-Wconstant-conversion]
PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
^~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/google/home/morbo/kvm-unit-tests/lib/linux/pci_regs.h:100:36: note: expanded from macro 'PCI_BASE_ADDRESS_IO_MASK'
#define PCI_BASE_ADDRESS_IO_MASK (~0x03UL)
^~~~~~~
lib/pci.c:110:30: error: implicit conversion from 'unsigned long' to 'uint32_t' (aka 'unsigned int') changes value from 18446744073709551600 to 4294967280 [-Werror,-Wconstant-conversion]
PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
^~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/google/home/morbo/kvm-unit-tests/lib/linux/pci_regs.h:99:37: note: expanded from macro 'PCI_BASE_ADDRESS_MEM_MASK'
#define PCI_BASE_ADDRESS_MEM_MASK (~0x0fUL)
^~~~~~~--- lib/pci.c | 18 +++++++++--------- lib/pci.h | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-)
lib/pci.c +9 -9
@@ -104,13 +104,13 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)return PCIDEVADDR_INVALID ; } -uint32_t pci_bar_mask (uint32_t bar) +uint64_t pci_bar_mask (uint32_t bar) {return (bar &PCI_BASE_ADDRESS_SPACE_IO ) ?PCI_BASE_ADDRESS_IO_MASK :PCI_BASE_ADDRESS_MEM_MASK ; } -uint32_t pci_bar_get (struct pci_dev *dev,int bar_num) +uint64_t pci_bar_get (struct pci_dev *dev,int bar_num) {ASSERT_BAR_NUM (bar_num);
@@ -120,13 +120,13 @@ uint32_t pci_bar_get(struct pci_dev *dev, int bar_num)static phys_addr_t __pci_bar_get_addr (struct pci_dev *dev,int bar_num) { -uint32_t bar =pci_bar_get (dev, bar_num); -uint32_t mask =pci_bar_mask (bar); +uint64_t bar =pci_bar_get (dev, bar_num); +uint64_t mask =pci_bar_mask (bar);uint64_t addr = bar & mask;phys_addr_t phys_addr;if (pci_bar_is64 (dev, bar_num)) - addr |= (uint64_t )pci_bar_get (dev, bar_num +1 ) <<32 ; + addr |=pci_bar_get (dev, bar_num +1 ) <<32 ; phys_addr =pci_translate_addr (dev->bdf, addr);assert (phys_addr !=INVALID_PHYS_ADDR );
@@ -189,7 +189,7 @@ static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num)phys_addr_t pci_bar_size (struct pci_dev *dev,int bar_num) { -uint32_t bar, size; +uint64_t bar, size; size =pci_bar_size_helper (dev, bar_num);if (!size)
@@ -210,7 +210,7 @@ phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num)bool pci_bar_is_memory (struct pci_dev *dev,int bar_num) { -uint32_t bar =pci_bar_get (dev, bar_num); +uint64_t bar =pci_bar_get (dev, bar_num);return !(bar &PCI_BASE_ADDRESS_SPACE_IO ); }
@@ -222,7 +222,7 @@ bool pci_bar_is_valid(struct pci_dev *dev, int bar_num)bool pci_bar_is64 (struct pci_dev *dev,int bar_num) { -uint32_t bar =pci_bar_get (dev, bar_num); +uint64_t bar =pci_bar_get (dev, bar_num);if (bar &PCI_BASE_ADDRESS_SPACE_IO )return false;
@@ -234,7 +234,7 @@ bool pci_bar_is64(struct pci_dev *dev, int bar_num)void pci_bar_print (struct pci_dev *dev,int bar_num) {phys_addr_t size, start, end; -uint32_t bar; +uint64_t bar;if (!pci_bar_is_valid (dev, bar_num))return ;
lib/pci.h +2 -2
@@ -60,8 +60,8 @@ extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);extern phys_addr_t pci_bar_get_addr (struct pci_dev *dev,int bar_num);extern void pci_bar_set_addr (struct pci_dev *dev,int bar_num,phys_addr_t addr);extern phys_addr_t pci_bar_size (struct pci_dev *dev,int bar_num); -extern uint32_t pci_bar_get (struct pci_dev *dev,int bar_num); -extern uint32_t pci_bar_mask (uint32_t bar); +extern uint64_t pci_bar_get (struct pci_dev *dev,int bar_num); +extern uint64_t pci_bar_mask (uint32_t bar);extern bool pci_bar_is64 (struct pci_dev *dev,int bar_num);extern bool pci_bar_is_memory (struct pci_dev *dev,int bar_num);extern bool pci_bar_is_valid (struct pci_dev *dev,int bar_num);
Signature · 1 line
2.23.0.700.g56cf767bdb-goog
Re: [kvm-unit-tests PATCH 2/3] pci: use uint64_t for unsigned long values
Hi,
Quoted 3 lines · Bill Wendling
The "pci_bar_*" functions work with unsigned long values, but were using
uint32_tfor the data types. Clang complains about this. So we bump up the type touint64_t.
Your patch might fix the warning, but the bar functions were returning 32 bits because the bars are 32 bits and because they are being read using functions that return 32 bit values.
Quoted 12 lines
lib/pci.c:110:3: error: implicit conversion from 'unsigned long' to 'uint32_t' (aka 'unsigned int') changes value from 18446744073709551612 to 4294967292 [-Werror,-Wconstant-conversion]
PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
^~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/google/home/morbo/kvm-unit-tests/lib/linux/pci_regs.h:100:36: note: expanded from macro 'PCI_BASE_ADDRESS_IO_MASK'
#define PCI_BASE_ADDRESS_IO_MASK (~0x03UL)
^~~~~~~
lib/pci.c:110:30: error: implicit conversion from 'unsigned long' to 'uint32_t' (aka 'unsigned int') changes value from 18446744073709551600 to 4294967280 [-Werror,-Wconstant-conversion]
PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
^~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/google/home/morbo/kvm-unit-tests/lib/linux/pci_regs.h:99:37: note: expanded from macro 'PCI_BASE_ADDRESS_MEM_MASK'
#define PCI_BASE_ADDRESS_MEM_MASK (~0x0fUL)I think the issue is that the mask should be 32 bits, but unsigned long is 64 bits on your architecture. I think it's safe to use ~0x0fU here, because we're only interested in the least significant 4 bits.
Regards,
Alex
Quoted 96 lines
^~~~~~~Signed-off-by Bill Wendling <[email protected]>--- lib/pci.c | 18 +++++++++--------- lib/pci.h | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-)lib/pci.c +9
-9@@ -104,13 +104,13 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)return PCIDEVADDR_INVALID ; } -uint32_t pci_bar_mask (uint32_t bar) +uint64_t pci_bar_mask (uint32_t bar) {return (bar &PCI_BASE_ADDRESS_SPACE_IO ) ?PCI_BASE_ADDRESS_IO_MASK :PCI_BASE_ADDRESS_MEM_MASK ; } -uint32_t pci_bar_get (struct pci_dev *dev,int bar_num) +uint64_t pci_bar_get (struct pci_dev *dev,int bar_num) {ASSERT_BAR_NUM (bar_num);@@ -120,13 +120,13 @@ uint32_t pci_bar_get(struct pci_dev *dev, int bar_num)static phys_addr_t __pci_bar_get_addr (struct pci_dev *dev,int bar_num) { -uint32_t bar =pci_bar_get (dev, bar_num); -uint32_t mask =pci_bar_mask (bar); +uint64_t bar =pci_bar_get (dev, bar_num); +uint64_t mask =pci_bar_mask (bar);uint64_t addr = bar & mask;phys_addr_t phys_addr;if (pci_bar_is64 (dev, bar_num)) - addr |= (uint64_t )pci_bar_get (dev, bar_num +1 ) <<32 ; + addr |=pci_bar_get (dev, bar_num +1 ) <<32 ; phys_addr =pci_translate_addr (dev->bdf, addr);assert (phys_addr !=INVALID_PHYS_ADDR );@@ -189,7 +189,7 @@ static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num)phys_addr_t pci_bar_size (struct pci_dev *dev,int bar_num) { -uint32_t bar, size; +uint64_t bar, size; size =pci_bar_size_helper (dev, bar_num);if (!size)@@ -210,7 +210,7 @@ phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num)bool pci_bar_is_memory (struct pci_dev *dev,int bar_num) { -uint32_t bar =pci_bar_get (dev, bar_num); +uint64_t bar =pci_bar_get (dev, bar_num);return !(bar &PCI_BASE_ADDRESS_SPACE_IO ); }@@ -222,7 +222,7 @@ bool pci_bar_is_valid(struct pci_dev *dev, int bar_num)bool pci_bar_is64 (struct pci_dev *dev,int bar_num) { -uint32_t bar =pci_bar_get (dev, bar_num); +uint64_t bar =pci_bar_get (dev, bar_num);if (bar &PCI_BASE_ADDRESS_SPACE_IO )return false;@@ -234,7 +234,7 @@ bool pci_bar_is64(struct pci_dev *dev, int bar_num)void pci_bar_print (struct pci_dev *dev,int bar_num) {phys_addr_t size, start, end; -uint32_t bar; +uint64_t bar;if (!pci_bar_is_valid (dev, bar_num))return ;lib/pci.h +2
-2@@ -60,8 +60,8 @@ extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);extern phys_addr_t pci_bar_get_addr (struct pci_dev *dev,int bar_num);extern void pci_bar_set_addr (struct pci_dev *dev,int bar_num,phys_addr_t addr);extern phys_addr_t pci_bar_size (struct pci_dev *dev,int bar_num); -extern uint32_t pci_bar_get (struct pci_dev *dev,int bar_num); -extern uint32_t pci_bar_mask (uint32_t bar); +extern uint64_t pci_bar_get (struct pci_dev *dev,int bar_num); +extern uint64_t pci_bar_mask (uint32_t bar);extern bool pci_bar_is64 (struct pci_dev *dev,int bar_num);extern bool pci_bar_is_memory (struct pci_dev *dev,int bar_num);extern bool pci_bar_is_valid (struct pci_dev *dev,int bar_num);
PATCH 1/1 pci: use uint64_t for unsigned long values
The "pci_bar_*" functions use 64-bit masks, but the results are assigned to 32-bit variables. Use 32-bit masks, since we're interested only in the least significant 4-bits.
--- lib/linux/pci_regs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
lib/linux/pci_regs.h +2 -2
@@ -96,8 +96,8 @@#define PCI_BASE_ADDRESS_MEM_TYPE_1M 0x02/* Below 1M [obsolete] */ #define PCI_BASE_ADDRESS_MEM_TYPE_64 0x04/* 64 bit address */ #define PCI_BASE_ADDRESS_MEM_PREFETCH 0x08/* prefetchable? */ -#define PCI_BASE_ADDRESS_MEM_MASK (~0x0fUL) -#define PCI_BASE_ADDRESS_IO_MASK (~0x03UL) +#define PCI_BASE_ADDRESS_MEM_MASK (~0x0fU) +#define PCI_BASE_ADDRESS_IO_MASK (~0x03U)/* bit 1 is reserved if address_space = 1 */ /* Header type 0 (normal devices) */
Signature · 1 line
2.23.0.700.g56cf767bdb-goog
Re: [kvm-unit-tests PATCH 1/1] pci: use uint64_t for unsigned long values
Hi,
Quoted 8 lines · Bill Wendling
The "pci_bar_*" functions use 64-bit masks, but the results are assigned to 32-bit variables. Use 32-bit masks, since we're interested only in the least significant 4-bits.
Signed-off-by Bill Wendling <[email protected]>--- lib/linux/pci_regs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
In my first comment I didn't realize that the file is actually a copy of a Linux user API header file (it's taken from include/uapi/linux/pci_regs.h). I don't think relying on changes to our own copy of the file is the best idea, because we might forget to include them the next time we sync our version with Linux.
I think the best approach would be to do the explicit cast to uint32_t in pci_bar_mask.
Thanks,
Alex
Quoted 16 lines
lib/linux/pci_regs.h +2
-2@@ -96,8 +96,8 @@#define PCI_BASE_ADDRESS_MEM_TYPE_1M 0x02/* Below 1M [obsolete] */ #define PCI_BASE_ADDRESS_MEM_TYPE_64 0x04/* 64 bit address */ #define PCI_BASE_ADDRESS_MEM_PREFETCH 0x08/* prefetchable? */ -#define PCI_BASE_ADDRESS_MEM_MASK (~0x0fUL) -#define PCI_BASE_ADDRESS_IO_MASK (~0x03UL) +#define PCI_BASE_ADDRESS_MEM_MASK (~0x0fU) +#define PCI_BASE_ADDRESS_IO_MASK (~0x03U)/* bit 1 is reserved if address_space = 1 */ /* Header type 0 (normal devices) */
Re: [kvm-unit-tests PATCH 1/1] pci: use uint64_t for unsigned long values
In reply to Bill WendlingThe subject "pci: use uint64_t for unsigned long values" does not reflect what the patch does.
This should be tagged v2.
Generally speaking, resend the whole series as v2 (or v3, etc...) even if only one patch is being tweaked. It's fairly obvious in this case that the other two other patches from v1 are still relevant, but that won't always be true.
Quoted 14 lines · Bill Wendling
The "pci_bar_*" functions use 64-bit masks, but the results are assigned to 32-bit variables. Use 32-bit masks, since we're interested only in the least significant 4-bits.
Signed-off-by Bill Wendling <[email protected]>--- lib/linux/pci_regs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)lib/linux/pci_regs.h +0
-0
lib/linux/pci_regs.h line 100
Quoted 7 lines
lib/linux/pci_regs.h +2
-2@@ -96,8 +96,8 @@#define PCI_BASE_ADDRESS_MEM_TYPE_1M 0x02/* Below 1M [obsolete] */ #define PCI_BASE_ADDRESS_MEM_TYPE_64 0x04/* 64 bit address */ #define PCI_BASE_ADDRESS_MEM_PREFETCH 0x08/* prefetchable? */ -#define PCI_BASE_ADDRESS_MEM_MASK (~0x0fUL) -#define PCI_BASE_ADDRESS_IO_MASK (~0x03UL) +#define PCI_BASE_ADDRESS_MEM_MASK (~0x0fU) +#define PCI_BASE_ADDRESS_IO_MASK (~0x03U)
The defines themselves should still be 64-bit, e.g. there's a flag right above to denote a 64-bit address. Explicitly casting the return value in pci_bar_mask() where we're dealing with 32-bit addresses would be preferable.
Quoted 6 lines
lib/linux/pci_regs.h +0
-0/* bit 1 is reserved if address_space = 1 */ /* Header type 0 (normal devices) */ Signature · 1 line
2.23.0.700.g56cf767bdb-goog
PATCH 3/3 Makefile: use "-Werror" in cc-option
In reply to Bill WendlingThe "cc-option" macro should use "-Werror" to determine if a flag is supported. Otherwise the test may not return a nonzero result. Also conditionalize some of the warning flags which aren't supported by clang.
--- Makefile | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)
Makefile +11 -8
@@ -46,30 +46,33 @@ include $(SRCDIR)/$(TEST_DIR)/Makefile # cc-option # Usage: OP_CFLAGS+=$(call cc-option, -falign-functions=0, -malign-functions=0) -cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \ +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) -COMMON_CFLAGS += -Wall -Wwrite-strings -Wclobbered -Wempty-body -Wuninitialized -COMMON_CFLAGS += -Wignored-qualifiers -Wunused-but-set-parameter -COMMON_CFLAGS += -Werror +COMMON_CFLAGS += -Wall -Wwrite-strings -Wempty-body -Wuninitialized +COMMON_CFLAGS += -Wignored-qualifiers -Werror + frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer fomit_frame_pointer := $(call cc-option, $(frame-pointer-flag), "") fnostack_protector := $(call cc-option, -fno-stack-protector, "") fnostack_protector_all := $(call cc-option, -fno-stack-protector-all, "") -wno_frame_address := $(call cc-option, -Wno-frame-address, "") fno_pic := $(call cc-option, -fno-pic, "") no_pie := $(call cc-option, -no-pie, "") COMMON_CFLAGS += $(fomit_frame_pointer) COMMON_CFLAGS += $(fno_stack_protector) COMMON_CFLAGS += $(fno_stack_protector_all) -COMMON_CFLAGS += $(wno_frame_address) COMMON_CFLAGS += $(if $(U32_LONG_FMT),-D__U32_LONG_FMT__,) COMMON_CFLAGS += $(fno_pic) $(no_pie) +COMMON_CFLAGS += $(call cc-option, -Wno-frame-address, "") +COMMON_CFLAGS += $(call cc-option, -Wclobbered, "") +COMMON_CFLAGS += $(call cc-option, -Wunused-but-set-parameter, "") +COMMON_CFLAGS += $(call cc-option, -Wmissing-parameter-type, "") +COMMON_CFLAGS += $(call cc-option, -Wold-style-declaration, "") + CFLAGS += $(COMMON_CFLAGS) -CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init -CFLAGS += -Wmissing-prototypes -Wstrict-prototypes +CFLAGS += -Woverride-init -Wmissing-prototypes -Wstrict-prototypes CXXFLAGS += $(COMMON_CFLAGS)
Signature · 1 line
2.23.0.700.g56cf767bdb-goog
Re: [kvm-unit-tests PATCH 3/3] Makefile: use "-Werror" in cc-option
Quoted 34 lines · Bill Wendling
The "cc-option" macro should use "-Werror" to determine if a flag is supported. Otherwise the test may not return a nonzero result. Also conditionalize some of the warning flags which aren't supported by clang.
Signed-off-by Bill Wendling <[email protected]>--- Makefile | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)Makefile +4
-5@@ -46,30 +46,33 @@ include $(SRCDIR)/$(TEST_DIR)/Makefile # cc-option # Usage: OP_CFLAGS+=$(call cc-option, -falign-functions=0, -malign-functions=0) -cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \ +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) -COMMON_CFLAGS += -Wall -Wwrite-strings -Wclobbered -Wempty-body -Wuninitialized -COMMON_CFLAGS += -Wignored-qualifiers -Wunused-but-set-parameter -COMMON_CFLAGS += -Werror +COMMON_CFLAGS += -Wall -Wwrite-strings -Wempty-body -Wuninitialized +COMMON_CFLAGS += -Wignored-qualifiers -Werror + frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer fomit_frame_pointer := $(call cc-option, $(frame-pointer-flag), "") fnostack_protector := $(call cc-option, -fno-stack-protector, "") fnostack_protector_all := $(call cc-option, -fno-stack-protector-all, "") -wno_frame_address := $(call cc-option, -Wno-frame-address, "")
Makefile include $(SRCDIR)/$(TEST_DIR)/Makefile · line 68
Quoted 10 lines
Makefile +1
-1fno_pic := $(call cc-option, -fno-pic, "") no_pie := $(call cc-option, -no-pie, "") COMMON_CFLAGS += $(fomit_frame_pointer) COMMON_CFLAGS += $(fno_stack_protector) COMMON_CFLAGS += $(fno_stack_protector_all) -COMMON_CFLAGS += $(wno_frame_address) COMMON_CFLAGS += $(if $(U32_LONG_FMT),-D__U32_LONG_FMT__,) COMMON_CFLAGS += $(fno_pic) $(no_pie) +COMMON_CFLAGS += $(call cc-option, -Wno-frame-address, "")
I think the old code used ":=" on purpose, so that the test is only done once. With your new code, the test is now done each time COMMON_CFLAGS gets evaluated, i.e. for each compiled object.
Could you please rewrite this test to use the ":=" detour for all lines that call cc-option?
Thanks,
Thomas
Re: [kvm-unit-tests PATCH 3/3] Makefile: use "-Werror" in cc-option
Quoted 45 lines · Thomas Huth
Quoted 43 lines · Bill Wendling
The "cc-option" macro should use "-Werror" to determine if a flag is supported. Otherwise the test may not return a nonzero result. Also conditionalize some of the warning flags which aren't supported by clang.
Signed-off-by Bill Wendling <[email protected]>--- Makefile | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)Makefile +4
-6@@ -46,30 +46,33 @@ include $(SRCDIR)/$(TEST_DIR)/Makefile # cc-option # Usage: OP_CFLAGS+=$(call cc-option, -falign-functions=0, -malign-functions=0) -cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \ +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) -COMMON_CFLAGS += -Wall -Wwrite-strings -Wclobbered -Wempty-body -Wuninitialized -COMMON_CFLAGS += -Wignored-qualifiers -Wunused-but-set-parameter -COMMON_CFLAGS += -Werror +COMMON_CFLAGS += -Wall -Wwrite-strings -Wempty-body -Wuninitialized +COMMON_CFLAGS += -Wignored-qualifiers -Werror + frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer fomit_frame_pointer := $(call cc-option, $(frame-pointer-flag), "") fnostack_protector := $(call cc-option, -fno-stack-protector, "") fnostack_protector_all := $(call cc-option, -fno-stack-protector-all, "") -wno_frame_address := $(call cc-option, -Wno-frame-address, "") fno_pic := $(call cc-option, -fno-pic, "") no_pie := $(call cc-option, -no-pie, "") COMMON_CFLAGS += $(fomit_frame_pointer) COMMON_CFLAGS += $(fno_stack_protector) COMMON_CFLAGS += $(fno_stack_protector_all) -COMMON_CFLAGS += $(wno_frame_address) COMMON_CFLAGS += $(if $(U32_LONG_FMT),-D__U32_LONG_FMT__,) COMMON_CFLAGS += $(fno_pic) $(no_pie)
Quoted 9 lines
Makefile include $(SRCDIR)/$(TEST_DIR)/Makefile · line 68
Quoted 1 line
Makefile +1
-0+COMMON_CFLAGS += $(call cc-option, -Wno-frame-address, "")I think the old code used ":=" on purpose, so that the test is only done once. With your new code, the test is now done each time
COMMON_CFLAGSgets evaluated, i.e. for each compiled object.Could you please rewrite this test to use the ":=" detour for all lines that call cc-option?
Does it rerun the "cc-option" call if the COMMON_CFLAGS is initially defined with ":="? I ask because the kernel calls its version of cc-option in the way I have it above, but the original definition of the variable uses ":=".
-bw
Re: [kvm-unit-tests PATCH 3/3] Makefile: use "-Werror" in cc-option
Quoted 52 lines · Bill Wendling
Quoted 51 lines · Thomas Huth
Quoted 34 lines · Bill Wendling
The "cc-option" macro should use "-Werror" to determine if a flag is supported. Otherwise the test may not return a nonzero result. Also conditionalize some of the warning flags which aren't supported by clang.
Signed-off-by Bill Wendling <[email protected]>--- Makefile | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)Makefile +4
-5@@ -46,30 +46,33 @@ include $(SRCDIR)/$(TEST_DIR)/Makefile # cc-option # Usage: OP_CFLAGS+=$(call cc-option, -falign-functions=0, -malign-functions=0) -cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \ +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) -COMMON_CFLAGS += -Wall -Wwrite-strings -Wclobbered -Wempty-body -Wuninitialized -COMMON_CFLAGS += -Wignored-qualifiers -Wunused-but-set-parameter -COMMON_CFLAGS += -Werror +COMMON_CFLAGS += -Wall -Wwrite-strings -Wempty-body -Wuninitialized +COMMON_CFLAGS += -Wignored-qualifiers -Werror + frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer fomit_frame_pointer := $(call cc-option, $(frame-pointer-flag), "") fnostack_protector := $(call cc-option, -fno-stack-protector, "") fnostack_protector_all := $(call cc-option, -fno-stack-protector-all, "") -wno_frame_address := $(call cc-option, -Wno-frame-address, "")Makefile include $(SRCDIR)/$(TEST_DIR)/Makefile · line 68
Quoted 10 lines
Makefile +1
-1fno_pic := $(call cc-option, -fno-pic, "") no_pie := $(call cc-option, -no-pie, "") COMMON_CFLAGS += $(fomit_frame_pointer) COMMON_CFLAGS += $(fno_stack_protector) COMMON_CFLAGS += $(fno_stack_protector_all) -COMMON_CFLAGS += $(wno_frame_address) COMMON_CFLAGS += $(if $(U32_LONG_FMT),-D__U32_LONG_FMT__,) COMMON_CFLAGS += $(fno_pic) $(no_pie) +COMMON_CFLAGS += $(call cc-option, -Wno-frame-address, "")I think the old code used ":=" on purpose, so that the test is only done once. With your new code, the test is now done each time
COMMON_CFLAGSgets evaluated, i.e. for each compiled object.
Quoted 5 lines
Quoted 3 lines
Could you please rewrite this test to use the ":=" detour for all lines that call cc-option?
Does it rerun the "cc-option" call if the
COMMON_CFLAGSis initially defined with ":="?
It does not rerun the "cc-option" call in that case, but compilation fails. I think it's likely due to the line that contains the "$(if $(U32_LONG_FMT),-D__U32_LONG_FMT__,)" since that likely needs to be evaluated dynamically.
Thomas
cover Patches for clang compilation
In reply to Bill WendlingThese four patches fix compilation issues that clang's encountering.
I ran the tests and there are no regressions with gcc.
Bill Wendling (4):
x86: emulator: use "SSE2" for the target
pci: cast the masks to the appropriate size
Makefile: use "-Werror" in cc-option
Makefile: add "cxx-option" for C++ builds
Makefile | 23 +++++++++++++++-------- lib/pci.c | 3 ++- x86/emulator.c | 2 +- 3 files changed, 18 insertions(+), 10 deletions(-)
Signature · 1 line
2.23.0.700.g56cf767bdb-goog
PATCH 1/4 x86: emulator: use "SSE2" for the target
The movdqu and movapd instructions are SSE2 instructions. Clang interprets the __attribute__((target("sse"))) as allowing SSE only instructions. Using SSE2 instructions cause an error.
--- x86/emulator.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
x86/emulator.c +1 -1
@@ -657,7 +657,7 @@ static bool sseeq(sse_union *v1, sse_union *v2)return ok; } -static __attribute__((target ("sse" )))void test_sse (sse_union *mem) +static __attribute__((target ("sse2" )))void test_sse (sse_union *mem) {sse_union v;
Signature · 1 line
2.23.0.700.g56cf767bdb-goog
PATCH 2/4 pci: cast the masks to the appropriate size
In reply to Bill WendlingAt this point, we're dealing with 32-bit addresses, therefore downcast the masks to 32-bits.
--- lib/pci.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
lib/pci.c +2 -1
@@ -107,7 +107,8 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)uint32_t pci_bar_mask (uint32_t bar) {return (bar &PCI_BASE_ADDRESS_SPACE_IO ) ? -PCI_BASE_ADDRESS_IO_MASK :PCI_BASE_ADDRESS_MEM_MASK ; + (uint32_t )PCI_BASE_ADDRESS_IO_MASK : + (uint32_t )PCI_BASE_ADDRESS_MEM_MASK ; }uint32_t pci_bar_get (struct pci_dev *dev,int bar_num)
Signature · 1 line
2.23.0.700.g56cf767bdb-goog
PATCH 3/4 Makefile: use "-Werror" in cc-option
In reply to Bill WendlingThe "cc-option" macro should use "-Werror" to determine if a flag is supported. Otherwise the test may not return a nonzero result. Also conditionalize some of the warning flags which aren't supported by clang.
--- Makefile | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)
Makefile +11 -8
@@ -46,30 +46,33 @@ include $(SRCDIR)/$(TEST_DIR)/Makefile # cc-option # Usage: OP_CFLAGS+=$(call cc-option, -falign-functions=0, -malign-functions=0) -cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \ +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) -COMMON_CFLAGS += -Wall -Wwrite-strings -Wclobbered -Wempty-body -Wuninitialized -COMMON_CFLAGS += -Wignored-qualifiers -Wunused-but-set-parameter -COMMON_CFLAGS += -Werror +COMMON_CFLAGS += -Wall -Wwrite-strings -Wempty-body -Wuninitialized +COMMON_CFLAGS += -Wignored-qualifiers -Werror + frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer fomit_frame_pointer := $(call cc-option, $(frame-pointer-flag), "") fnostack_protector := $(call cc-option, -fno-stack-protector, "") fnostack_protector_all := $(call cc-option, -fno-stack-protector-all, "") -wno_frame_address := $(call cc-option, -Wno-frame-address, "") fno_pic := $(call cc-option, -fno-pic, "") no_pie := $(call cc-option, -no-pie, "") COMMON_CFLAGS += $(fomit_frame_pointer) COMMON_CFLAGS += $(fno_stack_protector) COMMON_CFLAGS += $(fno_stack_protector_all) -COMMON_CFLAGS += $(wno_frame_address) COMMON_CFLAGS += $(if $(U32_LONG_FMT),-D__U32_LONG_FMT__,) COMMON_CFLAGS += $(fno_pic) $(no_pie) +COMMON_CFLAGS += $(call cc-option, -Wno-frame-address, "") +COMMON_CFLAGS += $(call cc-option, -Wclobbered, "") +COMMON_CFLAGS += $(call cc-option, -Wunused-but-set-parameter, "") +COMMON_CFLAGS += $(call cc-option, -Wmissing-parameter-type, "") +COMMON_CFLAGS += $(call cc-option, -Wold-style-declaration, "") + CFLAGS += $(COMMON_CFLAGS) -CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init -CFLAGS += -Wmissing-prototypes -Wstrict-prototypes +CFLAGS += -Woverride-init -Wmissing-prototypes -Wstrict-prototypes CXXFLAGS += $(COMMON_CFLAGS)
Signature · 1 line
2.23.0.700.g56cf767bdb-goog
PATCH 4/4 Makefile: add "cxx-option" for C++ builds
In reply to Bill WendlingThe C++ compiler may not support all of the same flags as the C compiler. Add a separate test for these flags.
--- Makefile | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
Makefile +6 -2
@@ -48,6 +48,8 @@ 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 ;) +cxx-option = $(shell if $(CXX) -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) COMMON_CFLAGS += -Wall -Wwrite-strings -Wempty-body -Wuninitialized
@@ -68,13 +70,15 @@ COMMON_CFLAGS += $(fno_pic) $(no_pie) COMMON_CFLAGS += $(call cc-option, -Wno-frame-address, "") COMMON_CFLAGS += $(call cc-option, -Wclobbered, "") COMMON_CFLAGS += $(call cc-option, -Wunused-but-set-parameter, "") -COMMON_CFLAGS += $(call cc-option, -Wmissing-parameter-type, "") -COMMON_CFLAGS += $(call cc-option, -Wold-style-declaration, "") CFLAGS += $(COMMON_CFLAGS) +CFLAGS += $(call cc-option, -Wmissing-parameter-type, "") +CFLAGS += $(call cc-option, -Wold-style-declaration, "") CFLAGS += -Woverride-init -Wmissing-prototypes -Wstrict-prototypes CXXFLAGS += $(COMMON_CFLAGS) +CXXFLAGS += $(call cxx-option, -Wmissing-parameter-type, "") +CXXFLAGS += $(call cxx-option, -Wold-style-declaration, "") autodepend-flags = -MMD -MF $(dir $*).$(notdir $*).d
Signature · 1 line
2.23.0.700.g56cf767bdb-goog
Crap! I used what I thought were the correct command line arguments for "git send-email", but it didn't add the "v2" bit. My apologies. All of these patches should be v2 for the originals.
Quoted 19 lines · Bill Wendling
These four patches fix compilation issues that clang's encountering.
I ran the tests and there are no regressions with gcc.
Bill Wendling (4):
x86: emulator: use "SSE2" for the target
pci: cast the masks to the appropriate size
Makefile: use "-Werror" in cc-option
Makefile: add "cxx-option" for C++ buildsMakefile | 23 +++++++++++++++-------- lib/pci.c | 3 ++- x86/emulator.c | 2 +- 3 files changed, 18 insertions(+), 10 deletions(-)--
2.23.0.700.g56cf767bdb-goog
Quoted 4 lines · Bill Wendling
Crap! I used what I thought were the correct command line arguments for "git send-email", but it didn't add the "v2" bit. My apologies. All of these patches should be v2 for the originals.
I recommend that you send them again with v2 in the title to avoid confusion.
Done.
Quoted 9 lines · Nadav Amit
Quoted 4 lines · Bill Wendling
Crap! I used what I thought were the correct command line arguments for "git send-email", but it didn't add the "v2" bit. My apologies. All of these patches should be v2 for the originals.
I recommend that you send them again with v2 in the title to avoid confusion.
v2 cover Patches for clang compilation
In reply to Bill WendlingThese four patches fix compilation issues that clang's encountering.
I ran the tests and there are no regressions with gcc.
Bill Wendling (4):
x86: emulator: use "SSE2" for the target
pci: cast the masks to the appropriate size
Makefile: use "-Werror" in cc-option
Makefile: add "cxx-option" for C++ builds
Makefile | 23 +++++++++++++++-------- lib/pci.c | 3 ++- x86/emulator.c | 2 +- 3 files changed, 18 insertions(+), 10 deletions(-)
Signature · 1 line
2.23.0.700.g56cf767bdb-goog
PATCH v2 1/4 x86: emulator: use "SSE2" for the target
The movdqu and movapd instructions are SSE2 instructions. Clang interprets the __attribute__((target("sse"))) as allowing SSE only instructions. Using SSE2 instructions cause an error.
--- x86/emulator.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
x86/emulator.c +1 -1
@@ -657,7 +657,7 @@ static bool sseeq(sse_union *v1, sse_union *v2)return ok; } -static __attribute__((target ("sse" )))void test_sse (sse_union *mem) +static __attribute__((target ("sse2" )))void test_sse (sse_union *mem) {sse_union v;
Signature · 1 line
2.23.0.700.g56cf767bdb-goog
PATCH v2 2/4 pci: cast the masks to the appropriate size
In reply to Bill WendlingAt this point, we're dealing with 32-bit addresses, therefore downcast the masks to 32-bits.
--- lib/pci.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
lib/pci.c +2 -1
@@ -107,7 +107,8 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)uint32_t pci_bar_mask (uint32_t bar) {return (bar &PCI_BASE_ADDRESS_SPACE_IO ) ? -PCI_BASE_ADDRESS_IO_MASK :PCI_BASE_ADDRESS_MEM_MASK ; + (uint32_t )PCI_BASE_ADDRESS_IO_MASK : + (uint32_t )PCI_BASE_ADDRESS_MEM_MASK ; }uint32_t pci_bar_get (struct pci_dev *dev,int bar_num)
Signature · 1 line
2.23.0.700.g56cf767bdb-goog
PATCH v2 3/4 Makefile: use "-Werror" in cc-option
In reply to Bill WendlingThe "cc-option" macro should use "-Werror" to determine if a flag is supported. Otherwise the test may not return a nonzero result. Also conditionalize some of the warning flags which aren't supported by clang.
--- Makefile | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)
Makefile +11 -8
@@ -46,30 +46,33 @@ include $(SRCDIR)/$(TEST_DIR)/Makefile # cc-option # Usage: OP_CFLAGS+=$(call cc-option, -falign-functions=0, -malign-functions=0) -cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \ +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) -COMMON_CFLAGS += -Wall -Wwrite-strings -Wclobbered -Wempty-body -Wuninitialized -COMMON_CFLAGS += -Wignored-qualifiers -Wunused-but-set-parameter -COMMON_CFLAGS += -Werror +COMMON_CFLAGS += -Wall -Wwrite-strings -Wempty-body -Wuninitialized +COMMON_CFLAGS += -Wignored-qualifiers -Werror + frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer fomit_frame_pointer := $(call cc-option, $(frame-pointer-flag), "") fnostack_protector := $(call cc-option, -fno-stack-protector, "") fnostack_protector_all := $(call cc-option, -fno-stack-protector-all, "") -wno_frame_address := $(call cc-option, -Wno-frame-address, "") fno_pic := $(call cc-option, -fno-pic, "") no_pie := $(call cc-option, -no-pie, "") COMMON_CFLAGS += $(fomit_frame_pointer) COMMON_CFLAGS += $(fno_stack_protector) COMMON_CFLAGS += $(fno_stack_protector_all) -COMMON_CFLAGS += $(wno_frame_address) COMMON_CFLAGS += $(if $(U32_LONG_FMT),-D__U32_LONG_FMT__,) COMMON_CFLAGS += $(fno_pic) $(no_pie) +COMMON_CFLAGS += $(call cc-option, -Wno-frame-address, "") +COMMON_CFLAGS += $(call cc-option, -Wclobbered, "") +COMMON_CFLAGS += $(call cc-option, -Wunused-but-set-parameter, "") +COMMON_CFLAGS += $(call cc-option, -Wmissing-parameter-type, "") +COMMON_CFLAGS += $(call cc-option, -Wold-style-declaration, "") + CFLAGS += $(COMMON_CFLAGS) -CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init -CFLAGS += -Wmissing-prototypes -Wstrict-prototypes +CFLAGS += -Woverride-init -Wmissing-prototypes -Wstrict-prototypes CXXFLAGS += $(COMMON_CFLAGS)
Signature · 1 line
2.23.0.700.g56cf767bdb-goog
PATCH v2 4/4 Makefile: add "cxx-option" for C++ builds
In reply to Bill WendlingThe C++ compiler may not support all of the same flags as the C compiler. Add a separate test for these flags.
--- Makefile | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
Makefile +6 -2
@@ -48,6 +48,8 @@ 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 ;) +cxx-option = $(shell if $(CXX) -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) COMMON_CFLAGS += -Wall -Wwrite-strings -Wempty-body -Wuninitialized
@@ -68,13 +70,15 @@ COMMON_CFLAGS += $(fno_pic) $(no_pie) COMMON_CFLAGS += $(call cc-option, -Wno-frame-address, "") COMMON_CFLAGS += $(call cc-option, -Wclobbered, "") COMMON_CFLAGS += $(call cc-option, -Wunused-but-set-parameter, "") -COMMON_CFLAGS += $(call cc-option, -Wmissing-parameter-type, "") -COMMON_CFLAGS += $(call cc-option, -Wold-style-declaration, "") CFLAGS += $(COMMON_CFLAGS) +CFLAGS += $(call cc-option, -Wmissing-parameter-type, "") +CFLAGS += $(call cc-option, -Wold-style-declaration, "") CFLAGS += -Woverride-init -Wmissing-prototypes -Wstrict-prototypes CXXFLAGS += $(COMMON_CFLAGS) +CXXFLAGS += $(call cxx-option, -Wmissing-parameter-type, "") +CXXFLAGS += $(call cxx-option, -Wold-style-declaration, "") autodepend-flags = -MMD -MF $(dir $*).$(notdir $*).d
Signature · 1 line
2.23.0.700.g56cf767bdb-goog
v3 cover Patches for clang compilation
In reply to Bill WendlingThis series of patches includes extra from the original patch set:
- Changes to the "use -Werror in cc-option" patch to address Thomas Huth's comments.
- Two fixes for code clang warned about: shifting negative numbers, and misuse of the "registers for local variables" feature.
Bill Wendling (6):
x86: emulator: use "SSE2" for the target
pci: cast the masks to the appropriate size
Makefile: use "-Werror" in cc-option
Makefile: add "cxx-option" for C++ builds
x86: use a non-negative number in shift
x86: use inline asm to retrieve stack pointer
Makefile | 30 +++++++++++++++++++++++------- lib/pci.c | 3 ++- x86/emulator.c | 2 +- x86/svm.c | 2 +- x86/vmx_tests.c | 8 ++++++-- 5 files changed, 33 insertions(+), 12 deletions(-)
Signature · 1 line
2.24.0.rc1.363.gb1bccd3e3d-goog
PATCH v3 1/6 x86: emulator: use "SSE2" for the target
The movdqu and movapd instructions are SSE2 instructions. Clang interprets the __attribute__((target("sse"))) as allowing SSE only instructions. Using SSE2 instructions cause an error.
--- x86/emulator.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
x86/emulator.c +1 -1
@@ -657,7 +657,7 @@ static bool sseeq(sse_union *v1, sse_union *v2)return ok; } -static __attribute__((target ("sse" )))void test_sse (sse_union *mem) +static __attribute__((target ("sse2" )))void test_sse (sse_union *mem) {sse_union v;
Signature · 1 line
2.24.0.rc1.363.gb1bccd3e3d-goog
Re: [kvm-unit-tests PATCH v3 1/6] x86: emulator: use "SSE2" for the target
Quoted 14 lines · Bill Wendling
The movdqu and movapd instructions are SSE2 instructions. Clang interprets the __attribute__((target("sse"))) as allowing SSE only instructions. Using SSE2 instructions cause an error.
Signed-off-by Bill Wendling <[email protected]>--- x86/emulator.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)x86/emulator.c +0
-0
x86/emulator.c static bool sseeq(sse_union *v1, sse_union *v2) · line 662
Quoted 7 lines
x86/emulator.c +1
-1@@ -657,7 +657,7 @@ static bool sseeq(sse_union *v1, sse_union *v2)return ok; } -static __attribute__((target ("sse" )))void test_sse (sse_union *mem) +static __attribute__((target ("sse2" )))void test_sse (sse_union *mem) {sse_union v;
That seems to work fine with both, gcc and clang, thus:
PATCH v3 2/6 pci: cast the masks to the appropriate size
In reply to Bill WendlingAt this point, we're dealing with 32-bit addresses, therefore downcast the masks to 32-bits.
--- lib/pci.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
lib/pci.c +2 -1
@@ -107,7 +107,8 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)uint32_t pci_bar_mask (uint32_t bar) {return (bar &PCI_BASE_ADDRESS_SPACE_IO ) ? -PCI_BASE_ADDRESS_IO_MASK :PCI_BASE_ADDRESS_MEM_MASK ; + (uint32_t )PCI_BASE_ADDRESS_IO_MASK : + (uint32_t )PCI_BASE_ADDRESS_MEM_MASK ; }uint32_t pci_bar_get (struct pci_dev *dev,int bar_num)
Signature · 1 line
2.24.0.rc1.363.gb1bccd3e3d-goog
Re: [kvm-unit-tests PATCH v3 2/6] pci: cast the masks to the appropriate size
Quoted 2 lines · Bill Wendling
At this point, we're dealing with 32-bit addresses, therefore downcast the masks to 32-bits.
In case you respin, please mention the warning from clang that you are fixing here.
Quoted 5 lines
lib/pci.c +0
-0
lib/pci.c pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id) · line 114
Quoted 9 lines
lib/pci.c +2
-1@@ -107,7 +107,8 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)uint32_t pci_bar_mask (uint32_t bar) {return (bar &PCI_BASE_ADDRESS_SPACE_IO ) ? -PCI_BASE_ADDRESS_IO_MASK :PCI_BASE_ADDRESS_MEM_MASK ; + (uint32_t )PCI_BASE_ADDRESS_IO_MASK : + (uint32_t )PCI_BASE_ADDRESS_MEM_MASK ; }uint32_t pci_bar_get (struct pci_dev *dev,int bar_num)
PATCH v3 3/6 Makefile: use "-Werror" in cc-option
In reply to Bill WendlingThe "cc-option" macro should use "-Werror" to determine if a flag is supported. Otherwise the test may not return a nonzero result. Also conditionalize some of the warning flags which aren't supported by clang.
--- Makefile | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
Makefile +10 -6
@@ -46,13 +46,13 @@ include $(SRCDIR)/$(TEST_DIR)/Makefile # cc-option # Usage: OP_CFLAGS+=$(call cc-option, -falign-functions=0, -malign-functions=0) -cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \ +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) -COMMON_CFLAGS += -Wall -Wwrite-strings -Wclobbered -Wempty-body -Wuninitialized -COMMON_CFLAGS += -Wignored-qualifiers -Wunused-but-set-parameter -COMMON_CFLAGS += -Werror +COMMON_CFLAGS += -Wall -Wwrite-strings -Wempty-body -Wuninitialized +COMMON_CFLAGS += -Wignored-qualifiers -Werror + frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer fomit_frame_pointer := $(call cc-option, $(frame-pointer-flag), "") fnostack_protector := $(call cc-option, -fno-stack-protector, "")
@@ -60,16 +60,20 @@ fnostack_protector_all := $(call cc-option, -fno-stack-protector-all, "") wno_frame_address := $(call cc-option, -Wno-frame-address, "") fno_pic := $(call cc-option, -fno-pic, "") no_pie := $(call cc-option, -no-pie, "") +wclobbered := $(call cc-option, -Wclobbered, "") +wunused_but_set_parameter := $(call cc-option, -Wunused-but-set-parameter, "") + COMMON_CFLAGS += $(fomit_frame_pointer) COMMON_CFLAGS += $(fno_stack_protector) COMMON_CFLAGS += $(fno_stack_protector_all) COMMON_CFLAGS += $(wno_frame_address) COMMON_CFLAGS += $(if $(U32_LONG_FMT),-D__U32_LONG_FMT__,) COMMON_CFLAGS += $(fno_pic) $(no_pie) +COMMON_CFLAGS += $(wclobbered) +COMMON_CFLAGS += $(wunused_but_set_parameter) CFLAGS += $(COMMON_CFLAGS) -CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init -CFLAGS += -Wmissing-prototypes -Wstrict-prototypes +CFLAGS += -Woverride-init -Wmissing-prototypes -Wstrict-prototypes CXXFLAGS += $(COMMON_CFLAGS)
Signature · 1 line
2.24.0.rc1.363.gb1bccd3e3d-goog
Re: [kvm-unit-tests PATCH v3 3/6] Makefile: use "-Werror" in cc-option
Quoted 37 lines · Bill Wendling
The "cc-option" macro should use "-Werror" to determine if a flag is supported. Otherwise the test may not return a nonzero result. Also conditionalize some of the warning flags which aren't supported by clang.
Signed-off-by Bill Wendling <[email protected]>--- Makefile | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)Makefile +5
-4@@ -46,13 +46,13 @@ include $(SRCDIR)/$(TEST_DIR)/Makefile # cc-option # Usage: OP_CFLAGS+=$(call cc-option, -falign-functions=0, -malign-functions=0) -cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \ +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) -COMMON_CFLAGS += -Wall -Wwrite-strings -Wclobbered -Wempty-body -Wuninitialized -COMMON_CFLAGS += -Wignored-qualifiers -Wunused-but-set-parameter -COMMON_CFLAGS += -Werror +COMMON_CFLAGS += -Wall -Wwrite-strings -Wempty-body -Wuninitialized +COMMON_CFLAGS += -Wignored-qualifiers -Werror + frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer fomit_frame_pointer := $(call cc-option, $(frame-pointer-flag), "") fnostack_protector := $(call cc-option, -fno-stack-protector, "")@@ -60,16 +60,20 @@ fnostack_protector_all := $(call cc-option, -fno-stack-protector-all, "") wno_frame_address := $(call cc-option, -Wno-frame-address, "") fno_pic := $(call cc-option, -fno-pic, "") no_pie := $(call cc-option, -no-pie, "") +wclobbered := $(call cc-option, -Wclobbered, "")
Makefile fnostack_protector_all := $(call cc-option, -fno-stack-protector-all, "") · line 73
Quoted 10 lines
Makefile +4
-0+wunused_but_set_parameter := $(call cc-option, -Wunused-but-set-parameter, "") + COMMON_CFLAGS += $(fomit_frame_pointer) COMMON_CFLAGS += $(fno_stack_protector) COMMON_CFLAGS += $(fno_stack_protector_all) COMMON_CFLAGS += $(wno_frame_address) COMMON_CFLAGS += $(if $(U32_LONG_FMT),-D__U32_LONG_FMT__,) COMMON_CFLAGS += $(fno_pic) $(no_pie) +COMMON_CFLAGS += $(wclobbered) +COMMON_CFLAGS += $(wunused_but_set_parameter)
Looks good to me up to this point here...
Makefile fnostack_protector_all := $(call cc-option, -fno-stack-protector-all, "") · line 76
Quoted 4 lines
Makefile +1
-2CFLAGS += $(COMMON_CFLAGS) -CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init -CFLAGS += -Wmissing-prototypes -Wstrict-prototypes +CFLAGS += -Woverride-init -Wmissing-prototypes -Wstrict-prototypes
... but I think this hunk rather belongs to the next patch instead?
Thomas
Re: [kvm-unit-tests PATCH v3 3/6] Makefile: use "-Werror" in cc-option
I sent out another set of patches for the -Werror, because I didn't see the ones I posted on the list and thought they got lost.
Quoted 61 lines · Thomas Huth
Quoted 37 lines · Bill Wendling
The "cc-option" macro should use "-Werror" to determine if a flag is supported. Otherwise the test may not return a nonzero result. Also conditionalize some of the warning flags which aren't supported by clang.
Signed-off-by Bill Wendling <[email protected]>--- Makefile | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)Makefile +5
-4@@ -46,13 +46,13 @@ include $(SRCDIR)/$(TEST_DIR)/Makefile # cc-option # Usage: OP_CFLAGS+=$(call cc-option, -falign-functions=0, -malign-functions=0) -cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \ +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) -COMMON_CFLAGS += -Wall -Wwrite-strings -Wclobbered -Wempty-body -Wuninitialized -COMMON_CFLAGS += -Wignored-qualifiers -Wunused-but-set-parameter -COMMON_CFLAGS += -Werror +COMMON_CFLAGS += -Wall -Wwrite-strings -Wempty-body -Wuninitialized +COMMON_CFLAGS += -Wignored-qualifiers -Werror + frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer fomit_frame_pointer := $(call cc-option, $(frame-pointer-flag), "") fnostack_protector := $(call cc-option, -fno-stack-protector, "")@@ -60,16 +60,20 @@ fnostack_protector_all := $(call cc-option, -fno-stack-protector-all, "") wno_frame_address := $(call cc-option, -Wno-frame-address, "") fno_pic := $(call cc-option, -fno-pic, "") no_pie := $(call cc-option, -no-pie, "") +wclobbered := $(call cc-option, -Wclobbered, "")Makefile fnostack_protector_all := $(call cc-option, -fno-stack-protector-all, "") · line 73
Quoted 10 lines
Makefile +4
-0+wunused_but_set_parameter := $(call cc-option, -Wunused-but-set-parameter, "") + COMMON_CFLAGS += $(fomit_frame_pointer) COMMON_CFLAGS += $(fno_stack_protector) COMMON_CFLAGS += $(fno_stack_protector_all) COMMON_CFLAGS += $(wno_frame_address) COMMON_CFLAGS += $(if $(U32_LONG_FMT),-D__U32_LONG_FMT__,) COMMON_CFLAGS += $(fno_pic) $(no_pie) +COMMON_CFLAGS += $(wclobbered) +COMMON_CFLAGS += $(wunused_but_set_parameter)Looks good to me up to this point here...
Makefile fnostack_protector_all := $(call cc-option, -fno-stack-protector-all, "") · line 76
Quoted 4 lines
Makefile +1
-2CFLAGS += $(COMMON_CFLAGS) -CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init -CFLAGS += -Wmissing-prototypes -Wstrict-prototypes +CFLAGS += -Woverride-init -Wmissing-prototypes -Wstrict-prototypes... but I think this hunk rather belongs to the next patch instead?
Thomas
PATCH v3 4/6 Makefile: add "cxx-option" for C++ builds
In reply to Bill WendlingThe C++ compiler may not support all of the same flags as the C compiler. Add a separate test for these flags.
--- Makefile | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
Makefile +13 -1
@@ -48,6 +48,8 @@ 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 ;) +cxx-option = $(shell if $(CXX) -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) COMMON_CFLAGS += -Wall -Wwrite-strings -Wempty-body -Wuninitialized
@@ -73,9 +75,19 @@ COMMON_CFLAGS += $(wclobbered) COMMON_CFLAGS += $(wunused_but_set_parameter) CFLAGS += $(COMMON_CFLAGS) +CXXFLAGS += $(COMMON_CFLAGS) + +wmissing_parameter_type := $(call cc-option, -Wmissing-parameter-type, "") +wold_style_declaration := $(call cc-option, -Wold-style-declaration, "") +CFLAGS += $(wmissing_parameter_type) +CFLAGS += $(wold_style_declaration) CFLAGS += -Woverride-init -Wmissing-prototypes -Wstrict-prototypes -CXXFLAGS += $(COMMON_CFLAGS) +# Clang's C++ compiler doesn't support some of the flags its C compiler does. +wmissing_parameter_type := $(call cxx-option, -Wmissing-parameter-type, "") +wold_style_declaration := $(call cxx-option, -Wold-style-declaration, "") +CXXFLAGS += $(wmissing_parameter_type) +CXXFLAGS += $(wold_style_declaration) autodepend-flags = -MMD -MF $(dir $*).$(notdir $*).d
Signature · 1 line
2.24.0.rc1.363.gb1bccd3e3d-goog
Re: [kvm-unit-tests PATCH v3 4/6] Makefile: add "cxx-option" for C++ builds
Quoted 29 lines · Bill Wendling
The C++ compiler may not support all of the same flags as the C compiler. Add a separate test for these flags.
Signed-off-by Bill Wendling <[email protected]>--- Makefile | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)Makefile +6
-0@@ -48,6 +48,8 @@ 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 ;) +cxx-option = $(shell if $(CXX) -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) COMMON_CFLAGS += -Wall -Wwrite-strings -Wempty-body -Wuninitialized@@ -73,9 +75,19 @@ COMMON_CFLAGS += $(wclobbered) COMMON_CFLAGS += $(wunused_but_set_parameter) CFLAGS += $(COMMON_CFLAGS) +CXXFLAGS += $(COMMON_CFLAGS) + +wmissing_parameter_type := $(call cc-option, -Wmissing-parameter-type, "") +wold_style_declaration := $(call cc-option, -Wold-style-declaration, "")
Makefile COMMON_CFLAGS += $(wclobbered) · line 90
Quoted 10 lines
Makefile +7
-1+CFLAGS += $(wmissing_parameter_type) +CFLAGS += $(wold_style_declaration) CFLAGS += -Woverride-init -Wmissing-prototypes -Wstrict-prototypes -CXXFLAGS += $(COMMON_CFLAGS) +# Clang's C++ compiler doesn't support some of the flags its C compiler does. +wmissing_parameter_type := $(call cxx-option, -Wmissing-parameter-type, "") +wold_style_declaration := $(call cxx-option, -Wold-style-declaration, "") +CXXFLAGS += $(wmissing_parameter_type) +CXXFLAGS += $(wold_style_declaration)
According to the man page of gcc-9, both options are for "C and Objective-C only". I think you can simply always remove them from the CXXFLAGS.
Thomas
PATCH v3 5/6 x86: use a non-negative number in shift
In reply to Bill WendlingShifting a negative number is undefined. Clang complains about it:
x86/svm.c:1131:38: error: shifting a negative signed value is undefined [-Werror,-Wshift-negative-value]
test->vmcb->control.tsc_offset = TSC_OFFSET_VALUE;
Using "~0ull" results in identical asm code:
before: movabsq $-281474976710656, %rsi after: movabsq $-281474976710656, %rsi
--- x86/svm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
x86/svm.c +1 -1
@@ -1122,7 +1122,7 @@ static bool npt_rw_l1mmio_check(struct test *test) }#define TSC_ADJUST_VALUE (1ll << 32) -#define TSC_OFFSET_VALUE (-1ll << 48) +#define TSC_OFFSET_VALUE (~0ull << 48)static bool ok;static void tsc_adjust_prepare (struct test *test)
Signature · 1 line
2.24.0.rc1.363.gb1bccd3e3d-goog
Re: [kvm-unit-tests PATCH v3 5/6] x86: use a non-negative number in shift
Quoted 21 lines · Bill Wendling
Shifting a negative number is undefined. Clang complains about it:
x86/svm.c:1131:38: error: shifting a negative signed value is undefined [-Werror,-Wshift-negative-value]test->vmcb->control.tsc_offset = TSC_OFFSET_VALUE;Using "~0ull" results in identical asm code:
before: movabsq $-281474976710656, %rsi after: movabsq $-281474976710656, %rsiSigned-off-by Bill Wendling <[email protected]>--- x86/svm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)x86/svm.c +0
-0
x86/svm.c static bool npt_rw_l1mmio_check(struct test *test) · line 1128
Quoted 8 lines
x86/svm.c +1
-1@@ -1122,7 +1122,7 @@ static bool npt_rw_l1mmio_check(struct test *test) }#define TSC_ADJUST_VALUE (1ll << 32) -#define TSC_OFFSET_VALUE (-1ll << 48) +#define TSC_OFFSET_VALUE (~0ull << 48)static bool ok;static void tsc_adjust_prepare (struct test *test)
PATCH v3 6/6 x86: use inline asm to retrieve stack pointer
In reply to Bill WendlingAccording to GCC's documentation, the only supported use for specifying registers for local variables is "to specify registers for input and output operands when calling Extended asm." Using it as a shortcut to get the value in a register isn't guaranteed to work, and clang complains that the variable is uninitialized.
--- x86/vmx_tests.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
x86/vmx_tests.c +6 -2
@@ -2138,7 +2138,9 @@ static void into_guest_main(void) .offset = (uintptr_t)&&into, .selector = KERNEL_CS32, }; - registeruintptr_t rsp asm("rsp" ); +uintptr_t rsp; + + asmvolatile ("mov %%rsp, %0" :"=r" (rsp));if (fp.offset != (uintptr_t )&&into) {printf ("Code address too high.\n" );
@@ -3221,7 +3223,9 @@ static void try_compat_invvpid(void *unused) .offset = (uintptr_t)&&invvpid, .selector = KERNEL_CS32, }; - registeruintptr_t rsp asm("rsp" ); +uintptr_t rsp; + + asmvolatile ("mov %%rsp, %0" :"=r" (rsp));TEST_ASSERT_MSG (fp.offset == (uintptr_t )&&invvpid,"Code address too high." );
Signature · 1 line
2.24.0.rc1.363.gb1bccd3e3d-goog