[Xen-devel] [PATCH v3] x86emul: fix test harness and fuzzer build dependencies

[Xen-devel] [PATCH v3] x86emul: fix test harness and fuzzer build dependencies

[Jan Beulich]

Commit fd35f32b4b ("tools/x86emul: Use struct cpuid_policy in the
userspace test harnesses") didn't account for the dependencies of
cpuid-autogen.h to potentially change between incremental builds. In
particular the harness has a "run" goal which is supposed to be usable
independently of the rest of the tools sub-tree building, and both the
harness and the fuzzer code are also supposed to be buildable
independently. Therefore a re-build of the generated header needs to
be triggered first, which is achieved by introducing a new top-level
target pattern (for just the "run" part for now).

Finally cpuid.o did not have any dependencies added for it.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: Something similar would be nice for building both tools/tests/*/
      and tools/fuzz/*/, but I'm uncertain whether respective top level
      targets build-tests-% and build-fuzz-% would be welcome.
---
v3: Introduce top level run-tests-% target.
v2.1: Split controversial parts from (hopefully) non-controversial ones.
v2: Guard $(MAKE) invocations by $(MAKELEVEL) checks.

--- a/Makefile
+++ b/Makefile
@@ -80,6 +80,9 @@ build-docs:
  test:
  	$(MAKE) -C tools/python test
  
+run-tests-%: build-tools-public-headers tools/tests/%/
+	$(MAKE) -C tools/tests/$* run
+
  # For most targets here,
  #   make COMPONENT-TARGET
  # is implemented, more or less, by
--- a/tools/fuzz/x86_instruction_emulator/Makefile
+++ b/tools/fuzz/x86_instruction_emulator/Makefile
@@ -26,13 +26,15 @@ GCOV_FLAGS := --coverage
  	$(CC) -c $(CFLAGS) $(GCOV_FLAGS) $< -o $@
  
  x86.h := $(addprefix $(XEN_ROOT)/tools/include/xen/asm/,\
-                     x86-vendors.h x86-defns.h msr-index.h)
+                     x86-vendors.h x86-defns.h msr-index.h) \
+         $(addprefix $(XEN_ROOT)/tools/include/xen/lib/x86/, \
+                     cpuid.h cpuid-autogen.h)
  x86_emulate.h := x86-emulate.h x86_emulate/x86_emulate.h $(x86.h)
  
  # x86-emulate.c will be implicit for both
  x86-emulate.o x86-emulate-cov.o: x86_emulate/x86_emulate.c $(x86_emulate.h)
  
-fuzz-emul.o fuzz-emulate-cov.o wrappers.o: $(x86_emulate.h)
+fuzz-emul.o fuzz-emulate-cov.o cpuid.o wrappers.o: $(x86_emulate.h)
  
  x86-insn-fuzzer.a: fuzz-emul.o x86-emulate.o cpuid.o
  	$(AR) rc $@ $^
--- a/tools/tests/x86_emulator/Makefile
+++ b/tools/tests/x86_emulator/Makefile
@@ -280,10 +280,12 @@ $(call cc-option-add,HOSTCFLAGS-x86_64,H
  HOSTCFLAGS += $(CFLAGS_xeninclude) -I. $(HOSTCFLAGS-$(XEN_COMPILE_ARCH))
  
  x86.h := $(addprefix $(XEN_ROOT)/tools/include/xen/asm/,\
-                     x86-vendors.h x86-defns.h msr-index.h)
+                     x86-vendors.h x86-defns.h msr-index.h) \
+         $(addprefix $(XEN_ROOT)/tools/include/xen/lib/x86/, \
+                     cpuid.h cpuid-autogen.h)
  x86_emulate.h := x86-emulate.h x86_emulate/x86_emulate.h $(x86.h)
  
