On Sat, Mar 02, 2019 at 08:26:36AM -0800, Linus Torvalds wrote:
> 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.
Yes, indeed. Mea culpa.
> 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().
I just sent a smaller fix (but with duplicated test for the size)
but yes, separating the check from the warn is much better.
> 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.
and found the problem. Sure.
-- Luc
