On Sat, Mar 02, 2019 at 11:34:15PM +0100, Thomas Weißschuh wrote:
>
> I'm actually not sure how to proceed.
> Should I integrate Linus' fix, your description and my test and resend them, or
> do you want to do it?
It's fine, thanks. I've taken Linus' version, adapted my description,
renamed you test and added another test. I'm proposing to push the
patch here under.
Thanks
-- Luc
>From 83cfb92761f4602bc08aa4be6fd9834a3b98d5e3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= <thomas@xxxxxxxx>
Date: Sat, 2 Mar 2019 13:16:05 +0100
Subject: [PATCH v2] expand: 'conservative' must not bypass valid simplifications
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
During the expansion of shifts, the variable 'conservative' is used
to inhibit any possible diagnostics (for example, because he needed
information is if the expression is a constant or not).
However, this must not inhibit the simplification of valid shift
expressions. Unfortunately, by moving the validation inside
check_shift_count(), this what was done by commit
0b73dee01 ("big-shift: move the check into check_shift_count()").
Found through a false positive VLA detected in the Linux kernel.
The array size was computed through min() on a shifted constant value
and sparse complained about it.
Fix this by changing the logic of check_shift_count():
1) moving the test of 'conservative' inside check_shift_count()
and only issuing warnings if set.
2) moving the warning part in a separate function: warn_shift_count()
3) let check_shift_count() return if the shift count is valid
so that the simplication can be eluded if not.
Fixes: 0b73dee0171a15800d0a4ae6225b602bf8961599
Signed-off-by: Thomas Weißschuh <thomas@xxxxxxxx>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@xxxxxxxxx>
---
expand.c | 23 ++++++++-----
validation/constexpr-shift.c | 12 +++++++
validation/expand/bad-shift.c | 64 +++++++++++++++++++++++++++++++++++
3 files changed, 91 insertions(+), 8 deletions(-)
create mode 100644 validation/constexpr-shift.c
create mode 100644 validation/expand/bad-shift.c
diff --git a/expand.c b/expand.c
index e8e50b080..95f9fda63 100644
--- a/expand.c
+++ b/expand.c
@@ -158,19 +158,14 @@ Float:
expr->type = EXPR_FVALUE;
}
-static void check_shift_count(struct expression *expr, struct expression *right)
+static void warn_shift_count(struct expression *expr, struct symbol *ctype, long long count)
{
- struct symbol *ctype = expr->ctype;
- long long count = get_longlong(right);
-
if (count < 0) {
if (!Wshift_count_negative)
return;
warning(expr->pos, "shift count is negative (%lld)", count);
return;
}
- if (count < ctype->bit_size)
- return;
if (ctype->type == SYM_NODE)
ctype = ctype->ctype.base_type;
@@ -179,6 +174,19 @@ static void check_shift_count(struct expression *expr, struct expression *right)
warning(expr->pos, "shift too big (%llu) for type %s", count, show_typename(ctype));
}
+/* Return true if constant shift size is valid */
+static bool check_shift_count(struct expression *expr, struct expression *right)
+{
+ struct symbol *ctype = expr->ctype;
+ long long count = get_longlong(right);
+
+ if (count >= 0 && count < ctype->bit_size)
+ return true;
+ if (!conservative)
+ warn_shift_count(expr, ctype, count);
+ return false;
+}
+
/*
* CAREFUL! We need to get the size and sign of the
* result right!
@@ -197,9 +205,8 @@ static int simplify_int_binop(struct expression *expr, struct symbol *ctype)
return 0;
r = right->value;
if (expr->op == SPECIAL_LEFTSHIFT || expr->op == SPECIAL_RIGHTSHIFT) {
- if (conservative)
+ if (!check_shift_count(expr, right))
return 0;
- check_shift_count(expr, right);
}
if (left->type != EXPR_VALUE)
return 0;
diff --git a/validation/constexpr-shift.c b/validation/constexpr-shift.c
new file mode 100644
index 000000000..df01b74e8
--- /dev/null
+++ b/validation/constexpr-shift.c
@@ -0,0 +1,12 @@
+#define __is_constexpr(x) \
+ (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
+
+static void test(int x) {
+ static int b[] = {
+ [__builtin_choose_expr(__is_constexpr(1 << 1), 1, x)] = 0,
+ };
+}
+
+/*
+ * check-name: constexpr-shift
+ */
diff --git a/validation/expand/bad-shift.c b/validation/expand/bad-shift.c
new file mode 100644
index 000000000..22c4341f1
--- /dev/null
+++ b/validation/expand/bad-shift.c
@@ -0,0 +1,64 @@
+#define MAX (sizeof(int) * __CHAR_BIT__)
+
+static int lmax(int a)
+{
+ return 1 << MAX;
+}
+
+static int lneg(int a)
+{
+ return 1 << -1;
+}
+
+static int rmax(int a)
+{
+ return 1 >> MAX;
+}
+
+static int rneg(int a)
+{
+ return 1 >> -1;
+}
+
+/*
+ * check-name: bad-shift
+ * check-command: test-linearize -Wno-decl $file
+ *
+ * check-output-start
+lmax:
+.L0:
+ <entry-point>
+ shl.32 %r1 <- $1, $32
+ ret.32 %r1
+
+
+lneg:
+.L2:
+ <entry-point>
+ shl.32 %r3 <- $1, $0xffffffff
+ ret.32 %r3
+
+
+rmax:
+.L4:
+ <entry-point>
+ asr.32 %r5 <- $1, $32
+ ret.32 %r5
+
+
+rneg:
+.L6:
+ <entry-point>
+ asr.32 %r7 <- $1, $0xffffffff
+ ret.32 %r7
+
+
+ * check-output-end
+ *
+ * check-error-start
+expand/bad-shift.c:5:18: warning: shift too big (32) for type int
+expand/bad-shift.c:10:18: warning: shift count is negative (-1)
+expand/bad-shift.c:15:18: warning: shift too big (32) for type int
+expand/bad-shift.c:20:18: warning: shift count is negative (-1)
+ * check-error-end
+ */
--
2.21.0
