On 2020-03-13 04:08, Christoph Hellwig wrote:
> On Thu, Mar 12, 2020 at 07:37:17PM -0700, Bart Van Assche wrote:
>> @@ -2680,8 +2681,7 @@ static void deb_space_print(struct scsi_tape *STp, int direction, char *units, u
>> if (!debugging)
>> return;
>>
>> - sc = cmd[2] & 0x80 ? 0xff000000 : 0;
>> - sc |= (cmd[2] << 16) | (cmd[3] << 8) | cmd[4];
>> + sc = sign_extend32(get_unaligned_be24(&cmd[2]), 23);
>
> Btw, didn't the old code here have undefined behavior if cmd[] is
> a u8 and we shift by a larger amount?
That's a great question. According to my interpretation of the C
standard the above code is fine:
<quote>
6.5.7 Bitwise shift operators
Syntax
shift-expression:
additive-expression
shift-expression << additive-expression
shift-expression >> additive-expression
Constraints
Each of the operands shall have integer type.
Semantics
The integer promotions are performed on each of the operands. The type
of the result is that of the promoted left operand. If the value of the
right operand is negative or is greater than or equal to the width of
the promoted left operand, the behavior is undefined. [ ... ]
</quote>
Since the RHS has integer type, I think the above means that cmd[2] and
cmd[3] are promoted to a signed integer before being shifted left. As
far as I know the Linux kernel only supports compilers for which
sizeof(int) >= 4 so I think that the old code did not trigger undefined
behavior.
Bart.
