Patches for clang compilation

PATCH In review Bill Wendling 5 participants 37 messages Oct 10, 2019 – Nov 28, 2019

Subsystems (3)

KVM Kbuild Library code

lore

Share & tools

lore

Bill Wendling

author

2019-10-10 18:35 UTC #0 lore raw

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

Bill Wendling

author

2019-10-10 18:35 UTC #1 lore raw

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.

Signed-off-by Bill Wendling <[email protected]>

---
 x86/emulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

x86/emulator.c +1 -1

index 621caf9..bec0154 100644
@@ -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

Bill Wendling

author

2019-10-10 18:35 UTC #2 lore raw

PATCH 2/3 pci: use uint64_t for unsigned long values

In reply to Bill Wendling

The "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)
                                         ^~~~~~~

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

index daa33e1..e554209 100644
@@ -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

index 689f03c..cd12938 100644
@@ -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

Alexandru Elisei

2019-10-11 09:12 UTC #3 lore raw

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_t for the data types. Clang complains about this. So we bump up the type to uint64_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

index daa33e1..e554209 100644
@@ -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

index 689f03c..cd12938 100644
@@ -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);

Bill Wendling

author

2019-10-12 08:26 UTC #4 lore raw

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.

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 +2 -2

index 1becea8..3bc2b92 100644
@@ -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

Alexandru Elisei

2019-10-14 16:33 UTC #5 lore raw

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

index 1becea8..3bc2b92 100644
@@ -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) */

Sean Christopherson

2019-10-14 16:36 UTC #6 lore raw

Re: [kvm-unit-tests PATCH 1/1] pci: use uint64_t for unsigned long values

In reply to Bill Wendling

The 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
index 1becea8..3bc2b92 100644

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

Bill Wendling

author

2019-10-10 18:35 UTC #7 lore raw

PATCH 3/3 Makefile: use "-Werror" in cc-option

In reply to 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 +11 -8

index 32414dc..3ec0458 100644
@@ -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

Thomas Huth

2019-10-15 07:29 UTC #8 lore raw

Re: [kvm-unit-tests PATCH 3/3] Makefile: use "-Werror" in cc-option

Quoted 34 lines · Bill Wendling

Signed-off-by Bill Wendling <[email protected]>

---
 Makefile | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

Makefile +4 -5

index 32414dc..3ec0458 100644
@@ -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 -1

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

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

Bill Wendling

author

2019-10-15 07:58 UTC #9 lore raw

Re: [kvm-unit-tests PATCH 3/3] Makefile: use "-Werror" in cc-option

Quoted 45 lines · Thomas Huth

Quoted 43 lines · Bill Wendling

Signed-off-by Bill Wendling <[email protected]>

---
 Makefile | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

Makefile +4 -6

index 32414dc..3ec0458 100644
@@ -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_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?

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

Thomas Huth

2019-10-15 08:12 UTC #10 lore raw

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

Signed-off-by Bill Wendling <[email protected]>

---
 Makefile | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

Makefile +4 -5

index 32414dc..3ec0458 100644
@@ -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 -1

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

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.

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_CFLAGS is 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

Bill Wendling

author

2019-10-14 19:24 UTC #11 lore raw

cover Patches for clang compilation

In reply to 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++ 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

Bill Wendling

author

2019-10-14 19:24 UTC #12 lore raw

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.

Signed-off-by Bill Wendling <[email protected]>

---
 x86/emulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

x86/emulator.c +1 -1

index 621caf9..bec0154 100644
@@ -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

Bill Wendling

author

2019-10-14 19:24 UTC #13 lore raw

PATCH 2/4 pci: cast the masks to the appropriate size

In reply to Bill Wendling

At this point, we're dealing with 32-bit addresses, therefore downcast the masks to 32-bits.

Signed-off-by Bill Wendling <[email protected]>

---
 lib/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

lib/pci.c +2 -1

index daa33e1..1b85411 100644
@@ -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

Bill Wendling

author

2019-10-14 19:24 UTC #14 lore raw

PATCH 3/4 Makefile: use "-Werror" in cc-option

In reply to Bill Wendling

Signed-off-by Bill Wendling <[email protected]>

---
 Makefile | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

Makefile +11 -8

index 32414dc..3ec0458 100644
@@ -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

Bill Wendling

author

2019-10-14 19:24 UTC #15 lore raw

PATCH 4/4 Makefile: add "cxx-option" for C++ builds

In reply to 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 | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Makefile +6 -2

index 3ec0458..b9ae791 100644
@@ -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

Bill Wendling

author

2019-10-14 19:26 UTC #16 lore raw

In reply to 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.

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++ builds
 Makefile       | 23 +++++++++++++++--------
 lib/pci.c      |  3 ++-
 x86/emulator.c |  2 +-
 3 files changed, 18 insertions(+), 10 deletions(-)
--
2.23.0.700.g56cf767bdb-goog

Nadav Amit

2019-10-14 23:56 UTC #17 lore raw

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.

Bill Wendling

author

2019-10-15 00:04 UTC #18 lore raw

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.

Bill Wendling

author

2019-10-15 00:04 UTC #19 lore raw

v2 cover Patches for clang compilation

In reply to 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++ 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

Bill Wendling

author

2019-10-15 00:04 UTC #20 lore raw

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.

Signed-off-by Bill Wendling <[email protected]>

---
 x86/emulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

x86/emulator.c +1 -1

index 621caf9..bec0154 100644
@@ -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

