Mon Jan 4 23:47:27 2021 UTC ()
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.


(rillig)
diff -r1.3 -r1.4 src/tests/usr.bin/xlint/lint1/msg_169.c
diff -r1.2 -r1.3 src/tests/usr.bin/xlint/lint1/msg_169.exp
diff -r1.130 -r1.131 src/usr.bin/xlint/lint1/tree.c
cvs diff -r1.3 -r1.4 src/tests/usr.bin/xlint/lint1/msg_169.c (expand / switch to unified diff)

--- src/tests/usr.bin/xlint/lint1/msg_169.c 2021/01/04 22:41:56 1.3
+++ src/tests/usr.bin/xlint/lint1/msg_169.c 2021/01/04 23:47:27 1.4
@@ -1,14 +1,14 @@ @@ -1,14 +1,14 @@
1/* $NetBSD: msg_169.c,v 1.3 2021/01/04 22:41:56 rillig Exp $ */ 1/* $NetBSD: msg_169.c,v 1.4 2021/01/04 23:47:27 rillig Exp $ */
2# 3 "msg_169.c" 2# 3 "msg_169.c"
3 3
4// Test for message: precedence confusion possible: parenthesize! [169] 4// Test for message: precedence confusion possible: parenthesize! [169]
5 5
6/* lint1-flags: -g -h -S -w */ 6/* lint1-flags: -g -h -S -w */
7 7
8typedef _Bool bool; 8typedef _Bool bool;
9 9
10void 10void
11confusing_shift_arith(unsigned a, unsigned b, unsigned c, unsigned char ch) 11confusing_shift_arith(unsigned a, unsigned b, unsigned c, unsigned char ch)
12{ 12{
13 unsigned con, okl, okr; 13 unsigned con, okl, okr;
14 14
@@ -136,16 +136,46 @@ cast_expressions(char a, char b, char c) @@ -136,16 +136,46 @@ cast_expressions(char a, char b, char c)
136 con = a | (unsigned)(b & c); 136 con = a | (unsigned)(b & c);
137 con = (unsigned)(a | b) & (unsigned)c; 137 con = (unsigned)(a | b) & (unsigned)c;
138 con = (unsigned)(a | b) & c; 138 con = (unsigned)(a | b) & c;
139} 139}
140 140
141void 141void
142expected_precedence(int a, int b, int c) 142expected_precedence(int a, int b, int c)
143{ 143{
144 int ok; 144 int ok;
145 145
146 ok = a + b * c; 146 ok = a + b * c;
147} 147}
148 148
149// TODO: add a test with unsigned long instead of unsigned, trying to 149/*
150// demonstrate that the typo in check_precedence_confusion actually has an 150 * Before tree.c 1.132 from 2021-01-04, there was a typo in
151// effect. 151 * check_precedence_confusion that prevented the right-hand operand from
 152 * being flagged as possibly confusing if there was an implicit conversion
 153 * or an explicit cast between the main operator ('|') and the nested
 154 * operator ('&').
 155 */
 156void
 157implicit_conversion_to_long(long la, int a)
 158{
 159 int ok;
 160
 161 ok = a & a | la; /* always marked as confusing */
 162 ok = la | a & a; /* marked as confusing since tree.c 1.132 */
 163
 164 ok = (a & a) | la; /* always ok */
 165 ok = la | (a & a); /* always ok */
 166
 167 /*
 168 * Before tree.c 1.132, this expression didn't generate a warning
 169 * because the right-hand operand was CVT, and there is no confusing
 170 * precedence between BITOR and CVT.
 171 *
 172 * Since tree.c 1.132, this expression doesn't generate a warning
 173 * because the right-hand operand is parenthesized. There is no way
 174 * to have the right operand casted and at the same time not
 175 * parenthesized since the cast operator has higher precedence.
 176 *
 177 * In summary, there is no visible change, but the implementation is
 178 * now works as intended.
 179 */
 180 ok = la | (int)(a & a); /* always ok */
 181}
cvs diff -r1.2 -r1.3 src/tests/usr.bin/xlint/lint1/Attic/msg_169.exp (expand / switch to unified diff)

