[src/trunk]: src/usr.bin/make make(1): fix evaluation of unreachable conditions

[rillig <rillig%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/2aee844679fa
branches:  trunk
changeset: 1011395:2aee844679fa
user:      rillig <rillig%NetBSD.org@localhost>
date:      Sun Jun 28 11:06:26 2020 +0000

description:
make(1): fix evaluation of unreachable conditions

Since 2015-10-11, make had evaluated unreachable conditions even though
the manual page said it didn't.

diffstat:

 usr.bin/make/cond.c                    |  28 +++++++++-----------
 usr.bin/make/unit-tests/cond-short.exp |   6 +++-
 usr.bin/make/unit-tests/cond-short.mk  |  45 ++++++++++++++++++++++++---------
 3 files changed, 50 insertions(+), 29 deletions(-)

diffs (200 lines):

diff -r 69535e562439 -r 2aee844679fa usr.bin/make/cond.c
--- a/usr.bin/make/cond.c       Sun Jun 28 09:42:40 2020 +0000
+++ b/usr.bin/make/cond.c       Sun Jun 28 11:06:26 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: cond.c,v 1.75 2017/04/16 20:59:04 riastradh Exp $      */
+/*     $NetBSD: cond.c,v 1.76 2020/06/28 11:06:26 rillig Exp $ */
 
 /*
  * Copyright (c) 1988, 1989, 1990 The Regents of the University of California.
@@ -70,14 +70,14 @@
  */
 
 #ifndef MAKE_NATIVE
-static char rcsid[] = "$NetBSD: cond.c,v 1.75 2017/04/16 20:59:04 riastradh Exp $";
+static char rcsid[] = "$NetBSD: cond.c,v 1.76 2020/06/28 11:06:26 rillig Exp $";
 #else
 #include <sys/cdefs.h>
 #ifndef lint
 #if 0
 static char sccsid[] = "@(#)cond.c     8.2 (Berkeley) 1/2/94";
 #else
-__RCSID("$NetBSD: cond.c,v 1.75 2017/04/16 20:59:04 riastradh Exp $");
+__RCSID("$NetBSD: cond.c,v 1.76 2020/06/28 11:06:26 rillig Exp $");
 #endif
 #endif /* not lint */
 #endif
@@ -146,7 +146,7 @@
  * last two fields are stored in condInvert and condDefProc, respectively.
  */
 static void CondPushBack(Token);
-static int CondGetArg(char **, char **, const char *);
+static int CondGetArg(Boolean, char **, char **, const char *);
 static Boolean CondDoDefined(int, const char *);
 static int CondStrMatch(const void *, const void *);
 static Boolean CondDoMake(int, const char *);
@@ -225,9 +225,6 @@
  * CondGetArg --
  *     Find the argument of a built-in function.
  *
- * Input:
- *     parens          TRUE if arg should be bounded by parens
- *
  * Results:
  *     The length of the argument and the address of the argument.
  *
@@ -238,7 +235,7 @@
  *-----------------------------------------------------------------------
  */
 static int
-CondGetArg(char **linePtr, char **argPtr, const char *func)
+CondGetArg(Boolean doEval, char **linePtr, char **argPtr, const char *func)
 {
     char         *cp;
     int                  argLen;
@@ -290,7 +287,8 @@
            int         len;
            void        *freeIt;
 
-           cp2 = Var_Parse(cp, VAR_CMD, VARF_UNDEFERR|VARF_WANTRES,
+           cp2 = Var_Parse(cp, VAR_CMD, VARF_UNDEFERR|
+                           (doEval ? VARF_WANTRES : 0),
                            &len, &freeIt);
            Buf_AddBytes(&buf, strlen(cp2), cp2);
            free(freeIt);
@@ -577,7 +575,7 @@
            /* if we are in quotes, then an undefined variable is ok */
            str = Var_Parse(condExpr, VAR_CMD,
                            ((!qt && doEval) ? VARF_UNDEFERR : 0) |
-                           VARF_WANTRES, &len, freeIt);
+                           (doEval ? VARF_WANTRES : 0), &len, freeIt);
            if (str == var_Error) {
                if (*freeIt) {
                    free(*freeIt);
@@ -813,7 +811,7 @@
 }
 
 static int
-get_mpt_arg(char **linePtr, char **argPtr, const char *func MAKE_ATTR_UNUSED)
+get_mpt_arg(Boolean doEval, char **linePtr, char **argPtr, const char *func MAKE_ATTR_UNUSED)
 {
     /*
      * Use Var_Parse to parse the spec in parens and return
@@ -827,7 +825,7 @@
     /* We do all the work here and return the result as the length */
     *argPtr = NULL;
 
-    val = Var_Parse(cp - 1, VAR_CMD, VARF_WANTRES, &length, &freeIt);
+    val = Var_Parse(cp - 1, VAR_CMD, doEval ? VARF_WANTRES : 0, &length, &freeIt);
     /*
      * Advance *linePtr to beyond the closing ). Note that
      * we subtract one because 'length' is calculated from 'cp - 1'.
@@ -864,7 +862,7 @@
     static const struct fn_def {
        const char  *fn_name;
        int         fn_name_len;
-        int         (*fn_getarg)(char **, char **, const char *);
+        int         (*fn_getarg)(Boolean, char **, char **, const char *);
        Boolean     (*fn_proc)(int, const char *);
     } fn_defs[] = {
        { "defined",   7, CondGetArg, CondDoDefined },
@@ -892,7 +890,7 @@
        if (*cp != '(')
            break;
 
-       arglen = fn_def->fn_getarg(&cp, &arg, fn_def->fn_name);
+       arglen = fn_def->fn_getarg(doEval, &cp, &arg, fn_def->fn_name);
        if (arglen <= 0) {
            condExpr = cp;
            return arglen < 0 ? TOK_ERROR : TOK_FALSE;
@@ -917,7 +915,7 @@
      * would be invalid if we did "defined(a)" - so instead treat as an
      * expression.
      */
-    arglen = CondGetArg(&cp, &arg, NULL);
+    arglen = CondGetArg(doEval, &cp, &arg, NULL);
     for (cp1 = cp; isspace(*(unsigned char *)cp1); cp1++)
        continue;
     if (*cp1 == '=' || *cp1 == '!')
diff -r 69535e562439 -r 2aee844679fa usr.bin/make/unit-tests/cond-short.exp
--- a/usr.bin/make/unit-tests/cond-short.exp    Sun Jun 28 09:42:40 2020 +0000
+++ b/usr.bin/make/unit-tests/cond-short.exp    Sun Jun 28 11:06:26 2020 +0000
@@ -1,5 +1,7 @@
-unexpected and
 expected and
-unexpected or
+expected and exists
+expected and empty
 expected or
+expected or exists
+expected or empty
 exit status 0
diff -r 69535e562439 -r 2aee844679fa usr.bin/make/unit-tests/cond-short.mk
--- a/usr.bin/make/unit-tests/cond-short.mk     Sun Jun 28 09:42:40 2020 +0000
+++ b/usr.bin/make/unit-tests/cond-short.mk     Sun Jun 28 11:06:26 2020 +0000
@@ -1,18 +1,13 @@
-# $NetBSD: cond-short.mk,v 1.1 2020/06/28 09:42:40 rillig Exp $
+# $NetBSD: cond-short.mk,v 1.2 2020/06/28 11:06:26 rillig Exp $
 #
 # Demonstrates that in conditions, the right-hand side of an && or ||
-# is evaluated even though it cannot influence the result.
-#
-# This is unexpected for several reasons:
+# is only evaluated if it can actually influence the result.
 #
-# 1.  The manual page says: "bmake will only evaluate a conditional as
-#     far as is necessary to determine its value."
+# Between 2015-10-11 and 2020-06-28, the right-hand side of an && or ||
+# operator was always evaluated, which was wrong.
 #
-# 2.  It differs from the way that these operators are evaluated in
-#     almost all other programming languages.
-#
-# 3.  In cond.c there are lots of doEval variables.
-#
+
+# The && operator.
 
 .if 0 && ${echo "unexpected and" 1>&2 :L:sh}
 .endif
@@ -20,13 +15,39 @@
 .if 1 && ${echo "expected and" 1>&2 :L:sh}
 .endif
 
+.if 0 && exists(nonexistent${echo "unexpected and exists" 1>&2 :L:sh})
+.endif
+
+.if 1 && exists(nonexistent${echo "expected and exists" 1>&2 :L:sh})
+.endif
+
+.if 0 && empty(${echo "unexpected and empty" 1>&2 :L:sh})
+.endif
+
+.if 1 && empty(${echo "expected and empty" 1>&2 :L:sh})
+.endif
+
+# The || operator.
+
 .if 1 || ${echo "unexpected or" 1>&2 :L:sh}
 .endif
 
 .if 0 || ${echo "expected or" 1>&2 :L:sh}
 .endif
 
-# The following paragraphs demonstrate the workaround.
+.if 1 || exists(nonexistent${echo "unexpected or exists" 1>&2 :L:sh})
+.endif
+
+.if 0 || exists(nonexistent${echo "expected or exists" 1>&2 :L:sh})
+.endif
+
+.if 1 || empty(${echo "unexpected or empty" 1>&2 :L:sh})
+.endif
+
+.if 0 || empty(${echo "expected or empty" 1>&2 :L:sh})
+.endif
+
+# Unreachable nested conditions are skipped completely as well.
 
 .if 0
 .  if ${echo "unexpected nested and" 1>&2 :L:sh}