Bill Wendling

author

2019-10-15 00:04 UTC #21 lore raw

PATCH v2 2/4 pci: cast the masks to the appropriate size

In reply to Bill Wendling

At this point, we're dealing with 32-bit addresses, therefore downcast the masks to 32-bits.

Signed-off-by Bill Wendling <[email protected]>

---
 lib/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

lib/pci.c +2 -1

index daa33e1..1b85411 100644
@@ -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

Bill Wendling

author

2019-10-15 00:04 UTC #22 lore raw

PATCH v2 3/4 Makefile: use "-Werror" in cc-option

In reply to Bill Wendling

Signed-off-by Bill Wendling <[email protected]>

---
 Makefile | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

Makefile +11 -8

index 32414dc..3ec0458 100644
@@ -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

Bill Wendling

author

2019-10-15 00:04 UTC #23 lore raw

PATCH v2 4/4 Makefile: add "cxx-option" for C++ builds

In reply to 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 | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Makefile +6 -2

index 3ec0458..b9ae791 100644
@@ -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

Bill Wendling

author

2019-10-30 21:04 UTC #24 lore raw

v3 cover Patches for clang compilation

In reply to Bill Wendling

This 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

Bill Wendling

author

2019-10-30 21:04 UTC #25 lore raw

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.

Signed-off-by Bill Wendling <[email protected]>

---
 x86/emulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

x86/emulator.c +1 -1

index 621caf9..bec0154 100644
@@ -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

Thomas Huth

Tested-by on v3 1/6

2019-11-28 07:31 UTC #26 lore raw

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
index 621caf9..bec0154 100644

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:

Tested-by Thomas Huth <[email protected]>

Bill Wendling

author

2019-10-30 21:04 UTC #27 lore raw

PATCH v3 2/6 pci: cast the masks to the appropriate size

In reply to Bill Wendling

At this point, we're dealing with 32-bit addresses, therefore downcast the masks to 32-bits.

Signed-off-by Bill Wendling <[email protected]>

---
 lib/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

lib/pci.c +2 -1

index daa33e1..1b85411 100644
@@ -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

Thomas Huth

Reviewed-by on v3 2/6

2019-11-08 08:39 UTC #28 lore raw

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

index daa33e1..1b85411 100644

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)

Reviewed-by Thomas Huth <[email protected]>

Bill Wendling

author

2019-10-30 21:04 UTC #29 lore raw

PATCH v3 3/6 Makefile: use "-Werror" in cc-option

In reply to Bill Wendling

Signed-off-by Bill Wendling <[email protected]>

---
 Makefile | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Makefile +10 -6

index 32414dc..6201c45 100644
@@ -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

Thomas Huth

2019-11-08 08:43 UTC #30 lore raw

Re: [kvm-unit-tests PATCH v3 3/6] Makefile: use "-Werror" in cc-option

Quoted 37 lines · Bill Wendling

Signed-off-by Bill Wendling <[email protected]>

---
  Makefile | 16 ++++++++++------
  1 file changed, 10 insertions(+), 6 deletions(-)

Makefile +5 -4

index 32414dc..6201c45 100644
@@ -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 -2

  CFLAGS += $(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

Bill Wendling

author

2019-11-08 20:36 UTC #31 lore raw

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

Signed-off-by Bill Wendling <[email protected]>

---
  Makefile | 16 ++++++++++------
  1 file changed, 10 insertions(+), 6 deletions(-)

Makefile +5 -4

index 32414dc..6201c45 100644
@@ -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 -2

  CFLAGS += $(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

Bill Wendling

author

2019-10-30 21:04 UTC #32 lore raw

PATCH v3 4/6 Makefile: add "cxx-option" for C++ builds

In reply to 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 +13 -1

index 6201c45..9cb47e6 100644
@@ -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

Thomas Huth

2019-11-08 08:49 UTC #33 lore raw

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

index 6201c45..9cb47e6 100644
@@ -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

Bill Wendling

author

2019-10-30 21:04 UTC #34 lore raw

PATCH v3 5/6 x86: use a non-negative number in shift

In reply to 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, %rsi

Signed-off-by Bill Wendling <[email protected]>

---
 x86/svm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

x86/svm.c +1 -1

index 4ddfaa4..cef43d5 100644
@@ -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

Thomas Huth

Reviewed-by on v3 5/6

2019-11-08 08:31 UTC #35 lore raw

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, %rsi
Signed-off-by Bill Wendling <[email protected]>
---
  x86/svm.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
x86/svm.c +0 -0
index 4ddfaa4..cef43d5 100644

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)

Reviewed-by Thomas Huth <[email protected]>

Bill Wendling

author

2019-10-30 21:04 UTC #36 lore raw

PATCH v3 6/6 x86: use inline asm to retrieve stack pointer

In reply to Bill Wendling

According 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.

Signed-off-by Bill Wendling <[email protected]>

---
 x86/vmx_tests.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

x86/vmx_tests.c +6 -2

index 4aebc3f..565b69b 100644
@@ -2138,7 +2138,9 @@ static void into_guest_main(void)
 		.offset = (uintptr_t)&&into,
 		.selector = KERNEL_CS32,
 	};
-	register uintptr_t rsp asm("rsp");
+	uintptr_t rsp;
+
+	asm volatile ("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,
 	};
-	register uintptr_t rsp asm("rsp");
+	uintptr_t rsp;
+
+	asm volatile ("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