[src/trunk]: src/usr.bin/xlint/lint1 lint: document and demonstrate the bug i...

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

details:   https://anonhg.NetBSD.org/src/rev/4202da1f8e67
branches:  trunk
changeset: 958442:4202da1f8e67
user:      rillig <rillig%NetBSD.org@localhost>
date:      Mon Jan 04 23:47:26 2021 +0000

description:
lint: document and demonstrate the bug in check_precedence_confusion

It took quite a while to get to the correct interpretation of this small
piece of code and to draw the right conclusions from it.  Now the bug is
finally ready to be fixed, as already announced in the test.

diffstat:

 tests/usr.bin/xlint/lint1/msg_169.c   |  38 +++++++++++++++++++++++++++++++---
 tests/usr.bin/xlint/lint1/msg_169.exp |   1 +
 usr.bin/xlint/lint1/tree.c            |  29 +++++---------------------
 3 files changed, 41 insertions(+), 27 deletions(-)

diffs (108 lines):

diff -r ca352d3f9776 -r 4202da1f8e67 tests/usr.bin/xlint/lint1/msg_169.c
--- a/tests/usr.bin/xlint/lint1/msg_169.c       Mon Jan 04 23:17:03 2021 +0000
+++ b/tests/usr.bin/xlint/lint1/msg_169.c       Mon Jan 04 23:47:26 2021 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: msg_169.c,v 1.3 2021/01/04 22:41:56 rillig Exp $       */
+/*     $NetBSD: msg_169.c,v 1.4 2021/01/04 23:47:27 rillig Exp $       */
 # 3 "msg_169.c"
 
 // Test for message: precedence confusion possible: parenthesize! [169]
@@ -146,6 +146,36 @@
        ok = a + b * c;
 }
 
-// TODO: add a test with unsigned long instead of unsigned, trying to
-//  demonstrate that the typo in check_precedence_confusion actually has an
-//  effect.
+/*
+ * Before tree.c 1.132 from 2021-01-04, there was a typo in
+ * check_precedence_confusion that prevented the right-hand operand from
+ * being flagged as possibly confusing if there was an implicit conversion
+ * or an explicit cast between the main operator ('|') and the nested
+ * operator ('&').
+ */
+void
+implicit_conversion_to_long(long la, int a)
+{
+       int ok;
+
+       ok = a & a | la;        /* always marked as confusing */
+       ok = la | a & a;        /* marked as confusing since tree.c 1.132 */
+
+       ok = (a & a) | la;      /* always ok */
+       ok = la | (a & a);      /* always ok */
+
+       /*
+        * Before tree.c 1.132, this expression didn't generate a warning
+        * because the right-hand operand was CVT, and there is no confusing
+        * precedence between BITOR and CVT.
+        *
+        * Since tree.c 1.132, this expression doesn't generate a warning
+        * because the right-hand operand is parenthesized.  There is no way
+        * to have the right operand casted and at the same time not
+        * parenthesized since the cast operator has higher precedence.
+        *
+        * In summary, there is no visible change, but the implementation is
+        * now works as intended.
+        */
+       ok = la | (int)(a & a); /* always ok */
+}
diff -r ca352d3f9776 -r 4202da1f8e67 tests/usr.bin/xlint/lint1/msg_169.exp
--- a/tests/usr.bin/xlint/lint1/msg_169.exp     Mon Jan 04 23:17:03 2021 +0000
+++ b/tests/usr.bin/xlint/lint1/msg_169.exp     Mon Jan 04 23:47:26 2021 +0000
@@ -22,3 +22,4 @@
 msg_169.c(126): warning: precedence confusion possible: parenthesize! [169]
 msg_169.c(127): warning: precedence confusion possible: parenthesize! [169]
 msg_169.c(131): warning: precedence confusion possible: parenthesize! [169]
+msg_169.c(161): warning: precedence confusion possible: parenthesize! [169]
diff -r ca352d3f9776 -r 4202da1f8e67 usr.bin/xlint/lint1/tree.c
--- a/usr.bin/xlint/lint1/tree.c        Mon Jan 04 23:17:03 2021 +0000
+++ b/usr.bin/xlint/lint1/tree.c        Mon Jan 04 23:47:26 2021 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: tree.c,v 1.130 2021/01/04 23:17:03 rillig Exp $        */
+/*     $NetBSD: tree.c,v 1.131 2021/01/04 23:47:26 rillig Exp $        */
 
 /*
  * Copyright (c) 1994, 1995 Jochen Pohl
@@ -37,7 +37,7 @@
 
 #include <sys/cdefs.h>
 #if defined(__RCSID) && !defined(lint)
-__RCSID("$NetBSD: tree.c,v 1.130 2021/01/04 23:17:03 rillig Exp $");
+__RCSID("$NetBSD: tree.c,v 1.131 2021/01/04 23:47:26 rillig Exp $");
 #endif
 
 #include <float.h>
@@ -4011,28 +4011,11 @@
                /*
                 * FIXME: There is a typo "tn->tn_op == CVT", which should
                 * rather be "rn->tn_op".  Since tn must be a binary operator,
-                * it can never be CVT.
-                *
-                * Before fixing this though, there should be a unit test
-                * that demonstrates an actual change in behavior when this
-                * bug gets fixed.
-                *
-                * rn must be a chain of casts and conversions, and at least
-                * one of these must be a parenthesized cast.
+                * it can never be CVT, so the loop is never taken.
                 *
-                * The argument of the innermost cast or conversion must not
-                * be parenthesized.
-                *
-                * The argument of the innermost cast or conversion must be
-                * an expression with confusing precedence.  Since all these
-                * expressions have lower precedence than a cast, these can
-                * only appear as a parenthesized expression.  This in turn
-                * makes the whole loop superfluous.
-                *
-                * An edge case might be due to constant folding, if the
-                * nodes created from constant folding did not preserve
-                * tn_parenthesized properly.  But that would be another bug,
-                * so it doesn't count as an argument.
+                * Since the loop is never taken, if the right-hand operand
+                * is CVT, it is not followed to the actually interesting
+                * operator.
                 */
                for (rn = tn->tn_right; tn->tn_op == CVT; rn = rn->tn_left)
                        rparn |= rn->tn_parenthesized;