From b289a7c0b8a0b8de2fed3fa10a04774c9d48f08e Mon Sep 17 00:00:00 2001
From: rillig <rillig@NetBSD.org>
Date: Wed, 23 Sep 2020 07:50:58 +0000
Subject: [PATCH] make(1): fix unexpected behavior in ::= variable modifier

Previously, the ::= modifier had returned an error value, which caused
the variable expression to be preserved.  This behavior was not useful
in this case; it had only been intended to be used for undefined
variables.

To fix it, distinguish between parse errors, undefined variables and
regular empty strings.
---
 usr.bin/make/unit-tests/cond-func-empty.mk |  4 +-
 usr.bin/make/unit-tests/counter-append.exp | 33 +++-------
 usr.bin/make/unit-tests/counter-append.mk  | 16 ++---
 usr.bin/make/unit-tests/counter.exp        | 36 +++--------
 usr.bin/make/unit-tests/counter.mk         | 16 ++---
 usr.bin/make/var.c                         | 72 +++++++++++-----------
 6 files changed, 66 insertions(+), 111 deletions(-)

diff --git a/usr.bin/make/unit-tests/cond-func-empty.mk b/usr.bin/make/unit-tests/cond-func-empty.mk
index dd392fd472ec..bf137d46e3f5 100644
--- a/usr.bin/make/unit-tests/cond-func-empty.mk
+++ b/usr.bin/make/unit-tests/cond-func-empty.mk
@@ -1,4 +1,4 @@
-# $NetBSD: cond-func-empty.mk,v 1.6 2020/09/04 21:08:44 rillig Exp $
+# $NetBSD: cond-func-empty.mk,v 1.7 2020/09/23 07:50:58 rillig Exp $
 #
 # Tests for the empty() function in .if conditions, which tests a variable
 # expression for emptiness.
@@ -116,7 +116,7 @@ ${:U }=	space
 .endif
 
 # The expression ${} for a variable with the empty name always evaluates
-# to an empty string (see Var_Parse, varNoError).
+# to an empty string (see Var_Parse, varUndefined).
 .if !empty()
 .  error
 .endif