--- src/tests/usr.bin/xlint/lint1/Attic/msg_169.exp 2021/01/04 15:52:51 1.2
+++ src/tests/usr.bin/xlint/lint1/Attic/msg_169.exp 2021/01/04 23:47:27 1.3
@@ -12,13 +12,14 @@ msg_169.c(50): warning: precedence confu @@ -12,13 +12,14 @@ msg_169.c(50): warning: precedence confu
12msg_169.c(54): warning: precedence confusion possible: parenthesize! [169] 12msg_169.c(54): warning: precedence confusion possible: parenthesize! [169]
13msg_169.c(68): warning: precedence confusion possible: parenthesize! [169] 13msg_169.c(68): warning: precedence confusion possible: parenthesize! [169]
14msg_169.c(72): warning: precedence confusion possible: parenthesize! [169] 14msg_169.c(72): warning: precedence confusion possible: parenthesize! [169]
15msg_169.c(76): warning: precedence confusion possible: parenthesize! [169] 15msg_169.c(76): warning: precedence confusion possible: parenthesize! [169]
16msg_169.c(80): warning: precedence confusion possible: parenthesize! [169] 16msg_169.c(80): warning: precedence confusion possible: parenthesize! [169]
17msg_169.c(84): warning: precedence confusion possible: parenthesize! [169] 17msg_169.c(84): warning: precedence confusion possible: parenthesize! [169]
18msg_169.c(88): warning: precedence confusion possible: parenthesize! [169] 18msg_169.c(88): warning: precedence confusion possible: parenthesize! [169]
19msg_169.c(92): warning: precedence confusion possible: parenthesize! [169] 19msg_169.c(92): warning: precedence confusion possible: parenthesize! [169]
20msg_169.c(96): warning: precedence confusion possible: parenthesize! [169] 20msg_169.c(96): warning: precedence confusion possible: parenthesize! [169]
21msg_169.c(101): warning: precedence confusion possible: parenthesize! [169] 21msg_169.c(101): warning: precedence confusion possible: parenthesize! [169]
22msg_169.c(126): warning: precedence confusion possible: parenthesize! [169] 22msg_169.c(126): warning: precedence confusion possible: parenthesize! [169]
23msg_169.c(127): warning: precedence confusion possible: parenthesize! [169] 23msg_169.c(127): warning: precedence confusion possible: parenthesize! [169]
24msg_169.c(131): warning: precedence confusion possible: parenthesize! [169] 24msg_169.c(131): warning: precedence confusion possible: parenthesize! [169]
 25msg_169.c(161): warning: precedence confusion possible: parenthesize! [169]
cvs diff -r1.130 -r1.131 src/usr.bin/xlint/lint1/tree.c (expand / switch to unified diff)