-x86-emulate.o test_x86_emulator.o evex-disp8.o wrappers.o: %.o: %.c $(x86_emulate.h)
+x86-emulate.o cpuid.o test_x86_emulator.o evex-disp8.o wrappers.o: %.o: %.c $(x86_emulate.h)
  	$(HOSTCC) $(HOSTCFLAGS) -c -g -o $@ $<
  
  x86-emulate.o: x86_emulate/x86_emulate.c

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3] x86emul: fix test harness and fuzzer build dependencies

[George Dunlap]

On 8/9/19 4:40 PM, Jan Beulich wrote:
> Commit fd35f32b4b ("tools/x86emul: Use struct cpuid_policy in the
> userspace test harnesses") didn't account for the dependencies of
> cpuid-autogen.h to potentially change between incremental builds. In
> particular the harness has a "run" goal which is supposed to be usable
> independently of the rest of the tools sub-tree building, and both the
> harness and the fuzzer code are also supposed to be buildable
> independently. Therefore a re-build of the generated header needs to
> be triggered first, which is achieved by introducing a new top-level
> target pattern (for just the "run" part for now).
> 
> Finally cpuid.o did not have any dependencies added for it.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TBD: Something similar would be nice for building both tools/tests/*/
>      and tools/fuzz/*/, but I'm uncertain whether respective top level
>      targets build-tests-% and build-fuzz-% would be welcome.
> ---
> v3: Introduce top level run-tests-% target.
> v2.1: Split controversial parts from (hopefully) non-controversial ones.
> v2: Guard $(MAKE) invocations by $(MAKELEVEL) checks.
> 
> --- a/Makefile
> +++ b/Makefile
> @@ -80,6 +80,9 @@ build-docs:
>  test:
>      $(MAKE) -C tools/python test
>  
> +run-tests-%: build-tools-public-headers tools/tests/%/
> +    $(MAKE) -C tools/tests/$* run
> +
>  # For most targets here,
>  #   make COMPONENT-TARGET
>  # is implemented, more or less, by

Hmm -- Thunderbird seems to know that there's only one space here, but
when I save this mail and try to `git am` it, I get hunk failures like this:

```
diff a/Makefile b/Makefile	(rejected hunks)
@@ -80,6 +80,9 @@ build-docs:
  test:
  	$(MAKE) -C tools/python test

+run-tests-%: build-tools-public-headers tools/tests/%/
+	$(MAKE) -C tools/tests/$* run
+
  # For most targets here,
  #   make COMPONENT-TARGET
  # is implemented, more or less, by
```

Note two spaces before the # rather than 1.  I looked at the raw email
and it has two spaces:

```
--- a/Makefile
+++ b/Makefile
@@ -80,6 +80,9 @@ build-docs:
  test:
  	$(MAKE) -C tools/python test

+run-tests-%: build-tools-public-headers tools/tests/%/
+	$(MAKE) -C tools/tests/$* run
+
  # For most targets here,
  #   make COMPONENT-TARGET
  # is implemented, more or less, by
```

Oh, weird -- but the version on gmail is a completely different
encoding: gmail has it as encoded base64, whereas my corporate mail has
it encoded as 7-bit.  What does everyone else see?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3] x86emul: fix test harness and fuzzer build dependencies

[Jan Beulich]

