Module Name: src
Committed By: rillig
Date: Mon Jun 19 12:53:57 UTC 2023
Modified Files:
src/usr.bin/make: cond.c make.h parse.c
src/usr.bin/make/unit-tests: directive-include-guard.exp
directive-include-guard.mk
Log Message:
make: if a makefile is protected by a guard, only include it once
"looks reasonable" sjg@
To generate a diff of this commit:
cvs rdiff -u -r1.346 -r1.347 src/usr.bin/make/cond.c
cvs rdiff -u -r1.321 -r1.322 src/usr.bin/make/make.h
cvs rdiff -u -r1.699 -r1.700 src/usr.bin/make/parse.c
cvs rdiff -u -r1.3 -r1.4 \
src/usr.bin/make/unit-tests/directive-include-guard.exp
cvs rdiff -u -r1.4 -r1.5 \
src/usr.bin/make/unit-tests/directive-include-guard.mk
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/usr.bin/make/cond.c
diff -u src/usr.bin/make/cond.c:1.346 src/usr.bin/make/cond.c:1.347
--- src/usr.bin/make/cond.c:1.346 Fri Jun 16 07:12:46 2023
+++ src/usr.bin/make/cond.c Mon Jun 19 12:53:57 2023
@@ -1,4 +1,4 @@
-/* $NetBSD: cond.c,v 1.346 2023/06/16 07:12:46 rillig Exp $ */
+/* $NetBSD: cond.c,v 1.347 2023/06/19 12:53:57 rillig Exp $ */
/*
* Copyright (c) 1988, 1989, 1990 The Regents of the University of California.
@@ -92,7 +92,7 @@
#include "dir.h"
/* "@(#)cond.c 8.2 (Berkeley) 1/2/94" */
-MAKE_RCSID("$NetBSD: cond.c,v 1.346 2023/06/16 07:12:46 rillig Exp $");
+MAKE_RCSID("$NetBSD: cond.c,v 1.347 2023/06/19 12:53:57 rillig Exp $");
/*
* Conditional expressions conform to this grammar:
@@ -1123,6 +1123,7 @@ Cond_EvalLine(const char *line)
/* Return state for previous conditional */
cond_depth--;
+ Parse_GuardEndif();
return cond_states[cond_depth] & IFS_ACTIVE
? CR_TRUE : CR_FALSE;
}
@@ -1150,6 +1151,7 @@ Cond_EvalLine(const char *line)
Parse_Error(PARSE_FATAL, "if-less else");
return CR_TRUE;
}
+ Parse_GuardElse();
state = cond_states[cond_depth];
if (state == IFS_INITIAL) {
@@ -1185,6 +1187,7 @@ Cond_EvalLine(const char *line)
Parse_Error(PARSE_FATAL, "if-less elif");
return CR_TRUE;
}
+ Parse_GuardElse();
state = cond_states[cond_depth];
if (state & IFS_SEEN_ELSE) {
Parse_Error(PARSE_WARNING, "extra elif");
@@ -1234,6 +1237,57 @@ Cond_EvalLine(const char *line)
return res;
}
+static bool
+skip_identifier(const char **pp)
+{
+ const char *p = *pp;
+
+ if (ch_isalpha(*p) || *p == '_') {
+ while (ch_isalnum(*p) || *p == '_')
+ p++;
+ *pp = p;
+ return true;
+ }
+ return false;
+}
+
+/*
+ * Tests whether the line is a conditional that forms a multiple-inclusion
+ * guard, and if so, extracts the guard variable name.
+ */
+char *
+Cond_ExtractGuard(const char *line)
+{
+ const char *p = line, *dir, *varname;
+ size_t dir_len;
+
+ if (!skip_string(&p, "."))
+ return NULL;
+ cpp_skip_hspace(&p);
+
+ dir = p;
+ while (ch_isalpha(*p))
+ p++;
+ dir_len = (size_t)(p - dir);
+ cpp_skip_hspace(&p);
+
+ if (dir_len == 2 && memcmp(dir, "if", 2) == 0) {
+ if (!skip_string(&p, "!defined("))
+ return NULL;
+ varname = p;
+ skip_identifier(&p);
+ if (p > varname && strcmp(p, ")") == 0)
+ return bmake_strsedup(varname, p);
+ }
+ if (dir_len == 6 && memcmp(dir, "ifndef", 6) == 0) {
+ varname = p;
+ skip_identifier(&p);
+ if (p > varname && *p == '\0')
+ return bmake_strsedup(varname, p);
+ }
+ return NULL;
+}
+
void
Cond_EndFile(void)
{
Index: src/usr.bin/make/make.h
diff -u src/usr.bin/make/make.h:1.321 src/usr.bin/make/make.h:1.322
--- src/usr.bin/make/make.h:1.321 Fri Jun 16 07:12:46 2023
+++ src/usr.bin/make/make.h Mon Jun 19 12:53:57 2023
@@ -1,4 +1,4 @@
-/* $NetBSD: make.h,v 1.321 2023/06/16 07:12:46 rillig Exp $ */
+/* $NetBSD: make.h,v 1.322 2023/06/19 12:53:57 rillig Exp $ */
/*
* Copyright (c) 1988, 1989, 1990, 1993
@@ -793,6 +793,7 @@ void Compat_Make(GNode *, GNode *);
extern unsigned int cond_depth;
CondResult Cond_EvalCondition(const char *) MAKE_ATTR_USE;
CondResult Cond_EvalLine(const char *) MAKE_ATTR_USE;
+char *Cond_ExtractGuard(const char *) MAKE_ATTR_USE;
void Cond_EndFile(void);
/* dir.c; see also dir.h */
@@ -857,6 +858,8 @@ void Parse_PushInput(const char *, unsig
void Parse_MainName(GNodeList *);
int Parse_NumErrors(void) MAKE_ATTR_USE;
unsigned int CurFile_CondMinDepth(void) MAKE_ATTR_USE;
+void Parse_GuardElse(void);
+void Parse_GuardEndif(void);
/* suff.c */
Index: src/usr.bin/make/parse.c
diff -u src/usr.bin/make/parse.c:1.699 src/usr.bin/make/parse.c:1.700
--- src/usr.bin/make/parse.c:1.699 Thu Jun 1 06:25:34 2023
+++ src/usr.bin/make/parse.c Mon Jun 19 12:53:57 2023
@@ -1,4 +1,4 @@
-/* $NetBSD: parse.c,v 1.699 2023/06/01 06:25:34 rillig Exp $ */
+/* $NetBSD: parse.c,v 1.700 2023/06/19 12:53:57 rillig Exp $ */
/*
* Copyright (c) 1988, 1989, 1990, 1993
@@ -105,7 +105,15 @@
#include "pathnames.h"
/* "@(#)parse.c 8.3 (Berkeley) 3/19/94" */
-MAKE_RCSID("$NetBSD: parse.c,v 1.699 2023/06/01 06:25:34 rillig Exp $");
+MAKE_RCSID("$NetBSD: parse.c,v 1.700 2023/06/19 12:53:57 rillig Exp $");
+
+/* Detects a multiple-inclusion guard in a makefile. */
+typedef enum {
+ GS_START, /* at the beginning of the file */
+ GS_COND, /* after the guard condition */
+ GS_DONE, /* after the closing .endif */
+ GS_NO /* the file is not guarded */
+} GuardState;
/*
* A file being read.
@@ -128,6 +136,9 @@ typedef struct IncludedFile {
char *buf_ptr; /* next char to be read */
char *buf_end; /* buf_end[-1] == '\n' */
+ GuardState guard;
+ char *guardVarname;
+
struct ForLoop *forLoop;
} IncludedFile;
@@ -299,6 +310,8 @@ static const struct {
enum PosixState posix_state = PS_NOT_YET;
+static HashTable guards;
+
static IncludedFile *
GetInclude(size_t i)
{
@@ -1212,6 +1225,7 @@ IncludeFile(const char *file, bool isSys
{
Buffer buf;
char *fullname; /* full pathname of file */
+ char *guardVarname;
int fd;
fullname = file[0] == '/' ? bmake_strdup(file) : NULL;
@@ -1231,6 +1245,14 @@ IncludeFile(const char *file, bool isSys
return;
}
+ guardVarname = HashTable_FindValue(&guards, fullname);
+ if (guardVarname != NULL
+ && GNode_ValueDirect(SCOPE_GLOBAL, guardVarname) != NULL) {
+ DEBUG2(PARSE, "Skipping '%s' because '%s' is already set\n",
+ fullname, guardVarname);
+ return;
+ }
+
if ((fd = open(fullname, O_RDONLY)) == -1) {
if (!silent)
Parse_Error(PARSE_FATAL, "Cannot open %s", fullname);
@@ -2174,6 +2196,8 @@ Parse_PushInput(const char *name, unsign
curFile->forBodyReadLines = readLines;
curFile->buf = buf;
curFile->depending = doing_depend; /* restore this on EOF */
+ curFile->guard = forLoop == NULL ? GS_START : GS_NO;
+ curFile->guardVarname = NULL;
curFile->forLoop = forLoop;
if (forLoop != NULL && !For_NextIteration(forLoop, &curFile->buf))
@@ -2316,8 +2340,15 @@ ParseEOF(void)
Cond_EndFile();
+ if (curFile->guard == GS_DONE) {
+ HashTable_Set(&guards,
+ curFile->name.str, curFile->guardVarname);
+ curFile->guardVarname = NULL;
+ }
+
FStr_Done(&curFile->name);
Buf_Done(&curFile->buf);
+ free(curFile->guardVarname);
if (curFile->forLoop != NULL)
ForLoop_Free(curFile->forLoop);
Vector_Pop(&includes);
@@ -2616,8 +2647,10 @@ static char *
ReadHighLevelLine(void)
{
char *line;
+ CondResult condResult;
for (;;) {
+ IncludedFile *curFile = CurFile();
line = ReadLowLevelLine(LK_NONEMPTY);
if (posix_state == PS_MAYBE_NEXT_LINE)
posix_state = PS_NOW_OR_NEVER;
@@ -2626,10 +2659,24 @@ ReadHighLevelLine(void)
if (line == NULL)
return NULL;
+ if (curFile->guard != GS_NO
+ && ((curFile->guard == GS_START && line[0] != '.')
+ || curFile->guard == GS_DONE))
+ curFile->guard = GS_NO;
if (line[0] != '.')
return line;
- switch (Cond_EvalLine(line)) {
+ condResult = Cond_EvalLine(line);
+ if (curFile->guard == GS_START) {
+ char *varname;
+ if (condResult == CR_TRUE
+ && (varname = Cond_ExtractGuard(line)) != NULL) {
+ curFile->guard = GS_COND;
+ curFile->guardVarname = varname;
+ } else
+ curFile->guard = GS_NO;
+ }
+ switch (condResult) {
case CR_FALSE: /* May also mean a syntax error. */
if (!SkipIrrelevantBranches())
return NULL;
@@ -2784,6 +2831,22 @@ Parse_VarAssign(const char *line, bool f
return true;
}
+void
+Parse_GuardElse(void)
+{
+ IncludedFile *curFile = CurFile();
+ if (cond_depth == curFile->condMinDepth + 1)
+ curFile->guard = GS_NO;
+}
+
+void
+Parse_GuardEndif(void)
+{
+ IncludedFile *curFile = CurFile();
+ if (cond_depth == curFile->condMinDepth && curFile->guard == GS_COND)
+ curFile->guard = GS_DONE;
+}
+
static char *
FindSemicolon(char *p)
{
@@ -2974,6 +3037,7 @@ Parse_Init(void)
sysIncPath = SearchPath_New();
defSysIncPath = SearchPath_New();
Vector_Init(&includes, sizeof(IncludedFile));
+ HashTable_Init(&guards);
}
/* Clean up the parsing module. */
@@ -2981,6 +3045,8 @@ void
Parse_End(void)
{
#ifdef CLEANUP
+ HashIter hi;
+
Lst_DoneCall(&targCmds, free);
assert(targets == NULL);
SearchPath_Free(defSysIncPath);
@@ -2988,6 +3054,10 @@ Parse_End(void)
SearchPath_Free(parseIncPath);
assert(includes.len == 0);
Vector_Done(&includes);
+ HashIter_Init(&hi, &guards);
+ while (HashIter_Next(&hi) != NULL)
+ free(hi.entry->value);
+ HashTable_Done(&guards);
#endif
}
Index: src/usr.bin/make/unit-tests/directive-include-guard.exp
diff -u src/usr.bin/make/unit-tests/directive-include-guard.exp:1.3 src/usr.bin/make/unit-tests/directive-include-guard.exp:1.4
--- src/usr.bin/make/unit-tests/directive-include-guard.exp:1.3 Sun Jun 18 20:43:52 2023
+++ src/usr.bin/make/unit-tests/directive-include-guard.exp Mon Jun 19 12:53:57 2023
@@ -1,19 +1,19 @@
Parse_PushInput: file guarded-ifndef.tmp, line 1
-Parse_PushInput: file guarded-ifndef.tmp, line 1
+Skipping 'guarded-ifndef.tmp' because 'GUARDED_IFNDEF' is already set
Parse_PushInput: file comments.tmp, line 1
-Parse_PushInput: file comments.tmp, line 1
-Parse_PushInput: file guarded-if.tmp, line 1
+Skipping 'comments.tmp' because 'COMMENTS' is already set
Parse_PushInput: file guarded-if.tmp, line 1
+Skipping 'guarded-if.tmp' because 'GUARDED_IF' is already set
Parse_PushInput: file triple-negation.tmp, line 1
Parse_PushInput: file triple-negation.tmp, line 1
Parse_PushInput: file varname-mismatch.tmp, line 1
Parse_PushInput: file varname-mismatch.tmp, line 1
Parse_PushInput: file varname-indirect.tmp, line 1
-Parse_PushInput: file varname-indirect.tmp, line 1
+Skipping 'varname-indirect.tmp' because 'VARNAME_INDIRECT' is already set
Parse_PushInput: file late-assignment.tmp, line 1
-Parse_PushInput: file late-assignment.tmp, line 1
-Parse_PushInput: file two-conditions.tmp, line 1
+Skipping 'late-assignment.tmp' because 'LATE_ASSIGNMENT' is already set
Parse_PushInput: file two-conditions.tmp, line 1
+Skipping 'two-conditions.tmp' because 'TWO_CONDITIONS' is already set
Parse_PushInput: file already-set.tmp, line 1
Parse_PushInput: file already-set.tmp, line 1
Parse_PushInput: file twice.tmp, line 1
@@ -31,5 +31,5 @@ Parse_PushInput: file if-elif.tmp, line
Parse_PushInput: file if-else.tmp, line 1
Parse_PushInput: file if-else.tmp, line 1
Parse_PushInput: file inner-if-elif-else.tmp, line 1
-Parse_PushInput: file inner-if-elif-else.tmp, line 1
+Skipping 'inner-if-elif-else.tmp' because 'INNER_IF_ELIF_ELSE' is already set
exit status 0
Index: src/usr.bin/make/unit-tests/directive-include-guard.mk
diff -u src/usr.bin/make/unit-tests/directive-include-guard.mk:1.4 src/usr.bin/make/unit-tests/directive-include-guard.mk:1.5
--- src/usr.bin/make/unit-tests/directive-include-guard.mk:1.4 Sun Jun 18 20:43:52 2023
+++ src/usr.bin/make/unit-tests/directive-include-guard.mk Mon Jun 19 12:53:57 2023
@@ -1,21 +1,20 @@
-# $NetBSD: directive-include-guard.mk,v 1.4 2023/06/18 20:43:52 rillig Exp $
+# $NetBSD: directive-include-guard.mk,v 1.5 2023/06/19 12:53:57 rillig Exp $
#
# Tests for multiple-inclusion guards in makefiles.
#
# A file that is guarded by a multiple-inclusion guard has the following form:
#
# .ifndef GUARD_NAME
-# GUARD_NAME= # any value
+# ...
+# GUARD_NAME= # any value, may also be empty
# ...
# .endif
#
-# When such a file is included for the second time, it has no effect, as all
-# its content is skipped.
+# When such a file is included later and the guard variable is set, including
+# the file has no effect, as all its content is skipped.
#
# See also:
# https://gcc.gnu.org/onlinedocs/cppinternals/Guard-Macros.html
-#
-# TODO: In these cases, do not include the file, to increase performance.
# This is the canonical form of a multiple-inclusion guard.
@@ -25,7 +24,7 @@ LINES.guarded-ifndef= \
'GUARDED_IFNDEF=' \
'.endif'
# expect: Parse_PushInput: file guarded-ifndef.tmp, line 1
-# expect: Parse_PushInput: file guarded-ifndef.tmp, line 1
+# expect: Skipping 'guarded-ifndef.tmp' because 'GUARDED_IFNDEF' is already set
# Comments and empty lines have no influence on the multiple-inclusion guard.
INCS+= comments
@@ -38,7 +37,7 @@ LINES.comments= \
'.endif' \
'\# comment'
# expect: Parse_PushInput: file comments.tmp, line 1
-# expect: Parse_PushInput: file comments.tmp, line 1
+# expect: Skipping 'comments.tmp' because 'COMMENTS' is already set
# An alternative form uses the 'defined' function. It is more verbose than
# the canonical form. There are other possible forms as well, such as with a
@@ -49,7 +48,7 @@ LINES.guarded-if= \
'GUARDED_IF=' \
'.endif'
# expect: Parse_PushInput: file guarded-if.tmp, line 1
-# expect: Parse_PushInput: file guarded-if.tmp, line 1
+# expect: Skipping 'guarded-if.tmp' because 'GUARDED_IF' is already set
# Triple negation is so uncommon that it's not recognized.
INCS+= triple-negation
@@ -69,23 +68,19 @@ LINES.varname-mismatch= \
# expect: Parse_PushInput: file varname-mismatch.tmp, line 1
# expect: Parse_PushInput: file varname-mismatch.tmp, line 1
-# The variable name in the assignment must only contain alphanumeric
-# characters and underscores, in particular, it must not be a dynamically
-# computed name.
+# The variable name in the guard condition must only contain alphanumeric
+# characters and underscores. The guard variable is more flexible, it can be
+# set anywhere, as long as it is set when the guarded file is included next.
INCS+= varname-indirect
LINES.varname-indirect= \
'.ifndef VARNAME_INDIRECT' \
'VARNAME_$${:UINDIRECT}=' \
'.endif'
# expect: Parse_PushInput: file varname-indirect.tmp, line 1
-# expect: Parse_PushInput: file varname-indirect.tmp, line 1
+# expect: Skipping 'varname-indirect.tmp' because 'VARNAME_INDIRECT' is already set
-# The variable assignment for the guard must directly follow the conditional.
-#
-# This requirement may be dropped entirely later, as the guard variable could
-# also be undefined while reading the file or at a later point, and as long as
-# the implementation checks the guard variable before skipping the file, the
-# optimization is still valid.
+# The time at which the guard variable is set doesn't matter, as long as it is
+# set when the file is included the next time.
INCS+= late-assignment
LINES.late-assignment= \
'.ifndef LATE_ASSIGNMENT' \
@@ -93,10 +88,10 @@ LINES.late-assignment= \
'LATE_ASSIGNMENT=' \
'.endif'
# expect: Parse_PushInput: file late-assignment.tmp, line 1
-# expect: Parse_PushInput: file late-assignment.tmp, line 1
+# expect: Skipping 'late-assignment.tmp' because 'LATE_ASSIGNMENT' is already set
-# There must be no other condition between the guard condition and the
-# variable assignment.
+# The time at which the guard variable is set doesn't matter, as long as it is
+# set when the file is included the next time.
INCS+= two-conditions
LINES.two-conditions= \
'.ifndef TWO_CONDITIONS' \
@@ -105,11 +100,11 @@ LINES.two-conditions= \
'. endif' \
'.endif'
# expect: Parse_PushInput: file two-conditions.tmp, line 1
-# expect: Parse_PushInput: file two-conditions.tmp, line 1
+# expect: Skipping 'two-conditions.tmp' because 'TWO_CONDITIONS' is already set
# If the guard variable is already set before the file is included for the
-# first time, that file is not considered to be guarded. It's a situation
-# that is uncommon in practice.
+# first time, the file is not considered guarded, as the makefile parser skips
+# all lines in the inactive part between the '.ifndef' and the '.endif'.
INCS+= already-set
LINES.already-set= \
'.ifndef ALREADY_SET' \
@@ -123,18 +118,20 @@ ALREADY_SET=
# several, even if they have the same effect.
INCS+= twice
LINES.twice= \
- '.ifndef TWICE' \
- 'TWICE=' \
+ '.ifndef TWICE_FIRST' \
+ 'TWICE_FIRST=' \
'.endif' \
- '.ifndef TWICE' \
- 'TWICE=' \
+ '.ifndef TWICE_SECOND' \
+ 'TWICE_SECOND=' \
'.endif'
# expect: Parse_PushInput: file twice.tmp, line 1
# expect: Parse_PushInput: file twice.tmp, line 1
# When multiple files use the same guard variable name, they exclude each
-# other. It's the responsibility of the makefile authors to choose suitable
-# variable names. Typical choices are ${PROJECT}_${DIR}_${FILE}_MK.
+# other. It's the responsibility of the makefile authors to choose unique
+# variable names. Typical choices are ${PROJECT}_${DIR}_${FILE}_MK. This is
+# the same situation as in the 'already-set' test, and the file is not
+# considered guarded.
INCS+= reuse
LINES.reuse= \
${LINES.guarded-if}
@@ -211,7 +208,8 @@ LINES.inner-if-elif-else = \
'. endif' \
'.endif'
# expect: Parse_PushInput: file inner-if-elif-else.tmp, line 1
-# expect: Parse_PushInput: file inner-if-elif-else.tmp, line 1
+# expect: Skipping 'inner-if-elif-else.tmp' because 'INNER_IF_ELIF_ELSE' is already set
+
# Include each of the files twice. The directive-include-guard.exp file
# contains a single entry for the files whose multiple-inclusion guard works,
