Makefile to build and test a C program

Question

Requirements:

• make shalt build read_file program in the root directory of the project from all the files in the src directory, and take care of all the dependencies, and place all .o and .d files in an obj directory (creating it if it doesn't exist).
• make valgrind shalt run the read_file program with all its options with the valgrind program.
• make benchmark shalt run the benchmark (a Bash script) and redirect its output to log.txt.
• make test shalt build all the tests sources in the test directory and place all executables in test/bin. It should take care of all the dependencies etc.
• make clean shalt remove the read_file program and obj directory created by make.
• make tclean shalt remove the test/bin directory created by make test.

Directory Structure:

.
├── benchmark
├── gen_files
├── LICENSE
├── Makefile
├── README.md
├── src
│   ├── common.c
│   ├── common.h
│   ├── FileBuf.h
│   ├── read_file.c
│   ├── readlines_fread.c
│   ├── readlines_fread.h
│   ├── readlines_getline.c
│   ├── readlines_getline.h
│   ├── readlines_mmap_getline.c
│   ├── readlines_mmap_getline.h
│   ├── readlines_mmap_memchr.c
│   ├── readlines_mmap_memchr.h
│   ├── readlines_read.c
│   └── readlines_read.h
└── test
    ├── acutest.h
    ├── readlines_fread_test_failure.c
    ├── readlines_fread_test_success.c
    ├── readlines_getline_test_failure.c
    ├── readlines_getline_test_success.c
    ├── readlines_mmap_getline_test_failure.c
    ├── readlines_mmap_getline_test_success.c
    ├── readlines_mmap_memchr_test_failure.c
    ├── readlines_mmap_memchr_test_success.c
    ├── readlines_read_test_failure.c
    ├── readlines_read_test_success.c
    ├── sample.txt
    └── stubs.c

2 directories, 33 files

Makefile:

CC = gcc-13

CFLAGS += -DBENCHMARKING

CFLAGS += -O3
CFLAGS += -std=gnu2x
CFLAGS += -s
CFLAGS += -no-pie

CFLAGS += -fno-builtin
CFLAGS += -fno-common
CFLAGS += -fno-omit-frame-pointer

CFLAGS += -Wall
CFLAGS += -Wextra
CFLAGS += -Warray-bounds
CFLAGS += -Wconversion
CFLAGS += -Wformat-signedness
CFLAGS += -Wno-parentheses
CFLAGS += -Wpedantic
CFLAGS += -Wstrict-prototypes
CFLAGS += -Wwrite-strings
CFLAGS += -Wno-missing-braces
CFLAGS += -Wno-missing-field-initializers

# CFLAGS += -fsanitize=address
# CFLAGS += -fsanitize=undefined
# CFLAGS += -fsanitize=bounds-strict
# CFLAGS += -fsanitize=leak
# CFLAGS += -fsanitize=null
# CFLAGS += -fsanitize=signed-integer-overflow
# CFLAGS += -fsanitize=bool
# CFLAGS += -fsanitize=pointer-overflow
# CFLAGS += -fsanitize-address-use-after-scope

# CFLAGS += -fanalyzer

TEST_CFLAGS = -std=gnu2x -O3 -s -Wall -Wextra

RM = /bin/rm -r
MKDIR = /bin/mkdir -p

SRC_DIR = src
SRCS = $(wildcard $(SRC_DIR)/*.c)
OBJ_DIR = obj
OBJS = $(patsubst $(SRC_DIR)/%.c,$(OBJ_DIR)/%.o,$(SRCS))
DEPS = $(OBJS:.o=.d)

TEST_SRC_DIR = test
TEST_SRCS = $(wildcard $(TEST_SRC_DIR)/*.c)
TEST_BIN_DIR = test/bin

TARGET = read_file

all: $(TARGET)
    
$(TARGET): $(OBJ_DIR) $(OBJS)
    $(CC) $(CFLAGS) -o $@ $(OBJS)

$(OBJ_DIR):
    $(MKDIR) $(OBJ_DIR)

$(OBJ_DIR)/%.o: $(SRC_DIR)/%.c
    $(CC) $(CFLAGS) -MMD -c -o $@ $<

-include $(DEPS)

valgrind: $(TARGET)
    valgrind --tool=memcheck --leak-check=yes ./read_file --mmap_memchr read_file
    valgrind --tool=memcheck --leak-check=yes ./read_file --getline read_file
    valgrind --tool=memcheck --leak-check=yes ./read_file --mmap_getline read_file
    valgrind --tool=memcheck --leak-check=yes ./read_file --fread read_file

benchmark: $(TARGET)
    ./benchmark > log.txt

$(TEST_BIN_DIR): $(OBJ_DIR) $(OBJS)
    $(MKDIR) $(TEST_BIN_DIR)

    $(CC) $(TEST_CFLAGS) -DFEOF $(OBJ_DIR)/common.o $(TEST_SRC_DIR)/readlines_fread_test_failure.c -o $(TEST_BIN_DIR)/readlines_fread_test_failure_feof
    $(CC) $(TEST_CFLAGS) -DRESIZE_FBUF $(OBJ_DIR)/common.o $(TEST_SRC_DIR)/readlines_fread_test_failure.c -o $(TEST_BIN_DIR)/readlines_fread_test_failure_resize_fbuf
    $(CC) $(TEST_CFLAGS) -DALLOC_AND_APPEND_LINE $(OBJ_DIR)/common.o $(TEST_SRC_DIR)/readlines_fread_test_failure.c -o $(TEST_BIN_DIR)/readlines_fread_test_failure_alloc_and_append_line
    $(CC) $(TEST_CFLAGS) -DWITH_FREAD $(OBJ_DIR)/common.o $(OBJ_DIR)/readlines_fread.o $(TEST_SRC_DIR)/readlines_all_test_success.c -o $(TEST_BIN_DIR)/readlines_fread_test_success

    $(CC) $(TEST_CFLAGS) -DGETLINE $(OBJ_DIR)/common.o $(TEST_SRC_DIR)/readlines_getline_test_failure.c -o $(TEST_BIN_DIR)/readlines_getline_test_failure_getline
    $(CC) $(TEST_CFLAGS) -DRESIZE_FBUF $(OBJ_DIR)/common.o $(TEST_SRC_DIR)/readlines_getline_test_failure.c -o $(TEST_BIN_DIR)/readlines_getline_test_failure_resize_fbuf
    $(CC) $(TEST_CFLAGS) -DAPPEND_LINE $(OBJ_DIR)/common.o $(TEST_SRC_DIR)/readlines_getline_test_failure.c -o $(TEST_BIN_DIR)/readlines_getline_test_failure_append_line
    $(CC) $(TEST_CFLAGS) -DWITH_GETLINE $(OBJ_DIR)/common.o $(OBJ_DIR)/readlines_getline.o $(TEST_SRC_DIR)/readlines_all_test_success.c -o $(TEST_BIN_DIR)/readlines_getline_test_success

    $(CC) $(TEST_CFLAGS) -DFSTAT $(OBJ_DIR)/common.o $(TEST_SRC_DIR)/readlines_mmap_memchr_test_failure.c -o $(TEST_BIN_DIR)/readlines_mmap_memchr_test_failure_fstat
    $(CC) $(TEST_CFLAGS) -DMMAP $(OBJ_DIR)/common.o $(TEST_SRC_DIR)/readlines_mmap_memchr_test_failure.c -o $(TEST_BIN_DIR)/readlines_mmap_memchr_test_failure_mmap
    $(CC) $(TEST_CFLAGS) -DRESIZE_FBUF $(OBJ_DIR)/common.o $(TEST_SRC_DIR)/readlines_mmap_memchr_test_failure.c -o $(TEST_BIN_DIR)/readlines_mmap_memchr_test_failure_resize_fbuf
    $(CC) $(TEST_CFLAGS) -DAPPEND_LINE $(OBJ_DIR)/common.o $(TEST_SRC_DIR)/readlines_mmap_memchr_test_failure.c -o $(TEST_BIN_DIR)/readlines_mmap_memchr_test_failure_append_line
    $(CC) $(TEST_CFLAGS) -DWITH_MMAP_MEMCHR $(OBJ_DIR)/common.o $(OBJ_DIR)/readlines_mmap_memchr.o $(TEST_SRC_DIR)/readlines_all_test_success.c -o $(TEST_BIN_DIR)/readlines_mmap_memchr_test_success

    $(CC) $(TEST_CFLAGS) -DFSTAT $(OBJ_DIR)/common.o $(TEST_SRC_DIR)/readlines_mmap_getline_test_failure.c -o $(TEST_BIN_DIR)/readlines_mmap_getline_test_failure_fstat
    $(CC) $(TEST_CFLAGS) -DMMAP $(OBJ_DIR)/common.o $(TEST_SRC_DIR)/readlines_mmap_getline_test_failure.c -o $(TEST_BIN_DIR)/readlines_mmap_getline_test_failure_mmap
    $(CC) $(TEST_CFLAGS) -DFMEMOPEN $(OBJ_DIR)/common.o $(TEST_SRC_DIR)/readlines_mmap_getline_test_failure.c -o $(TEST_BIN_DIR)/readlines_mmap_getline_test_failure_fmemopen
    $(CC) $(TEST_CFLAGS) -DGETLINE $(OBJ_DIR)/common.o $(TEST_SRC_DIR)/readlines_mmap_getline_test_failure.c -o $(TEST_BIN_DIR)/readlines_mmap_getline_test_failure_readlines_getline
    $(CC) $(TEST_CFLAGS) -DWITH_MMAP_GETLINE $(OBJ_DIR)/common.o $(OBJ_DIR)/readlines_getline.o $(OBJ_DIR)/readlines_mmap_getline.o $(TEST_SRC_DIR)/readlines_all_test_success.c -o $(TEST_BIN_DIR)/readlines_mmap_getline_test_success

    $(CC) $(TEST_CFLAGS) -DFILENO $(OBJ_DIR)/common.o $(TEST_SRC_DIR)/readlines_read_test_failure.c -o $(TEST_BIN_DIR)/readlines_read_test_failure_fstat
    $(CC) $(TEST_CFLAGS) -DREAD $(OBJ_DIR)/common.o $(TEST_SRC_DIR)/readlines_read_test_failure.c -o $(TEST_BIN_DIR)/readlines_read_test_failure_mmap
    $(CC) $(TEST_CFLAGS) -DRESIZE_FBUF $(OBJ_DIR)/common.o $(TEST_SRC_DIR)/readlines_read_test_failure.c -o $(TEST_BIN_DIR)/readlines_read_test_failure_resize_fbuf
    $(CC) $(TEST_CFLAGS) -DALLOC_AND_APPEND_LINE $(OBJ_DIR)/common.o $(TEST_SRC_DIR)/readlines_read_test_failure.c -o $(TEST_BIN_DIR)/readlines_read_test_failure_append_line
    $(CC) $(TEST_CFLAGS) -DWITH_READ $(OBJ_DIR)/common.o $(OBJ_DIR)/readlines_read.o $(TEST_SRC_DIR)/readlines_all_test_success.c -o $(TEST_BIN_DIR)/readlines_read_test_success

test: $(TEST_BIN_DIR) $(OBJS) $(TEST_SRCS)
    @for test in $(wildcard $(TEST_BIN_DIR)/*); do \
        echo "Running $$test";  \
    ./$$test;   \
    done

clean:
    $(RM) $(TARGET) $(OBJ_DIR)

tclean:
    $(RM) $(TEST_BIN_DIR)

.PHONY: all valgrind benchmark test clean tclean
.DELETE_ON_ERROR:

I have achieved most of that, except for the correct dependencies for the test sources. Currently, any change any to the test sources is not detected by the make. Moreover, if any source file in the src directory is changed, all of the sources in the test directory are rebuilt.

(There's also the problem that I can't link the test executable with obj/common.o and obj/readlines_fread.o et cetera because the test sources (all having "failure" in their name) would not be able to detect the stubs that the macros with -D are defining, but that's for another day perhaps).

Link to github repository: cread.

Review Request:

How do I reduce the duplication that the test target has, whilst correctly specifying the dependencies?

Any simplifications I can make?

Answer 1

specification

make shalt build ...

Writing down requirements in English can be very helpful, keep doing it. But please spell it "shall". In KJV it appears fifteen hundred times, in fourteen hundred of them next to "thou". But "thou" is going to look a little funny in a modern spec.

conventional targets

make tclean

This is a pretty OK target name, you can certainly keep it.

Consider adopting a more broadly used target name seen in other packages, such as clean-all or very-clean. Then even without consulting documentation, a maintenance engineer will immediately understand that this is "more destructive" and should take care, perhaps should even consult the docs or the source or use make -n, before blindly plunging ahead with it.

OTOH these five PHONYs are perfect:

• all
• valgrind
• benchmark
• test
• clean

standard

CFLAGS += -std=gnu2x

I know, we're using GCC. And maybe there is a good reason for needing this. But for new projects you might consider habitually asking for ISO compliance, e.g. c17 or c2x. Go with it till you hit a speedbump that makes gnu2x look attractive. And then you'll be able to write down a comment for future maintainers explaining which GNU feature we're relying on.

Generally the "pedantic plus others" looks great. It's unclear to me why TEST_CFLAGS drops most of them. And I assume there was some "sanitize" trouble.

surprising switches

RM = /bin/rm -r
MKDIR = /bin/mkdir -p
...
clean:
    $(RM) $(TARGET) $(OBJ_DIR)

Please don't do that.

I suppose the -p is harmless enough. But that -r is a land mine waiting to go off. This is a POLA violation. Down at the call site (the clean recipe) we should be explicitly supplying a -r recursive argument, for clarity to anyone working with this. Hiding it way at the top gives misleading signals to someone reading the recipe.

OTOH you might consider putting repeated flags into VALGRIND or VALGRIND_FLAGS.

Also, please don't hardcode full pathnames; prefer to use $PATH. On my ubuntu jammy server, it happens that /bin is symlinked down into /usr/bin. But there are lots of machines where /bin is on the root filesystem so it's available during fsck before /usr has been mounted. There's just no reason to include a full pathname.

propagate the dep

Rather than hardcoding the binary name in a bash script

benchmark: $(TARGET)
    ./benchmark > log.txt

you might consider writing ./benchmark $(TARGET) > log.txt, and then have bash pick it out from argv.

test deps

reduce the duplication that the test target has

Yeah, I agree, that does look kind of nightmarish.

First thought on this is to simply throw *.o into a test.a archive. Including common.o. And then build executable binaries as needed.

But then we have the {FEOF, RESIZE_FBUF, ALLOC_AND_APPEND_LINE} stuff to worry about. Maybe that doesn't quite belong here? Maybe deal with it in .c and .h source code? And then the Makefile need only worry about these objects:

• readlines_fread_test_failure_feof.o
• readlines_fread_test_failure_resize_fbuf.o
• readlines_fread_test_failure_alloc_and_append_line.o

It would be simple to write rules to build the associated executables.

I'm thinking we could have three very short *.c files for those three variants, which simply #define and then #include the bulk of the source.

Flags like ALLOC_AND_APPEND_LINE appear repeatedly. Trading efficiency for simplicity, would it perhaps make any sense to gather up a list of all flags, and rebuild everything once per flag, re-using a single executable name? Ok, prolly not, just a thought. More realistically, perhaps define five option lists with 3 to 4 -D defs in each, and loop over those lists to produce five test run results.

Answer 2

Perhaps rename to GNUmakefile:

OBJS = $(patsubst $(SRC_DIR)/%.c,$(OBJ_DIR)/%.o,$(SRCS))

The BSDs do not recognize this pattern rule. So the Makefile is specific to GNU make, and would not be understood by other versions of make. The Linux man page for make suggests naming the makefile GNUmakefile (which is also the first name searched) in such a case. As J_H mentions in the comments, this is not a big deal.

Do not hardcode the executable name:

As J_H suggested in his answer for the benchmark target, use $(TARGET) for the valgrind target instead of hardcoding read_file in 8 places:

valgrind: $(TARGET)
    valgrind --tool=memcheck --leak-check=yes ./$(TARGET) --mmap_memchr $(TARGET)
    valgrind --tool=memcheck --leak-check=yes ./$(TARGET) --getline $(TARGET)
    valgrind --tool=memcheck --leak-check=yes ./$(TARGET) --mmap_getline $(TARGET)
    valgrind --tool=memcheck --leak-check=yes ./$(TARGET) --fread $(TARGET)

Moreover, OP seems to be missing a run with the --read argument to the program being tested.

Sanitizers and analyzers do not make sense for benchmarking:

Yes, keep them in debug mode, but they would negatively impact the benchmarks if code is compiled with them. Consider:

• Having separate debug and release targets.
• Only enabling the sanitizers and analyzers in debug build.
• Having the valgrind target unconditionally build the target executable, as the previous build may have been a debug build, in which case the analyzers and sanitizers would be present, and they would not work with Valgrind.

This approach also eliminates the need to comment and uncomment the CFLAGS for the analyzers/sanitizers repeatedly during builds with run clean.

Prefer order-only prerequisites:

As vnp mentions in the post's comments:

-p, AFAICT serves the only purpose, namely to silence mkdir when the target directory exists. IMHO, an order-only prerequisite is cleaner.

Currently, we rebuild all the object files whenever the obj directory’s timestamp changes, which we certainly do not want. Note that the timestamps on directories change whenever a file is added, removed, or renamed. Order-only prerequisites are never checked when determining if the target is out of date.