Commit fd35f32b4b ("tools/x86emul: Use struct cpuid_policy in the
userspace test harnesses") didn't account for the dependencies of
cpuid-autogen.h to potentially change between incremental builds. In
particular the harness has a "run" goal which is supposed to be usable
independently of the rest of the tools sub-tree building, and both the
harness and the fuzzer code are also supposed to be buildable
independently. Therefore a re-build of the generated header needs to be
triggered first, which is achieved by introducing a new top-level target
pattern (for just the "run" part for now).

Further cpuid.o did not have any dependencies added for it.

Finally, while at it, add a "run" target to the cpu-policy test harness.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: Something similar would be nice for building both tools/tests/*/
     and tools/fuzz/*/, but I'm uncertain whether respective top level
     targets build-tests-% and build-fuzz-% would be welcome.
---
v3: Introduce top level run-tests-% target.
v2.1: Split controversial parts from (hopefully) non-controversial ones.
v2: Guard $(MAKE) invocations by $(MAKELEVEL) checks.

--- a/Makefile
+++ b/Makefile
@@ -80,6 +80,9 @@ build-docs:
 test:
 	$(MAKE) -C tools/python test
 
+run-tests-%: build-tools-public-headers tools/tests/%/
+	$(MAKE) -C tools/tests/$* run
+
 # For most targets here,
 #   make COMPONENT-TARGET
 # is implemented, more or less, by
--- a/tools/fuzz/x86_instruction_emulator/Makefile
+++ b/tools/fuzz/x86_instruction_emulator/Makefile
@@ -26,13 +26,15 @@ GCOV_FLAGS := --coverage
 	$(CC) -c $(CFLAGS) $(GCOV_FLAGS) $< -o $@
 
 x86.h := $(addprefix $(XEN_ROOT)/tools/include/xen/asm/,\
-                     x86-vendors.h x86-defns.h msr-index.h)
+                     x86-vendors.h x86-defns.h msr-index.h) \
+         $(addprefix $(XEN_ROOT)/tools/include/xen/lib/x86/, \
+                     cpuid.h cpuid-autogen.h)
 x86_emulate.h := x86-emulate.h x86_emulate/x86_emulate.h $(x86.h)
 
 # x86-emulate.c will be implicit for both
 x86-emulate.o x86-emulate-cov.o: x86_emulate/x86_emulate.c $(x86_emulate.h)
 
-fuzz-emul.o fuzz-emulate-cov.o wrappers.o: $(x86_emulate.h)
+fuzz-emul.o fuzz-emulate-cov.o cpuid.o wrappers.o: $(x86_emulate.h)
 
 x86-insn-fuzzer.a: fuzz-emul.o x86-emulate.o cpuid.o
 	$(AR) rc $@ $^
--- a/tools/tests/cpu-policy/Makefile
+++ b/tools/tests/cpu-policy/Makefile
@@ -17,6 +17,10 @@ endif
 .PHONY: all
 all: $(TARGET-y)
 
+.PHONY: run
+run: $(TARGET-y)
+	./$(TARGET-y)
+
 .PHONY: clean
 clean:
 	$(RM) -f -- *.o .*.d .*.d2 test-cpu-policy
--- a/tools/tests/x86_emulator/Makefile
+++ b/tools/tests/x86_emulator/Makefile
@@ -280,10 +280,12 @@ $(call cc-option-add,HOSTCFLAGS-x86_64,H
 HOSTCFLAGS += $(CFLAGS_xeninclude) -I. $(HOSTCFLAGS-$(XEN_COMPILE_ARCH))
 
 x86.h := $(addprefix $(XEN_ROOT)/tools/include/xen/asm/,\
-                     x86-vendors.h x86-defns.h msr-index.h)
+                     x86-vendors.h x86-defns.h msr-index.h) \
+         $(addprefix $(XEN_ROOT)/tools/include/xen/lib/x86/, \
+                     cpuid.h cpuid-autogen.h)
 x86_emulate.h := x86-emulate.h x86_emulate/x86_emulate.h $(x86.h)
 
-x86-emulate.o test_x86_emulator.o evex-disp8.o wrappers.o: %.o: %.c $(x86_emulate.h)
+x86-emulate.o cpuid.o test_x86_emulator.o evex-disp8.o wrappers.o: %.o: %.c $(x86_emulate.h)
 	$(HOSTCC) $(HOSTCFLAGS) -c -g -o $@ $<
 
 x86-emulate.o: x86_emulate/x86_emulate.c

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3] x86emul: fix test harness and fuzzer build dependencies

[Andrew Cooper]