diff --git a/usr.bin/make/unit-tests/counter-append.exp b/usr.bin/make/unit-tests/counter-append.exp
index d3764090f524..81c209ee9932 100644
--- a/usr.bin/make/unit-tests/counter-append.exp
+++ b/usr.bin/make/unit-tests/counter-append.exp
@@ -12,7 +12,7 @@ Var_Parse: ${COUNTER:[#]} with VARE_WANTRES|VARE_ASSIGN
 Applying ${COUNTER:[...} to " a" (VARE_WANTRES|VARE_ASSIGN, none, none)
 Modifier part: "#"
 Result of ${COUNTER:[#]} is "1" (VARE_WANTRES|VARE_ASSIGN, none, none)
-Global:A = ${COUNTER::+=a}1
+Global:A = 1
 Global:B = 
 Var_Parse: ${NEXT} with VARE_WANTRES|VARE_ASSIGN
 Var_Parse: ${COUNTER::+=a}${COUNTER:[#]} with VARE_WANTRES|VARE_ASSIGN
@@ -24,7 +24,7 @@ Var_Parse: ${COUNTER:[#]} with VARE_WANTRES|VARE_ASSIGN
 Applying ${COUNTER:[...} to " a a" (VARE_WANTRES|VARE_ASSIGN, none, none)
 Modifier part: "#"
 Result of ${COUNTER:[#]} is "2" (VARE_WANTRES|VARE_ASSIGN, none, none)
-Global:B = ${COUNTER::+=a}2
+Global:B = 2
 Global:C = 
 Var_Parse: ${NEXT} with VARE_WANTRES|VARE_ASSIGN
 Var_Parse: ${COUNTER::+=a}${COUNTER:[#]} with VARE_WANTRES|VARE_ASSIGN
@@ -36,45 +36,30 @@ Var_Parse: ${COUNTER:[#]} with VARE_WANTRES|VARE_ASSIGN
 Applying ${COUNTER:[...} to " a a a" (VARE_WANTRES|VARE_ASSIGN, none, none)
 Modifier part: "#"
 Result of ${COUNTER:[#]} is "3" (VARE_WANTRES|VARE_ASSIGN, none, none)
-Global:C = ${COUNTER::+=a}3
+Global:C = 3
 Global:RELEVANT = no
 Global:RELEVANT = yes (run-time part)
 Result of ${RELEVANT::=yes (run-time part)} is "" (VARE_WANTRES, none, none)
 Var_Parse: ${A:Q} B=${B:Q} C=${C:Q} COUNTER=${COUNTER:[#]:Q} with VARE_WANTRES
-Var_Parse: ${COUNTER::+=a}1 with VARE_WANTRES
-Applying ${COUNTER::...} to " a a a" (VARE_WANTRES, none, none)
-Modifier part: "a"
-Global:COUNTER =  a a a a
-Result of ${COUNTER::+=a} is "" (VARE_WANTRES, none, none)
 Applying ${A:Q} to "1" (VARE_WANTRES, none, none)
 QuoteMeta: [1]
 Result of ${A:Q} is "1" (VARE_WANTRES, none, none)
 Var_Parse: ${B:Q} C=${C:Q} COUNTER=${COUNTER:[#]:Q} with VARE_WANTRES
-Var_Parse: ${COUNTER::+=a}2 with VARE_WANTRES
-Applying ${COUNTER::...} to " a a a a" (VARE_WANTRES, none, none)
-Modifier part: "a"
-Global:COUNTER =  a a a a a
-Result of ${COUNTER::+=a} is "" (VARE_WANTRES, none, none)
 Applying ${B:Q} to "2" (VARE_WANTRES, none, none)
 QuoteMeta: [2]
 Result of ${B:Q} is "2" (VARE_WANTRES, none, none)
 Var_Parse: ${C:Q} COUNTER=${COUNTER:[#]:Q} with VARE_WANTRES
-Var_Parse: ${COUNTER::+=a}3 with VARE_WANTRES
-Applying ${COUNTER::...} to " a a a a a" (VARE_WANTRES, none, none)
-Modifier part: "a"
-Global:COUNTER =  a a a a a a
-Result of ${COUNTER::+=a} is "" (VARE_WANTRES, none, none)
 Applying ${C:Q} to "3" (VARE_WANTRES, none, none)
 QuoteMeta: [3]
 Result of ${C:Q} is "3" (VARE_WANTRES, none, none)
 Var_Parse: ${COUNTER:[#]:Q} with VARE_WANTRES
-Applying ${COUNTER:[...} to " a a a a a a" (VARE_WANTRES, none, none)
+Applying ${COUNTER:[...} to " a a a" (VARE_WANTRES, none, none)
 Modifier part: "#"
-Result of ${COUNTER:[#]} is "6" (VARE_WANTRES, none, none)
-Applying ${COUNTER:Q} to "6" (VARE_WANTRES, none, none)
-QuoteMeta: [6]
-Result of ${COUNTER:Q} is "6" (VARE_WANTRES, none, none)
-A=1 B=2 C=3 COUNTER=6
+Result of ${COUNTER:[#]} is "3" (VARE_WANTRES, none, none)
+Applying ${COUNTER:Q} to "3" (VARE_WANTRES, none, none)
+QuoteMeta: [3]
+Result of ${COUNTER:Q} is "3" (VARE_WANTRES, none, none)
+A=1 B=2 C=3 COUNTER=3
 Var_Parse: ${RELEVANT::=no} with VARE_WANTRES
 Applying ${RELEVANT::...} to "yes (run-time part)" (VARE_WANTRES, none, none)
 Modifier part: "no"
diff --git a/usr.bin/make/unit-tests/counter-append.mk b/usr.bin/make/unit-tests/counter-append.mk
index c988e4535705..08b314029d04 100644
--- a/usr.bin/make/unit-tests/counter-append.mk
+++ b/usr.bin/make/unit-tests/counter-append.mk
@@ -1,16 +1,10 @@
-# $NetBSD: counter-append.mk,v 1.1 2020/09/23 03:33:55 rillig Exp $
+# $NetBSD: counter-append.mk,v 1.2 2020/09/23 07:50:58 rillig Exp $
 #
-# Demonstrates that it is not easily possible to let make count
-# the number of times a variable is actually accessed, using the
-# ::+= variable modifier.
+# Demonstrates how to let make count the number of times a variable
+# is actually accessed, using the ::+= variable modifier.
 #
-# As of 2020-09-23, the counter ends up at having 6 words, even
-# though the NEXT variable is only accessed 3 times.  This is
-# surprising.
-#
-# A hint to this surprising behavior is that the variables don't
-# get fully expanded.  For example, A does not simply contain the
-# value "1" but an additional unexpanded ${COUNTER:...} before it.
+# This works since 2020-09-23.  Before that, the counter ended up at having
+# 6 words, even though the NEXT variable was only accessed 3 times.
 
 .MAKEFLAGS: -dv
 
diff --git a/usr.bin/make/unit-tests/counter.exp b/usr.bin/make/unit-tests/counter.exp
index 3c2203339058..95ba605cd61a 100644
--- a/usr.bin/make/unit-tests/counter.exp
+++ b/usr.bin/make/unit-tests/counter.exp
@@ -9,12 +9,11 @@ Var_Parse: ${COUNTER} a}${COUNTER:[#]} with VARE_WANTRES
 Modifier part: " a"
 Global:COUNTER =  a
 Result of ${COUNTER::=${COUNTER} a} is "" (VARE_WANTRES|VARE_ASSIGN, none, none)
-Var_Parse: ${COUNTER} a}${COUNTER:[#]} with VARE_WANTRES|VARE_ASSIGN
 Var_Parse: ${COUNTER:[#]} with VARE_WANTRES|VARE_ASSIGN
 Applying ${COUNTER:[...} to " a" (VARE_WANTRES|VARE_ASSIGN, none, none)
 Modifier part: "#"
 Result of ${COUNTER:[#]} is "1" (VARE_WANTRES|VARE_ASSIGN, none, none)
-Global:A = ${COUNTER::= a a}1
+Global:A = 1
 Global:B = 
 Var_Parse: ${NEXT} with VARE_WANTRES|VARE_ASSIGN
 Var_Parse: ${COUNTER::=${COUNTER} a}${COUNTER:[#]} with VARE_WANTRES|VARE_ASSIGN
@@ -23,12 +22,11 @@ Var_Parse: ${COUNTER} a}${COUNTER:[#]} with VARE_WANTRES
 Modifier part: " a a"
 Global:COUNTER =  a a
 Result of ${COUNTER::=${COUNTER} a} is "" (VARE_WANTRES|VARE_ASSIGN, none, none)
-Var_Parse: ${COUNTER} a}${COUNTER:[#]} with VARE_WANTRES|VARE_ASSIGN
 Var_Parse: ${COUNTER:[#]} with VARE_WANTRES|VARE_ASSIGN
 Applying ${COUNTER:[...} to " a a" (VARE_WANTRES|VARE_ASSIGN, none, none)
 Modifier part: "#"
 Result of ${COUNTER:[#]} is "2" (VARE_WANTRES|VARE_ASSIGN, none, none)
-Global:B = ${COUNTER::= a a a}2
+Global:B = 2
 Global:C = 
 Var_Parse: ${NEXT} with VARE_WANTRES|VARE_ASSIGN
 Var_Parse: ${COUNTER::=${COUNTER} a}${COUNTER:[#]} with VARE_WANTRES|VARE_ASSIGN
@@ -37,50 +35,34 @@ Var_Parse: ${COUNTER} a}${COUNTER:[#]} with VARE_WANTRES
 Modifier part: " a a a"
 Global:COUNTER =  a a a
 Result of ${COUNTER::=${COUNTER} a} is "" (VARE_WANTRES|VARE_ASSIGN, none, none)
-Var_Parse: ${COUNTER} a}${COUNTER:[#]} with VARE_WANTRES|VARE_ASSIGN
 Var_Parse: ${COUNTER:[#]} with VARE_WANTRES|VARE_ASSIGN
 Applying ${COUNTER:[...} to " a a a" (VARE_WANTRES|VARE_ASSIGN, none, none)
 Modifier part: "#"
 Result of ${COUNTER:[#]} is "3" (VARE_WANTRES|VARE_ASSIGN, none, none)
-Global:C = ${COUNTER::= a a a a}3
+Global:C = 3
 Global:RELEVANT = no
 Global:RELEVANT = yes (run-time part)
 Result of ${RELEVANT::=yes (run-time part)} is "" (VARE_WANTRES, none, none)
 Var_Parse: ${A:Q} B=${B:Q} C=${C:Q} COUNTER=${COUNTER:[#]:Q} with VARE_WANTRES
-Var_Parse: ${COUNTER::= a a}1 with VARE_WANTRES
-Applying ${COUNTER::...} to " a a a" (VARE_WANTRES, none, none)
-Modifier part: " a a"
-Global:COUNTER =  a a
-Result of ${COUNTER::= a a} is "" (VARE_WANTRES, none, none)
 Applying ${A:Q} to "1" (VARE_WANTRES, none, none)
 QuoteMeta: [1]
 Result of ${A:Q} is "1" (VARE_WANTRES, none, none)
 Var_Parse: ${B:Q} C=${C:Q} COUNTER=${COUNTER:[#]:Q} with VARE_WANTRES
-Var_Parse: ${COUNTER::= a a a}2 with VARE_WANTRES
-Applying ${COUNTER::...} to " a a" (VARE_WANTRES, none, none)
-Modifier part: " a a a"
-Global:COUNTER =  a a a
-Result of ${COUNTER::= a a a} is "" (VARE_WANTRES, none, none)
 Applying ${B:Q} to "2" (VARE_WANTRES, none, none)
 QuoteMeta: [2]
 Result of ${B:Q} is "2" (VARE_WANTRES, none, none)
 Var_Parse: ${C:Q} COUNTER=${COUNTER:[#]:Q} with VARE_WANTRES
-Var_Parse: ${COUNTER::= a a a a}3 with VARE_WANTRES
-Applying ${COUNTER::...} to " a a a" (VARE_WANTRES, none, none)
-Modifier part: " a a a a"
-Global:COUNTER =  a a a a
-Result of ${COUNTER::= a a a a} is "" (VARE_WANTRES, none, none)
 Applying ${C:Q} to "3" (VARE_WANTRES, none, none)
 QuoteMeta: [3]
 Result of ${C:Q} is "3" (VARE_WANTRES, none, none)
 Var_Parse: ${COUNTER:[#]:Q} with VARE_WANTRES
-Applying ${COUNTER:[...} to " a a a a" (VARE_WANTRES, none, none)
+Applying ${COUNTER:[...} to " a a a" (VARE_WANTRES, none, none)
 Modifier part: "#"
-Result of ${COUNTER:[#]} is "4" (VARE_WANTRES, none, none)
-Applying ${COUNTER:Q} to "4" (VARE_WANTRES, none, none)
-QuoteMeta: [4]
-Result of ${COUNTER:Q} is "4" (VARE_WANTRES, none, none)
-A=1 B=2 C=3 COUNTER=4
+Result of ${COUNTER:[#]} is "3" (VARE_WANTRES, none, none)
+Applying ${COUNTER:Q} to "3" (VARE_WANTRES, none, none)
+QuoteMeta: [3]
+Result of ${COUNTER:Q} is "3" (VARE_WANTRES, none, none)
+A=1 B=2 C=3 COUNTER=3
 Var_Parse: ${RELEVANT::=no} with VARE_WANTRES
 Applying ${RELEVANT::...} to "yes (run-time part)" (VARE_WANTRES, none, none)
 Modifier part: "no"
diff --git a/usr.bin/make/unit-tests/counter.mk b/usr.bin/make/unit-tests/counter.mk
index 47d586842052..7d8b6355464a 100644
--- a/usr.bin/make/unit-tests/counter.mk
+++ b/usr.bin/make/unit-tests/counter.mk
@@ -1,16 +1,10 @@
-# $NetBSD: counter.mk,v 1.2 2020/09/23 03:33:55 rillig Exp $
+# $NetBSD: counter.mk,v 1.3 2020/09/23 07:50:58 rillig Exp $
 #
-# Demonstrates that it is not easily possible to let make count
-# the number of times a variable is actually accessed, using the
-# ::= variable modifier.
+# Demonstrates how to let make count the number of times a variable
+# is actually accessed, using the ::= variable modifier.
 #
-# As of 2020-08-02, the counter ends up at having 4 words, even
-# though the NEXT variable is only accessed 3 times.  This is
-# surprising.
-#
-# A hint to this surprising behavior is that the variables don't
-# get fully expanded.  For example, A does not simply contain the
-# value "1" but an additional unexpanded ${COUNTER:...} before it.
+# This works since 2020-09-23.  Before that, the counter ended up at having
+# 4 words, even though the NEXT variable was only accessed 3 times.
 
 .MAKEFLAGS: -dv
 
diff --git a/usr.bin/make/var.c b/usr.bin/make/var.c
index 7ba9fdfebdb6..d0d8e19e49c1 100644
--- a/usr.bin/make/var.c
+++ b/usr.bin/make/var.c
@@ -1,4 +1,4 @@
-/*	$NetBSD: var.c,v 1.535 2020/09/23 04:27:39 rillig Exp $	*/
+/*	$NetBSD: var.c,v 1.536 2020/09/23 07:50:58 rillig Exp $	*/
 
 /*
  * Copyright (c) 1988, 1989, 1990, 1993
@@ -121,7 +121,7 @@
 #include    "metachar.h"
 
 /*	"@(#)var.c	8.3 (Berkeley) 3/19/94" */
-MAKE_RCSID("$NetBSD: var.c,v 1.535 2020/09/23 04:27:39 rillig Exp $");
+MAKE_RCSID("$NetBSD: var.c,v 1.536 2020/09/23 07:50:58 rillig Exp $");
 
 #define VAR_DEBUG_IF(cond, fmt, ...)	\
     if (!(DEBUG(VAR) && (cond)))	\
@@ -140,21 +140,21 @@ ENUM_FLAGS_RTTI_3(VarEvalFlags,
  */
 char **savedEnv = NULL;
 
-/*
- * This is a harmless return value for Var_Parse that can be used by Var_Subst
- * to determine if there was an error in parsing -- easier than returning
- * a flag, as things outside this module don't give a hoot.
- */
+/* Special return value for Var_Parse, indicating a parse error.  It may be
+ * caused by an undefined variable, a syntax error in a modifier or
+ * something entirely different. */
 char var_Error[] = "";
 
-/*
- * Similar to var_Error, but returned when the 'VARE_UNDEFERR' flag for
- * Var_Parse is not set.
- *
- * Why not just use a constant? Well, GCC likes to condense identical string
- * instances...
- */
-static char varNoError[] = "";
+/* Special return value for Var_Parse, indicating an undefined variable in
+ * a case where VARE_UNDEFERR is not set.  This undefined variable is
+ * typically a dynamic variable such as ${.TARGET}, whose expansion needs to
+ * be deferred until it is defined in an actual target. */
+static char varUndefined[] = "";
+
+/* Special return value for Var_Parse, just to avoid allocating empty strings.
+ * In contrast to var_Error and varUndefined, this is not an error marker but
+ * just an ordinary successful return value. */
+static char emptyString[] = "";
 
 /*
  * Traditionally we consume $$ during := like any other expansion.
@@ -2171,7 +2171,7 @@ ApplyModifier_ShellCommand(const char **pp, ApplyModifiersState *st)
     if (st->eflags & VARE_WANTRES)
 	st->newVal = Cmd_Exec(cmd, &errfmt);
     else
-	st->newVal = varNoError;
+	st->newVal = emptyString;
     free(cmd);
 
     if (errfmt != NULL)
@@ -2872,9 +2872,7 @@ ApplyModifier_Assign(const char **pp, ApplyModifiersState *st)
 	}
     }
     free(val);
-    st->newVal = varNoError;	/* XXX: varNoError is kind of an error,
-				 * the intention here is to just return
-				 * an empty string. */
+    st->newVal = emptyString;
     return AMR_OK;
 }
 
@@ -3040,7 +3038,7 @@ ApplyModifiers(
 		st.val = ApplyModifiers(&rval_pp, st.val, '\0', '\0', v,
 					exprFlags, ctxt, eflags, freePtr);
 		if (st.val == var_Error
-		    || (st.val == varNoError && !(st.eflags & VARE_UNDEFERR))
+		    || (st.val == varUndefined && !(st.eflags & VARE_UNDEFERR))
 		    || *rval_pp != '\0') {
 		    free(freeIt);
 		    goto out;	/* error already reported */
@@ -3181,7 +3179,7 @@ ApplyModifiers(
 		    if (errfmt)
 			Error(errfmt, st.val);
 		} else
-		    st.newVal = varNoError;
+		    st.newVal = emptyString;
 		p += 2;
 		res = AMR_OK;
 	    } else
@@ -3234,7 +3232,8 @@ ApplyModifiers(
 		*freePtr = NULL;
 	    }
 	    st.val = st.newVal;
-	    if (st.val != var_Error && st.val != varNoError) {
+	    if (st.val != var_Error && st.val != varUndefined &&
+		st.val != emptyString) {
 		*freePtr = st.val;
 	    }
 	}
@@ -3249,7 +3248,7 @@ ApplyModifiers(
     }
 out:
     *pp = p;
-    assert(st.val != NULL);	/* Use var_Error or varNoError instead. */
+    assert(st.val != NULL);	/* Use var_Error or varUndefined instead. */
     *exprFlags = st.exprFlags;
     return st.val;
 
@@ -3327,7 +3326,7 @@ ShortVarValue(char varname, const GNode *ctxt, VarEvalFlags eflags)
 	    return "$(.ARCHIVE)";
 	}
     }
-    return eflags & VARE_UNDEFERR ? var_Error : varNoError;
+    return eflags & VARE_UNDEFERR ? var_Error : varUndefined;
 }
 
 /* Parse a variable name, until the end character or a colon, whichever
@@ -3420,20 +3419,21 @@ ValidShortVarname(char varname, const char *start)
  *
  * Results:
  *	Returns the value of the variable expression, never NULL.
- *	var_Error if there was a parse error and VARE_UNDEFERR was set.
- *	varNoError if there was a parse error and VARE_UNDEFERR was not set.
+ *	Returns var_Error if there was a parse error and VARE_UNDEFERR was
+ *	set.
+ *	Returns varUndefined if there was an undefined variable and
+ *	VARE_UNDEFERR was not set.
  *
- *	Parsing should continue at str + *lengthPtr.
- *	TODO: Document the value of *lengthPtr on parse errors.  It might be
- *	0, or +1, or the index of the parse error, or the guessed end of the
+ *	Parsing should continue at *pp.
+ *	TODO: Document the value of *pp on parse errors.  It might be advanced
+ *	by 0, or +1, or the index of the parse error, or the guessed end of the
  *	variable expression.
  *
  *	If var_Error is returned, a diagnostic may or may not have been
  *	printed. XXX: This is inconsistent.
  *
- *	If varNoError is returned, a diagnostic may or may not have been
- *	printed. XXX: This is inconsistent, and as of 2020-09-08, returning
- *	varNoError is even used to return a regular, non-error empty string.
+ *	If varUndefined is returned, a diagnostic may or may not have been
+ *	printed. XXX: This is inconsistent.
  *
  *	After using the returned value, *freePtr must be freed, preferably
  *	using bmake_free since it is NULL in most cases.
@@ -3588,7 +3588,7 @@ Var_Parse(const char **pp, GNode *ctxt, VarEvalFlags eflags,
 		}
 
 		free(varname);
-		*out_val = varNoError;
+		*out_val = varUndefined;
 		return VPR_OK;
 	    }
 
@@ -3686,8 +3686,8 @@ Var_Parse(const char **pp, GNode *ctxt, VarEvalFlags eflags,
 		*freePtr = nstr;
 	    } else {
 		/* The expression is still undefined, therefore discard the
-		 * actual value and return an empty string instead. */
-		nstr = (eflags & VARE_UNDEFERR) ? var_Error : varNoError;
+		 * actual value and return an error marker instead. */
+		nstr = (eflags & VARE_UNDEFERR) ? var_Error : varUndefined;
 	    }
 	}
 	if (nstr != Buf_GetAll(&v->val, NULL))
@@ -3757,7 +3757,7 @@ Var_Subst(const char *str, GNode *ctxt, VarEvalFlags eflags, char **out_res)
 	    (void)Var_Parse(&nested_str, ctxt, eflags, &val, &freeIt);
 	    /* TODO: handle errors */
 
-	    if (val == var_Error || val == varNoError) {
+	    if (val == var_Error || val == varUndefined) {
 		/*
 		 * If performing old-time variable substitution, skip over
 		 * the variable and continue with the substitution. Otherwise,