Pedantic fixes to build system (configure, Makefile) - #8933
Conversation
39e6288 to
d8f5abf
Compare
| CFLAGS = $(CPPFLAGS) $(CWARNFLAGS) $(CDEBUGFLAGS) $(COPTFLAGS) -I $(CCANDIR) $(EXTERNAL_INCLUDE_FLAGS) -I . -I$(CPATH) $(SQLITE3_CFLAGS) $(SODIUM_CFLAGS) $(POSTGRES_INCLUDE) $(FEATURES) $(COVFLAGS) $(DEV_CFLAGS) -DSHACHAIN_BITS=48 -DJSMN_PARENT_LINKS $(PIE_CFLAGS) $(COMPAT_CFLAGS) $(CSANFLAGS) | ||
| # Put the environment-inherited flags *last* so the user has the final say. | ||
| CPPFLAGS := -DCLN_NEXT_VERSION="\"$(CLN_NEXT_VERSION)\"" -DPKGLIBEXECDIR="\"$(pkglibexecdir)\"" -DBINDIR="\"$(bindir)\"" -DPLUGINDIR="\"$(plugindir)\"" -DCCAN_TAL_NEVER_RETURN_NULL=1 -DSHACHAIN_BITS=48 -DJSMN_PARENT_LINKS $(CPPFLAGS) | ||
| CFLAGS := $(CWARNFLAGS) $(CDEBUGFLAGS) $(COPTFLAGS) -I $(CCANDIR) $(EXTERNAL_INCLUDE_FLAGS) -I . -I$(CPATH) $(SQLITE3_CFLAGS) $(SODIUM_CFLAGS) $(POSTGRES_INCLUDE) $(FEATURES) $(COVFLAGS) $(DEV_CFLAGS) $(PIE_CFLAGS) $(COMPAT_CFLAGS) $(CSANFLAGS) $(CFLAGS) |
There was a problem hiding this comment.
Isnt the old CFLAGS = $(CPPFLAGS) $(CWARNFLAGS) . . . included CPPFLAGS directly, and new CFLAGS := $(CWARNFLAGS) $(CDEBUGFLAGS) . . . $(CFLAGS) does not - CPPFLAGS now only reaches the compiler through $(COMPILE.c), which the pr only wires into the generic %.o: %.c pattern rule. But Makefile has some explicit rules like:
ccan-tal.o: $(CCANDIR)/ccan/tal/tal.c
@$(call VERBOSE, "cc $<", $(CC) $(CFLAGS) -c -o $@ $<)
And these still call $(CC) $(CFLAGS) directly?, so they now lose everything that moved into CPPFLAGS, plus any macOS Homebrew -I paths. The worst case is ccan-tal.o, and ccan/tal/tal.c has:
static void *null_alloc_failed(void)
{
#ifdef CCAN_TAL_NEVER_RETURN_NULL
abort();
#else
return NULL;
#endif
}
Without -DCCAN_TAL_NEVER_RETURN_NULL=1, an out of memory tal_alloc now returns NULL instead of aborting? - while the rest of the codebase (compiled through the fixed pattern rule) is written on the assumption tal never returns NULL?
Maybe we need to keep CPPFLAGS folded into CFLAGS (drop the split), or update every explicit compile rule to use $(COMPILE.c) or at minimum $(CC) $(CPPFLAGS) $(CFLAGS) instead of $(CC) $(CFLAGS)?
There was a problem hiding this comment.
Thanks. I missed that. I'm on vacation now, but I'll do a thorough review for other direct invocations of $(CC) after I return (in a few days). The correct usage is $(COMPILE.c) when $(CC) is being invoked to compile a C source file to produce an object file. There really isn't a common scenario in which $(CC) should be used in a Make recipe except perhaps when querying/testing for compiler features. When asking the compiler to do something, the more specific variables should be used.
| echo "Warning: dsymutil not found. Install Xcode Command Line Tools for better debug support." | ||
| fi | ||
| else | ||
| CDEBUGFLAGS=${CDEBUGFLAGS--std=gnu11 -g -fstack-protector-strong} |
There was a problem hiding this comment.
Should we add $CFLAGS to all of these invocations, or keep -std=gnu11 centralized in one flags variable that all of them already reference??
There was a problem hiding this comment.
Some build systems put the -std= option directly in CC (and CXX if applicable). I don't really like that, as some code assumes that CC and friends contain simple path names with no arguments. However, there isn't a great place to specify the -std= option. It needs to be passed to the preprocessor, as feature test macros are affected by the C/C++ standard in use, but naïvely adding the -std= option to CPPFLAGS is suboptimal because C++ code will need a different standard than C code. Some build systems work around this by introducing a CXXCPPFLAGS variable that is specific to the C++ preprocessor, but that's very non-standard. I usually make the compromise of locally overriding the value of CPPFLAGS just for C++ source files. This won't be an issue for CLN for the time being since it currently has no C++ source files.
It's not POSIX-compatible. Use printf instead. Changelog-None
Make predefines variables COMPILE.c and LINK.c, providing the default commands for compiling and linking C programs: COMPILE.c = $(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c LINK.c = $(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $(TARGET_ARCH) Use these variables where appropriate. A few points of interest: * Using $(LINK.o) to link a C program is not correct, as it does not pass $(CFLAGS) to the linker driver. Passing $(CFLAGS) may be necessary for correct operation. For instance, -m32 can be specified in CFLAGS to build for a 32-bit ABI on a 64-bit-native system, and -flto can be specified in CFLAGS to enable link-time optimization. The linker driver needs to be told both of these in order to produce correct output. * CFLAGS is not supposed to subsume CPPFLAGS. The latter are logically the flags for the C preprocessor, while the former are the flags for the C compiler. The standard COMPILE.c variable incorporates both sets of flags since it invokes both the preprocessor and the compiler with one command. The standard LINK.c variable also incorporates both since it can be used to preprocess, compile, and link a C program all in one shot. In its more typical usage (linking precompiled object files), the linker driver accepts but makes no use of any preprocessor flags supplied to it. * CFLAGS logically shouldn't include any -D (or -U) options, as those are meant for the preprocessor and not the compiler. It arguably shouldn't include any -I options either, but I didn't make that fix here. Changelog-None
It doesn't logically belong in CDEBUGFLAGS. Makefile now *prepends* its default CPPFLAGS and CFLAGS to the environment- supplied flags. This allows the user to override individual flags by setting these variables through configure, without disturbing all the rest of the flags that Makefile wants by default. Changelog-None
The code intends to pass "$DEFAULT_COPTFLAGS" and "$DEBUGBUILD" as arguments $1 and $4 to default_cwarnflags(), but it had mistakenly doubled the double- quotes, which would have caused the values of those variables to be subjected to word splitting after substitution. Remove the extra double-quote marks. Changelog-None
Changelog-None
7238aa2 to
b1aea32
Compare
Checklist
Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:
tools/lightning-downgrade(N/A)