libflash/test: Rewrite Makefile.check to improve scalability

The current implementation makes it hard to expand the list of tests if
we want to build anything that doesn't link to mbox-server. This is a
consequence of embedding the $(LIBFLASH_TEST_EXTRA) variable inside the
recipes for building test executables, which makes the makefile a bit of
a maze to navigate.

To address this we could go the route of duplicating the
$(LIBFLASH_TEST), $(LIBFLASH_TEST_EXTRA) and the corresponding make
directives (targets/prerequisites/recipes) each time we want to link a
binary against a new set of objects, but that seems ham-fisted.

Further, $(LIBFLASH_TEST_EXTRA) is defined in terms of the relevant
object (.o) files, but the recipes it is used in otherwise use source
(.c) paths for compilation. These other paths are typically to non-test
code that needs to be compiled into the test executable, but we can't
use object files at the usual path because we will typically have a
conflict of architectures (PPC64 for the skiboot object, x86_64 for the
test object). This in turn means that we will compile source files
multiple times (once for each test binary it is required in) rather than
re-using an existing object file.

Further, the current structure of the Makefile requires we #include the
.c file under test directly into the test source if we want it in a
specific test case due to the relationship of the prerequisites to the
build (only the first source prerequisite is included in the build). The
include-the-c-file approach can have some annoying side-effects with
respect to macros, typically errors regarding redefinition. While it is
useful for testing static functions in the source under test, it would
be nice if this approach was optional rather than required.

This change attempts to address all of these issues. The outcome is we
have precise control of which objects get linked into each test binary,
we avoid the architecture clash problem, we re-use existing compiled
objects (avoiding recompilation), and we make the include-the-c-file
approach optional.

The general approach is to generate a new directory hierarchy of object
files under a `$(HOSTCC) -dumpmachine` directory in the repository root
and use these for linking the test cases. Objects that land in this
segregated tree are described by a _SOURCES variable for each test,
similar in structure and behaviour to automake's _SOURCES variables.
Again similar to automake, a check_PROGRAMS variable is used that
describes the path of each test binary to be built.

The test binary paths are mapped to the corresponding _SOURCES variable
by some secondary-evaluation wizardry that no-one has to pay any
attention to once it is written. Whilst the implementation is perhaps
slightly tricky, it allows us to avoid the recipe headache of
unconditionally linking in objects defined in variables that don't
directly participate in the target's prerequisites, and so prevents the
explosion of variables as we implement tests that require disjoint sets
of dependencies.

This is initially intended as an isolated experiment with the libflash
test makefile, but it's feasible that the scope of the concept could be
expanded to other test Makefiles.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Stewart Smith <stewart@linux.ibm.com>

1 files changed, 137 insertions, 26 deletions

diff --git a/libflash/test/Makefile.check b/libflash/test/Makefile.check
index f4ad96e..da45884 100644
--- a/libflash/test/Makefile.check
+++ b/libflash/test/Makefile.check
@@ -1,42 +1,153 @@
 # -*-Makefile-*-
-TEST_FLAGS = -D__TEST__
+libflash_test_test_flash_SOURCES = \
+	libflash/test/test-flash.c \
+	libflash/test/stubs.c \
+	libflash/test/mbox-server.c
 
-LIBFLASH_TEST := libflash/test/test-flash libflash/test/test-ecc libflash/test/test-blocklevel libflash/test/test-mbox
+libflash_test_test_ecc_SOURCES = \
+	libflash/test/test-ecc.c \
+	libflash/test/stubs.c \
+	libflash/test/mbox-server.c
 
-LCOV_EXCLUDE += $(LIBFLASH_TEST:%=%.c)
+libflash_test_test_mbox_SOURCES = \
+	libflash/test/test-mbox.c \
+	libflash/test/stubs.c \
+	libflash/test/mbox-server.c
 
-.PHONY : libflash-check libc-coverage
-libflash-check: $(LIBFLASH_TEST:%=%-check) $(CORE_TEST:%=%-gcov-run)
-libflash-coverage: $(LIBFLASH_TEST:%=%-gcov-run)
+check_PROGRAMS = \
+	libflash/test/test-flash \
+	libflash/test/test-ecc \
+	libflash/test/test-mbox
 
-check: libflash-check libc-coverage
-coverage: libflash-coverage
+TEST_FLAGS = -D__TEST__
 