On 05/09/2019 15:09, Jan Beulich wrote:
> Commit fd35f32b4b ("tools/x86emul: Use struct cpuid_policy in the
> userspace test harnesses") didn't account for the dependencies of
> cpuid-autogen.h to potentially change between incremental builds. In
> particular the harness has a "run" goal which is supposed to be usable
> independently of the rest of the tools sub-tree building, and both the
> harness and the fuzzer code are also supposed to be buildable
> independently. Therefore a re-build of the generated header needs to be
> triggered first, which is achieved by introducing a new top-level target
> pattern (for just the "run" part for now).
>
> Further cpuid.o did not have any dependencies added for it.
>
> Finally, while at it, add a "run" target to the cpu-policy test harness.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> TBD: Something similar would be nice for building both tools/tests/*/
>      and tools/fuzz/*/, but I'm uncertain whether respective top level
>      targets build-tests-% and build-fuzz-% would be welcome.

Fuzz targets are much more complicated to set up and run correctly.  I'd
skip them for now.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3] x86emul: fix test harness and fuzzer build dependencies

[Jan Beulich]

On 09.09.2019 13:01, Andrew Cooper wrote:
> On 05/09/2019 15:09, Jan Beulich wrote:
>> Commit fd35f32b4b ("tools/x86emul: Use struct cpuid_policy in the
>> userspace test harnesses") didn't account for the dependencies of
>> cpuid-autogen.h to potentially change between incremental builds. In
>> particular the harness has a "run" goal which is supposed to be usable
>> independently of the rest of the tools sub-tree building, and both the
>> harness and the fuzzer code are also supposed to be buildable
>> independently. Therefore a re-build of the generated header needs to be
>> triggered first, which is achieved by introducing a new top-level target
>> pattern (for just the "run" part for now).
>>
>> Further cpuid.o did not have any dependencies added for it.
>>
>> Finally, while at it, add a "run" target to the cpu-policy test harness.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

>> ---
>> TBD: Something similar would be nice for building both tools/tests/*/
>>      and tools/fuzz/*/, but I'm uncertain whether respective top level
>>      targets build-tests-% and build-fuzz-% would be welcome.
> 
> Fuzz targets are much more complicated to set up and run correctly.  I'd
> skip them for now.

Well, building shouldn't be that difficult - one only needs to set the
compiler etc correctly iirc. Running is less straightforward, which is
why I've mentioned only the build part.

Independent of the fuzzer aspect, what about a special top level build
target for the test harnesses (alongside the run one introduced here)?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3] x86emul: fix test harness and fuzzer build dependencies

[Jan Beulich]

On 09.09.2019 13:01, Andrew Cooper wrote:
> On 05/09/2019 15:09, Jan Beulich wrote:
>> Commit fd35f32b4b ("tools/x86emul: Use struct cpuid_policy in the
>> userspace test harnesses") didn't account for the dependencies of
>> cpuid-autogen.h to potentially change between incremental builds. In
>> particular the harness has a "run" goal which is supposed to be usable
>> independently of the rest of the tools sub-tree building, and both the
>> harness and the fuzzer code are also supposed to be buildable
>> independently. Therefore a re-build of the generated header needs to be
>> triggered first, which is achieved by introducing a new top-level target
>> pattern (for just the "run" part for now).
>>
>> Further cpuid.o did not have any dependencies added for it.
>>
>> Finally, while at it, add a "run" target to the cpu-policy test harness.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Ian,

you had been particularly unhappy about the v2 approach. While
not strictly required for committing, I'd still prefer to have
your agreement with this approach (or, of course, suggestions
for improvement).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3] x86emul: fix test harness and fuzzer build dependencies

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v3] x86emul: fix test harness and fuzzer build dependencies"):
> you had been particularly unhappy about the v2 approach. While
> not strictly required for committing, I'd still prefer to have
> your agreement with this approach (or, of course, suggestions
> for improvement).

Thanks for the enquiry.  I appreciate the chance to comment.  I do not
object to this approach.

I think the extra convenience is not really worth the the complexity
in the top-level Makefile but if you think it is worthwhile I won't
stand in your way.

I think it's important that these run targets not get entangled with
the rest of the build system: ie that no non-run targets call them.
If we maintain that rule then we won't introduce new build race bugs.

So overall, 

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel