[v3] x86emul: fix test harness and fuzzer build dependencies
diff mbox series

Message ID 68db6d1a-6498-30a6-6604-a568056dd1e0@suse.com
State New, archived
Headers show
Series
  • [v3] x86emul: fix test harness and fuzzer build dependencies
Related show

Commit Message

Jan Beulich Sept. 5, 2019, 2:09 p.m. UTC
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.

Comments

Andrew Cooper Sept. 9, 2019, 11:01 a.m. UTC | #1
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.
Jan Beulich Sept. 9, 2019, 11:37 a.m. UTC | #2
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
Jan Beulich Sept. 9, 2019, 11:39 a.m. UTC | #3
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
Ian Jackson Sept. 9, 2019, 12:49 p.m. UTC | #4
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.

Patch
diff mbox series

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