--- src/usr.bin/xlint/lint1/tree.c 2021/01/04 23:17:03 1.130
+++ src/usr.bin/xlint/lint1/tree.c 2021/01/04 23:47:26 1.131
@@ -1,14 +1,14 @@ @@ -1,14 +1,14 @@
1/* $NetBSD: tree.c,v 1.130 2021/01/04 23:17:03 rillig Exp $ */ 1/* $NetBSD: tree.c,v 1.131 2021/01/04 23:47:26 rillig Exp $ */
2 2
3/* 3/*
4 * Copyright (c) 1994, 1995 Jochen Pohl 4 * Copyright (c) 1994, 1995 Jochen Pohl
5 * All Rights Reserved. 5 * All Rights Reserved.
6 * 6 *
7 * Redistribution and use in source and binary forms, with or without 7 * Redistribution and use in source and binary forms, with or without
8 * modification, are permitted provided that the following conditions 8 * modification, are permitted provided that the following conditions
9 * are met: 9 * are met:
10 * 1. Redistributions of source code must retain the above copyright 10 * 1. Redistributions of source code must retain the above copyright
11 * notice, this list of conditions and the following disclaimer. 11 * notice, this list of conditions and the following disclaimer.
12 * 2. Redistributions in binary form must reproduce the above copyright 12 * 2. Redistributions in binary form must reproduce the above copyright
13 * notice, this list of conditions and the following disclaimer in the 13 * notice, this list of conditions and the following disclaimer in the
14 * documentation and/or other materials provided with the distribution. 14 * documentation and/or other materials provided with the distribution.
@@ -27,27 +27,27 @@ @@ -27,27 +27,27 @@
27 * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, 27 * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
28 * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY 28 * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
29 * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT 29 * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
30 * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF 30 * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
31 * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 31 * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
32 */ 32 */
33 33
34#if HAVE_NBTOOL_CONFIG_H 34#if HAVE_NBTOOL_CONFIG_H
35#include "nbtool_config.h" 35#include "nbtool_config.h"
36#endif 36#endif
37 37
38#include <sys/cdefs.h> 38#include <sys/cdefs.h>
39#if defined(__RCSID) && !defined(lint) 39#if defined(__RCSID) && !defined(lint)
40__RCSID("$NetBSD: tree.c,v 1.130 2021/01/04 23:17:03 rillig Exp $"); 40__RCSID("$NetBSD: tree.c,v 1.131 2021/01/04 23:47:26 rillig Exp $");
41#endif 41#endif
42 42
43#include <float.h> 43#include <float.h>
44#include <limits.h> 44#include <limits.h>
45#include <math.h> 45#include <math.h>
46#include <signal.h> 46#include <signal.h>
47#include <stdlib.h> 47#include <stdlib.h>
48#include <string.h> 48#include <string.h>
49 49
50#include "lint1.h" 50#include "lint1.h"
51#include "cgram.h" 51#include "cgram.h"
52 52
53static tnode_t *new_integer_constant_node(tspec_t, int64_t); 53static tnode_t *new_integer_constant_node(tspec_t, int64_t);
@@ -4001,48 +4001,31 @@ check_precedence_confusion(tnode_t *tn) @@ -4001,48 +4001,31 @@ check_precedence_confusion(tnode_t *tn)
4001 lparn = 0; 4001 lparn = 0;
4002 for (ln = tn->tn_left; ln->tn_op == CVT; ln = ln->tn_left) 4002 for (ln = tn->tn_left; ln->tn_op == CVT; ln = ln->tn_left)
4003 lparn |= ln->tn_parenthesized; 4003 lparn |= ln->tn_parenthesized;
4004 lparn |= ln->tn_parenthesized; 4004 lparn |= ln->tn_parenthesized;
4005 lop = ln->tn_op; 4005 lop = ln->tn_op;
4006 4006
4007 dprint_node(tn); 4007 dprint_node(tn);
4008 4008
4009 if (mp->m_binary) { 4009 if (mp->m_binary) {
4010 rparn = 0; 4010 rparn = 0;
4011 /* 4011 /*
4012 * FIXME: There is a typo "tn->tn_op == CVT", which should 4012 * FIXME: There is a typo "tn->tn_op == CVT", which should
4013 * rather be "rn->tn_op". Since tn must be a binary operator, 4013 * rather be "rn->tn_op". Since tn must be a binary operator,
4014 * it can never be CVT. 4014 * it can never be CVT, so the loop is never taken.
4015 * 4015 *
4016 * Before fixing this though, there should be a unit test 4016 * Since the loop is never taken, if the right-hand operand
4017 * that demonstrates an actual change in behavior when this 4017 * is CVT, it is not followed to the actually interesting
4018 * bug gets fixed. 4018 * operator.
4019 * 
4020 * rn must be a chain of casts and conversions, and at least 
4021 * one of these must be a parenthesized cast. 
4022 * 
4023 * The argument of the innermost cast or conversion must not 
4024 * be parenthesized. 
4025 * 
4026 * The argument of the innermost cast or conversion must be 
4027 * an expression with confusing precedence. Since all these 
4028 * expressions have lower precedence than a cast, these can 
4029 * only appear as a parenthesized expression. This in turn 
4030 * makes the whole loop superfluous. 
4031 * 
4032 * An edge case might be due to constant folding, if the 
4033 * nodes created from constant folding did not preserve 
4034 * tn_parenthesized properly. But that would be another bug, 
4035 * so it doesn't count as an argument. 
4036 */ 4019 */
4037 for (rn = tn->tn_right; tn->tn_op == CVT; rn = rn->tn_left) 4020 for (rn = tn->tn_right; tn->tn_op == CVT; rn = rn->tn_left)
4038 rparn |= rn->tn_parenthesized; 4021 rparn |= rn->tn_parenthesized;
4039 rparn |= rn->tn_parenthesized; 4022 rparn |= rn->tn_parenthesized;
4040 rop = rn->tn_op; 4023 rop = rn->tn_op;
4041 } 4024 }
4042 4025
4043 dowarn = 0; 4026 dowarn = 0;
4044 4027
4045 switch (tn->tn_op) { 4028 switch (tn->tn_op) {
4046 case SHL: 4029 case SHL:
4047 case SHR: 4030 case SHR:
4048 if (!lparn && (lop == PLUS || lop == MINUS)) { 4031 if (!lparn && (lop == PLUS || lop == MINUS)) {