Re: [PATCH] Simplify shift operations on constants

[Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>]

On Sat, Mar 2, 2019 at 4:16 AM Thomas Weißschuh <thomas@xxxxxxxx> wrote:
>
> The result of a shift operation on a constants by a constant value is
> also constant.

Yes it is, and sparse used to get this right.

This was actually broken by commit 0b73dee0 ("big-shift: move the
check into check_shift_count()") which clearly _intended_ to do no
harm, but which was completely broken.

The

        if (conservative)
                return 0;

test makes no sense at all. Your patch "fixed" it by limiting it to a
non-constant value, but that doesn't actually fix anything in reality,
it just hides the broken check (and it will also hide the warning from
check_shift_count().

The code *used* to do

        if (r >= ctype->bit_size) {
                if (conservative)
                        return 0;
...

ie it would *not* simplify expressions where the shift size was equal
to or larger than the type size. Which is correct. But then the check
against the type size was moved into the "check_shift_count()"
function, and now the "if (conservative)" test makes no sense
what-so-ever any more.

I think a better patch for expand.c would be something like the
attached, that fixes the logic mistake, and moves the "conservative"
check too into check_shift_count().

NOTE! This is entirely untested, and I think all credit for the patch
should go to Thomas who write a test-case and a changelog. I just
think the expand.c part should be made to make sense.

Thanks,

                   Linus

 expand.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/expand.c b/expand.c
index e8e50b08..95f9fda6 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;