+.PHONY: libflash-check libflash-coverage
+libflash-check: $(check_PROGRAMS:%=%-check) $(CORE_TEST:%=%-gcov-run)
+libflash-coverage: $(check_PROGRAMS:%=%-gcov-run)
+clean: libflash-test-clean
+check: libflash-check
+coverage: libflash-coverage
 strict-check: TEST_FLAGS += -D__STRICT_TEST__
 strict-check: check
 
-$(LIBFLASH_TEST:%=%-gcov-run) : %-run: %
-	$(call QTEST, TEST-COVERAGE ,$< , $<)
+LCOV_EXCLUDE += $(check_PROGRAMS:%=%.c)
 
-$(LIBFLASH_TEST:%=%-check) : %-check: %
-	$(call QTEST, RUN-TEST ,$(VALGRIND) $<, $<)
+$(check_PROGRAMS:%=%-check) : %-check : %
+	$(call QTEST, RUN-TEST , $(VALGRIND) $<, $<)
 
-LIBFLASH_TEST_EXTRA :=  libflash/test/stubs.o libflash/test/mbox-server.o
-$(LIBFLASH_TEST_EXTRA) : %.o : %.c
-	$(call Q, HOSTCC ,$(HOSTCC) $(HOSTCFLAGS) $(TEST_FLAGS) -Wno-suggest-attribute=const -g -c -o $@ $<, $<)
+# Transform a prerequisite into something approximating a variable name. This
+# is used to map check_PROGRAMS prerequisits to the corresponding _SOURCES
+# variable.
+#
+# For example:
+#
+#   $(call prereq2var,libflash/test/test-mbox)
+#
+# Will output:
+#
+#   'libflash_test_test_mbox'
+#
+prereq2var = $(subst /,_,$(subst -,_,$(1)))
 
-$(LIBFLASH_TEST) : libflash/libflash.c libflash/ecc.c libflash/blocklevel.c $(LIBFLASH_TEST_EXTRA)
-$(LIBFLASH_TEST) : % : %.c
-	$(call Q, HOSTCC ,$(HOSTCC) $(HOSTCFLAGS) $(TEST_FLAGS) -Wno-suggest-attribute=const -O0 -g -I include -I . -o $@ $< $(LIBFLASH_TEST_EXTRA), $<)
+# Generate prerequisites from a target based on the target's corresponding
+# _SOURCES variable.
+#
+# For example, with:
+#
+#   libflash_test_test_mbox_SOURCES = \
+#       libflash/test/test-mbox.c \
+#       libflash/test/stubs.c \
+#       libflash/test/mbox-server.c
+#   HOST_TRIPLE = x86_64-linux-gnu
+#
+# A call to target2prereq where the target is libflash/test/test-mbox:
+#
+#   $(call target2prereq,$@,$(HOST_TRIPLE)/)
+#
+# Will output:
+#
+#   x86_64-linux-gnu/libflash/test/test-mbox.o
+#   x86_64-linux-gnu/libflash/test/stubs.o
+#   x86_64-linux-gnu/libflash/test/mbox-server.o
+target2prereq = $(patsubst %.c,%.o,$(addprefix $(2),$($(call prereq2var,$(1))_SOURCES)))
 
-$(LIBFLASH_TEST:%=%-gcov): %-gcov : %.c %
-	$(call Q, HOSTCC ,$(HOSTCC) $(HOSTCFLAGS) $(HOSTGCOVCFLAGS) $(TEST_FLAGS) -Wno-suggest-attribute=const -I include -I . -o $@ $< $(LIBFLASH_TEST_EXTRA), $<)
+# Generate path stems for all applications in check_PROGRAMS. This is usef
+#
+# For example, with:
+#
+#   libflash_test_test_mbox_SOURCES = \
+#       libflash/test/test-mbox.c \
+#       libflash/test/stubs.c \
+#       libflash/test/mbox-server.c
+#   libflash_test_test_ecc_SOURCES = \
+#       libflash/test/test-ecc.c \
+#       libflash/test/stubs.c \
+#       libflash/test/mbox-server.c
+#   check_PROGRAMS = libflash/test/test-mbox libflash/test/test-ecc
+#   HOST_TRIPLE = x86_64-linux-gnu
+#
+# A call to:
+#
+#   $(call objstem,$(check_PROGRAMS),$(HOST_TRIPLE)/)
+#
+# Will output:
+#
+#   x86_64-linux-gnu/libflash/test/test-mbox
+#   x86_64-linux-gnu/libflash/test/stubs
+#   x86_64-linux-gnu/libflash/test/mbox-server
+#   x86_64-linux-gnu/libflash/test/test-ecc
+#   x86_64-linux-gnu/libflash/test/stubs
+#   x86_64-linux-gnu/libflash/test/mbox-server
+objstem = $(patsubst %.c,%,$(addprefix $(2),$(foreach bin,$(1),$($(call prereq2var,$(bin))_SOURCES))))
 
