| Message ID | 68db6d1a-6498-30a6-6604-a568056dd1e0@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] x86emul: fix test harness and fuzzer build dependencies | expand |
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.
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
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
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.
--- 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
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.