--include $(wildcard libflash/test/*.d)
+# Record the host platform triple to separate test vs production objects.
+HOST_TRIPLE = $(shell $(HOSTCC) -dumpmachine)
 
-clean: libflash-test-clean
+# Mirror the skiboot directory structure under a directory named after the host
+# triple in the skiboot root directory, and place the built objects in this
+# mirrored structure.
+$(HOST_TRIPLE)/%.o : %.c
+	@mkdir -p $(dir $@)
+	$(call Q, HOSTCC ,$(HOSTCC) $(HOSTCFLAGS) $(TEST_FLAGS) -Wno-suggest-attribute=const -g -c -o $@ $<, $@)
+
+# Use GNU make metaprogramming dynamically define targets and prequisites for
+# binaries listed in check_PROGRAMS.
+#
+# Secondary expansion[1] allows us to use the target automatic variable ($@) in
+# the prequisite list. Knowing the target we can map to the corresponding
+# _SOURCES variable to learn what to build and link. Finally, make sure the
+# artifacts are output under the $(HOST_TRIPLE) directory to separate them from
+# objects intended for skiboot proper.
+#
+# [1] https://www.gnu.org/software/make/manual/html_node/Secondary-Expansion.html#Secondary-Expansion
+.SECONDEXPANSION:
+$(check_PROGRAMS) : $$(call target2prereq,$$@,$(HOST_TRIPLE)/)
+	$(call Q, HOSTCC , $(HOSTCC) $(HOSTCFLAGS) $(TEST_FLAGS) -Wno-suggest-attribute=const -O0 -g -o $@ $^, $@)
+
+.PHONY: libflash-test-clean
+libflash-test-clean: OBJ_STEMS = $(call objstem,$(check_PROGRAMS),$(HOST_TRIPLE)/)
+libflash-test-clean: libflash-test-gcov-clean
+	$(RM) $(check_PROGRAMS)
+	$(RM) $(OBJ_STEMS:%=%.o)
+	$(RM) $(OBJ_STEMS:%=%.d)
+
+# gcov support: Build objects under $(HOST_TRIPLE)/gcov/
+$(check_PROGRAMS:%=%-gcov-run) : %-run: %
+	$(call QTEST, TEST-COVERAGE ,$< , $<)
+
+$(HOST_TRIPLE)/gcov/%.o : %.c
+	@mkdir -p $(dir $@)
+	$(call Q, HOSTCC ,$(HOSTCC) $(HOSTCFLAGS) $(HOSTGCOVCFLAGS) $(TEST_FLAGS) -Wno-suggest-attribute=const -g -c -o $@ $<, $@)
+
+.SECONDEXPANSION:
+$(check_PROGRAMS:%=%-gcov) : $$(call target2prereq,$$(patsubst %-gcov,%,$$@),$(HOST_TRIPLE)/gcov/)
+	$(call Q, HOSTCC , $(HOSTCC) $(HOSTCFLAGS) $(HOSTGCOVCFLAGS) $(TEST_FLAGS) -Wno-suggest-attribute=const -O0 -g -o $@ $^, $@)
 
-libflash-test-clean:
-	$(RM) libflash/test/*.o $(LIBFLASH_TEST)
-	$(RM) libflash/test/*.d
-	$(RM) libflash/test/*-gcov
+.PHONY: libflash-test-gcov-clean
+libflash-test-gcov-clean: GCOV_OBJ_STEMS = $(call objstem,$(check_PROGRAMS),$(HOST_TRIPLE)/gcov/)
+libflash-test-gcov-clean:
+	$(RM) $(check_PROGRAMS:%=%-gcov)
+	$(RM) $(GCOV_OBJ_STEMS:%=%.o)
+	$(RM) $(GCOV_OBJ_STEMS:%=%.d)
+	$(RM) $(GCOV_OBJ_STEMS:%=%.gcda)
+	$(RM) $(GCOV_OBJ_STEMS:%=%.gcno)