[Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Paolo Bonzini]

This is reported by Coverity.  The algorithm description at
ftp://ftp.icm.edu.pl/packages/ggi/doc/hw/sparc/Sparc.pdf suggests
that the 32-bit parts of rs2, after the left shift, is treated
as a 64-bit integer.  Bits 32 and above are used to do the
saturating truncation.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-sparc/vis_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c
index 383cc8b..45fc7db 100644
--- a/target-sparc/vis_helper.c
+++ b/target-sparc/vis_helper.c
@@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr, uint64_t rs2)
     for (word = 0; word < 2; word++) {
         uint32_t val;
         int32_t src = rs2 >> (word * 32);
-        int64_t scaled = src << scale;
+        int64_t scaled = (int64_t)src << scale;
         int64_t from_fixed = scaled >> 16;
 
         val = (from_fixed < -32768 ? -32768 :
-- 
2.5.0

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Peter Maydell]

On 2 November 2015 at 14:05, Paolo Bonzini <pbonzini@redhat.com> wrote:
> This is reported by Coverity.  The algorithm description at
> ftp://ftp.icm.edu.pl/packages/ggi/doc/hw/sparc/Sparc.pdf suggests
> that the 32-bit parts of rs2, after the left shift, is treated
> as a 64-bit integer.  Bits 32 and above are used to do the
> saturating truncation.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target-sparc/vis_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c
> index 383cc8b..45fc7db 100644
> --- a/target-sparc/vis_helper.c
> +++ b/target-sparc/vis_helper.c
> @@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr, uint64_t rs2)
>      for (word = 0; word < 2; word++) {
>          uint32_t val;
>          int32_t src = rs2 >> (word * 32);
> -        int64_t scaled = src << scale;
> +        int64_t scaled = (int64_t)src << scale;
>          int64_t from_fixed = scaled >> 16;

This will now shift left into the sign bit of a signed integer,
which is undefined behaviour.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Paolo Bonzini]

On 02/11/2015 15:09, Peter Maydell wrote:
>> > diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c
>> > index 383cc8b..45fc7db 100644
>> > --- a/target-sparc/vis_helper.c
>> > +++ b/target-sparc/vis_helper.c
>> > @@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr, uint64_t rs2)
>> >      for (word = 0; word < 2; word++) {
>> >          uint32_t val;
>> >          int32_t src = rs2 >> (word * 32);
>> > -        int64_t scaled = src << scale;
>> > +        int64_t scaled = (int64_t)src << scale;
>> >          int64_t from_fixed = scaled >> 16;
> This will now shift left into the sign bit of a signed integer,
> which is undefined behaviour.

Why "now"?  It would have done the same before.

Paolo

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Peter Maydell]

On 2 November 2015 at 14:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 02/11/2015 15:09, Peter Maydell wrote:
>>> > diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c
>>> > index 383cc8b..45fc7db 100644
>>> > --- a/target-sparc/vis_helper.c
>>> > +++ b/target-sparc/vis_helper.c
>>> > @@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr, uint64_t rs2)
>>> >      for (word = 0; word < 2; word++) {
>>> >          uint32_t val;
>>> >          int32_t src = rs2 >> (word * 32);
>>> > -        int64_t scaled = src << scale;
>>> > +        int64_t scaled = (int64_t)src << scale;
>>> >          int64_t from_fixed = scaled >> 16;
>> This will now shift left into the sign bit of a signed integer,
>> which is undefined behaviour.
>
> Why "now"?  It would have done the same before.

True, but I was reviewing the new code rather than the
code you were taking away :-)

Incidentally, that manual says the fpackfix and fpack32 insns
use a 4 bit GSR.scale_factor value, but our code is masking
by 0x1f in helper_fpack32 and helper_fpackfix. Which is right?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Paolo Bonzini]

On 02/11/2015 16:13, Peter Maydell wrote:
> On 2 November 2015 at 14:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 02/11/2015 15:09, Peter Maydell wrote:
>>>>> diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c
>>>>> index 383cc8b..45fc7db 100644
>>>>> --- a/target-sparc/vis_helper.c
>>>>> +++ b/target-sparc/vis_helper.c
>>>>> @@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr, uint64_t rs2)
>>>>>      for (word = 0; word < 2; word++) {
>>>>>          uint32_t val;
>>>>>          int32_t src = rs2 >> (word * 32);
>>>>> -        int64_t scaled = src << scale;
>>>>> +        int64_t scaled = (int64_t)src << scale;
>>>>>          int64_t from_fixed = scaled >> 16;
>>> This will now shift left into the sign bit of a signed integer,
>>> which is undefined behaviour.
>>
>> Why "now"?  It would have done the same before.
> 
> True, but I was reviewing the new code rather than the
> code you were taking away :-)
> 
> Incidentally, that manual says the fpackfix and fpack32 insns
> use a 4 bit GSR.scale_factor value, but our code is masking
> by 0x1f in helper_fpack32 and helper_fpackfix. Which is right?

I don't know... That manual even says that GSR has no bit defined above
bit 6 (where scale_factor is 3 to 6).

It's possible that a newer processor defines a 5-bit scale factor; I
honestly have no idea.

Paolo

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Richard Henderson]

On 11/02/2015 04:13 PM, Peter Maydell wrote:
> On 2 November 2015 at 14:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 02/11/2015 15:09, Peter Maydell wrote:
>>>>> diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c
>>>>> index 383cc8b..45fc7db 100644
>>>>> --- a/target-sparc/vis_helper.c
>>>>> +++ b/target-sparc/vis_helper.c
>>>>> @@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr, uint64_t rs2)
>>>>>       for (word = 0; word < 2; word++) {
>>>>>           uint32_t val;
>>>>>           int32_t src = rs2 >> (word * 32);
>>>>> -        int64_t scaled = src << scale;
>>>>> +        int64_t scaled = (int64_t)src << scale;
>>>>>           int64_t from_fixed = scaled >> 16;
>>> This will now shift left into the sign bit of a signed integer,
>>> which is undefined behaviour.
>>
>> Why "now"?  It would have done the same before.
>
> True, but I was reviewing the new code rather than the
> code you were taking away :-)
>
> Incidentally, that manual says the fpackfix and fpack32 insns
> use a 4 bit GSR.scale_factor value, but our code is masking
> by 0x1f in helper_fpack32 and helper_fpackfix. Which is right?

The 2011 manual has 5 bits for fpack32 and fpackfix; fpack16 uses only 4 bits.

I do think we'd be better served by casting to uint64_t on that line.  Note 
that fpackfix requires the same correction.  And it wouldn't hurt to cast to 
uint32_t in fpack16, lest we anger the self-same shifting gods.


r~

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Paolo Bonzini]

On 04/11/2015 11:12, Richard Henderson wrote:
> On 11/02/2015 04:13 PM, Peter Maydell wrote:
>> On 2 November 2015 at 14:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>>
>>> On 02/11/2015 15:09, Peter Maydell wrote:
>>>>>> diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c
>>>>>> index 383cc8b..45fc7db 100644
>>>>>> --- a/target-sparc/vis_helper.c
>>>>>> +++ b/target-sparc/vis_helper.c
>>>>>> @@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr,
>>>>>> uint64_t rs2)
>>>>>>       for (word = 0; word < 2; word++) {
>>>>>>           uint32_t val;
>>>>>>           int32_t src = rs2 >> (word * 32);
>>>>>> -        int64_t scaled = src << scale;
>>>>>> +        int64_t scaled = (int64_t)src << scale;
>>>>>>           int64_t from_fixed = scaled >> 16;
>>>> This will now shift left into the sign bit of a signed integer,
>>>> which is undefined behaviour.
>>>
>>> Why "now"?  It would have done the same before.
>>
>> True, but I was reviewing the new code rather than the
>> code you were taking away :-)
>>
>> Incidentally, that manual says the fpackfix and fpack32 insns
>> use a 4 bit GSR.scale_factor value, but our code is masking
>> by 0x1f in helper_fpack32 and helper_fpackfix. Which is right?
> 
> The 2011 manual has 5 bits for fpack32 and fpackfix; fpack16 uses only 4
> bits.
> 
> I do think we'd be better served by casting to uint64_t on that line. 
> Note that fpackfix requires the same correction.  And it wouldn't hurt
> to cast to uint32_t in fpack16, lest we anger the self-same shifting gods.

Hmmm.. say src = -0x80000000, scale = 1;

scaled     = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000
from_fixed = 0xffffffff00000000 >> 16  = 0x0000ffffffff0000

Now from_fixed is positive and you get 32767 instead of -32768.  In
other words, we would have to cast to uint64_t on the scaled assignment,
and back to int64_t on the from_fixed assignment.  I must be
misunderstanding your suggestion.

Paolo

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Richard Henderson]

On 11/04/2015 11:45 AM, Paolo Bonzini wrote:
>>>>>>>            int32_t src = rs2 >> (word * 32);
>>>>>>> -        int64_t scaled = src << scale;
>>>>>>> +        int64_t scaled = (int64_t)src << scale;
>>>>>>>            int64_t from_fixed = scaled >> 16;
...
>>
>> I do think we'd be better served by casting to uint64_t on that line.
>> Note that fpackfix requires the same correction.  And it wouldn't hurt
>> to cast to uint32_t in fpack16, lest we anger the self-same shifting gods.
>
> Hmmm.. say src = -0x80000000, scale = 1;
>
> scaled     = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000
> from_fixed = 0xffffffff00000000 >> 16  = 0x0000ffffffff0000
>
> Now from_fixed is positive and you get 32767 instead of -32768.  In
> other words, we would have to cast to uint64_t on the scaled assignment,
> and back to int64_t on the from_fixed assignment.  I must be
> misunderstanding your suggestion.

   int64_t scaled = (uint64_t)src << scale;

I.e. one explicit conversion and one implicit conversion.


r~

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Paolo Bonzini]

On 04/11/2015 12:05, Richard Henderson wrote:
> On 11/04/2015 11:45 AM, Paolo Bonzini wrote:
>>>>>>>>            int32_t src = rs2 >> (word * 32);
>>>>>>>> -        int64_t scaled = src << scale;
>>>>>>>> +        int64_t scaled = (int64_t)src << scale;
>>>>>>>>            int64_t from_fixed = scaled >> 16;
> ...
>>>
>>> I do think we'd be better served by casting to uint64_t on that line.
>>> Note that fpackfix requires the same correction.  And it wouldn't hurt
>>> to cast to uint32_t in fpack16, lest we anger the self-same shifting
>>> gods.
>>
>> Hmmm.. say src = -0x80000000, scale = 1;
>>
>> scaled     = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000
>> from_fixed = 0xffffffff00000000 >> 16  = 0x0000ffffffff0000
>>
>> Now from_fixed is positive and you get 32767 instead of -32768.  In
>> other words, we would have to cast to uint64_t on the scaled assignment,
>> and back to int64_t on the from_fixed assignment.  I must be
>> misunderstanding your suggestion.
> 
>   int64_t scaled = (uint64_t)src << scale;
> 
> I.e. one explicit conversion and one implicit conversion.

That does the job, but it also does look like a typo...

Paolo

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 04/11/2015 12:05, Richard Henderson wrote:
>> On 11/04/2015 11:45 AM, Paolo Bonzini wrote:
>>>>>>>>>            int32_t src = rs2 >> (word * 32);
>>>>>>>>> -        int64_t scaled = src << scale;
>>>>>>>>> +        int64_t scaled = (int64_t)src << scale;
>>>>>>>>>            int64_t from_fixed = scaled >> 16;
>> ...
>>>>
>>>> I do think we'd be better served by casting to uint64_t on that line.
>>>> Note that fpackfix requires the same correction.  And it wouldn't hurt
>>>> to cast to uint32_t in fpack16, lest we anger the self-same shifting
>>>> gods.
>>>
>>> Hmmm.. say src = -0x80000000, scale = 1;
>>>
>>> scaled     = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000
>>> from_fixed = 0xffffffff00000000 >> 16  = 0x0000ffffffff0000
>>>
>>> Now from_fixed is positive and you get 32767 instead of -32768.  In
>>> other words, we would have to cast to uint64_t on the scaled assignment,
>>> and back to int64_t on the from_fixed assignment.  I must be
>>> misunderstanding your suggestion.
>> 
>>   int64_t scaled = (uint64_t)src << scale;
>> 
>> I.e. one explicit conversion and one implicit conversion.
>
> That does the job, but it also does look like a typo...

Make the implicit conversion explicit then.

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Paolo Bonzini]

On 04/11/2015 15:07, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 04/11/2015 12:05, Richard Henderson wrote:
>>> On 11/04/2015 11:45 AM, Paolo Bonzini wrote:
>>>>>>>>>>            int32_t src = rs2 >> (word * 32);
>>>>>>>>>> -        int64_t scaled = src << scale;
>>>>>>>>>> +        int64_t scaled = (int64_t)src << scale;
>>>>>>>>>>            int64_t from_fixed = scaled >> 16;
>>> ...
>>>>>
>>>>> I do think we'd be better served by casting to uint64_t on that line.
>>>>> Note that fpackfix requires the same correction.  And it wouldn't hurt
>>>>> to cast to uint32_t in fpack16, lest we anger the self-same shifting
>>>>> gods.
>>>>
>>>> Hmmm.. say src = -0x80000000, scale = 1;
>>>>
>>>> scaled     = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000
>>>> from_fixed = 0xffffffff00000000 >> 16  = 0x0000ffffffff0000
>>>>
>>>> Now from_fixed is positive and you get 32767 instead of -32768.  In
>>>> other words, we would have to cast to uint64_t on the scaled assignment,
>>>> and back to int64_t on the from_fixed assignment.  I must be
>>>> misunderstanding your suggestion.
>>>
>>>   int64_t scaled = (uint64_t)src << scale;
>>>
>>> I.e. one explicit conversion and one implicit conversion.
>>
>> That does the job, but it also does look like a typo...
> 
> Make the implicit conversion explicit then.

Sorry, but I'll say it again: there's _no way_ that a sane compiler will
_ever_ use this particular bit of undefined behavior.

I'm generally against uglifying the code to placate ubsan, but
especially so in this case: it is not common code and it would only
affect people running fpackfix under ubsan.

Paolo

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 04/11/2015 15:07, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> On 04/11/2015 12:05, Richard Henderson wrote:
>>>> On 11/04/2015 11:45 AM, Paolo Bonzini wrote:
>>>>>>>>>>>            int32_t src = rs2 >> (word * 32);
>>>>>>>>>>> -        int64_t scaled = src << scale;
>>>>>>>>>>> +        int64_t scaled = (int64_t)src << scale;
>>>>>>>>>>>            int64_t from_fixed = scaled >> 16;
>>>> ...
>>>>>>
>>>>>> I do think we'd be better served by casting to uint64_t on that line.
>>>>>> Note that fpackfix requires the same correction.  And it wouldn't hurt
>>>>>> to cast to uint32_t in fpack16, lest we anger the self-same shifting
>>>>>> gods.
>>>>>
>>>>> Hmmm.. say src = -0x80000000, scale = 1;
>>>>>
>>>>> scaled     = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000
>>>>> from_fixed = 0xffffffff00000000 >> 16  = 0x0000ffffffff0000
>>>>>
>>>>> Now from_fixed is positive and you get 32767 instead of -32768.  In
>>>>> other words, we would have to cast to uint64_t on the scaled assignment,
>>>>> and back to int64_t on the from_fixed assignment.  I must be
>>>>> misunderstanding your suggestion.
>>>>
>>>>   int64_t scaled = (uint64_t)src << scale;
>>>>
>>>> I.e. one explicit conversion and one implicit conversion.
>>>
>>> That does the job, but it also does look like a typo...
>> 
>> Make the implicit conversion explicit then.
>
> Sorry, but I'll say it again: there's _no way_ that a sane compiler will
> _ever_ use this particular bit of undefined behavior.
>
> I'm generally against uglifying the code to placate ubsan, but
> especially so in this case: it is not common code and it would only
> affect people running fpackfix under ubsan.

Oh, I don't disagree at all with "let's not uglify code".

I wish compilers hadn't become so nasty, though.  I wish they had more
respect for the imperfect real-world code they compile, and less
benchmark worship.

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Mark Cave-Ayland]

On 04/11/15 11:05, Richard Henderson wrote:

> On 11/04/2015 11:45 AM, Paolo Bonzini wrote:
>>>>>>>>            int32_t src = rs2 >> (word * 32);
>>>>>>>> -        int64_t scaled = src << scale;
>>>>>>>> +        int64_t scaled = (int64_t)src << scale;
>>>>>>>>            int64_t from_fixed = scaled >> 16;
> ...
>>>
>>> I do think we'd be better served by casting to uint64_t on that line.
>>> Note that fpackfix requires the same correction.  And it wouldn't hurt
>>> to cast to uint32_t in fpack16, lest we anger the self-same shifting
>>> gods.
>>
>> Hmmm.. say src = -0x80000000, scale = 1;
>>
>> scaled     = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000
>> from_fixed = 0xffffffff00000000 >> 16  = 0x0000ffffffff0000
>>
>> Now from_fixed is positive and you get 32767 instead of -32768.  In
>> other words, we would have to cast to uint64_t on the scaled assignment,
>> and back to int64_t on the from_fixed assignment.  I must be
>> misunderstanding your suggestion.
> 
>   int64_t scaled = (uint64_t)src << scale;
> 
> I.e. one explicit conversion and one implicit conversion.

I suspect Richard knows more about this part of SPARC emulation than I
do, so I'd be fine with a solution similar to the above if everyone
agress. Let me know if you need me to send a SPARC pull request,
although it will probably be quicker coming from Paolo/Richard at the
moment.


ATB,

Mark.

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Paolo Bonzini]

On 04/11/2015 18:53, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 04/11/2015 15:07, Markus Armbruster wrote:
>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>>
>>>> On 04/11/2015 12:05, Richard Henderson wrote:
>>>>> On 11/04/2015 11:45 AM, Paolo Bonzini wrote:
>>>>>>>>>>>>            int32_t src = rs2 >> (word * 32);
>>>>>>>>>>>> -        int64_t scaled = src << scale;
>>>>>>>>>>>> +        int64_t scaled = (int64_t)src << scale;
>>>>>>>>>>>>            int64_t from_fixed = scaled >> 16;
>>>>> ...
>>>>>>>
>>>>>>> I do think we'd be better served by casting to uint64_t on that line.
>>>>>>> Note that fpackfix requires the same correction.  And it wouldn't hurt
>>>>>>> to cast to uint32_t in fpack16, lest we anger the self-same shifting
>>>>>>> gods.
>>>>>>
>>>>>> Hmmm.. say src = -0x80000000, scale = 1;
>>>>>>
>>>>>> scaled     = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000
>>>>>> from_fixed = 0xffffffff00000000 >> 16  = 0x0000ffffffff0000
>>>>>>
>>>>>> Now from_fixed is positive and you get 32767 instead of -32768.  In
>>>>>> other words, we would have to cast to uint64_t on the scaled assignment,
>>>>>> and back to int64_t on the from_fixed assignment.  I must be
>>>>>> misunderstanding your suggestion.
>>>>>
>>>>>   int64_t scaled = (uint64_t)src << scale;
>>>>>
>>>>> I.e. one explicit conversion and one implicit conversion.
>>>>
>>>> That does the job, but it also does look like a typo...
>>>
>>> Make the implicit conversion explicit then.
>>
>> Sorry, but I'll say it again: there's _no way_ that a sane compiler will
>> _ever_ use this particular bit of undefined behavior.
>>
>> I'm generally against uglifying the code to placate ubsan, but
>> especially so in this case: it is not common code and it would only
>> affect people running fpackfix under ubsan.
> 
> Oh, I don't disagree at all with "let's not uglify code".
> 
> I wish compilers hadn't become so nasty, though.  I wish they had more
> respect for the imperfect real-world code they compile, and less
> benchmark worship.

It's not benchmark worship.  For example, being able to optimize "x * 6
/ 2" to "x * 3" is useful, but it's only possible if you can treat
integer overflow as undefined.  In fact GCC compiles it to

	leal	(%rdi,%rdi,2), %eax 	add	r0, r0, r0, lsl #1

(x86 and ARM respectively) for int, and to

	leal	(%rdi,%rdi,2), %eax	mov	r3, r0, asl #3
	andl	$2147483647, %eax	sub	r0, r3, r0, asl #1
					mov	r0, r0, lsr #1

for unsigned int.  This is less efficient; stuff like this happens in
*real programs* and even in hot loops.  For an even more extreme case,
"x * 10000000 / 1000000" with int and unsigned:

	leal	(%rdi,%rdi,4), %eax	mov	r3, r0, asl #3
	addl	%eax, %eax		add	r0, r3, r0, lsl #1

vs.

	imull	$10000000, %edi, %edx	movw	r3, #38528
	movl	$1125899907, %ecx	movw	r2, #56963
	movl	%edx, %eax		movt	r3, 152
	mull	%ecx			movt	r2, 17179
	movl	%edx, %eax		mul	r0, r3, r0
	shrl	$18, %eax		umull	r0, r1, r0, r2
					mov	r0, r1, lsr #18

(For completeness I'll note that this optimization may also _hide_ bugs
on ints, which can be both a good or a bad thing; compiler warnings and
static analysis can help fixing the code).

Similarly for optimizing

    for (i = 0; i <= n; i++)
      p[i] = 0;

to any of

    for (q = p, r = p + n; q <= r; q++)
      *q = 0;

or

    for (q = p, i = n + 1; i-- > 0; q++)
      *q = 0;

Both of which are cases of strength reduction that are expected by any
optimizing compiler (without even going into vectorization).  Yet they
are not possible without treating overflow as undefined.

The last may seem strange, but it's easy for a compiler to look at the
original loop and derive that it has n + 1 iterations.  This can be used
with hardware loop counter registers (e.g. CTR on PowerPC) or
decrement-and-loop instructions (e.g. bdnz on PowerPC, loop on x86).

DSP code, for example, is full of code like this where n is known at
compile time, and one would have to write assembly code if the compiler
didn't about these instruction.

As for the so-much-loathed type-based alias analysis, it lets you
optimize this:

    void f(float **a, float **b, int m, int n)
    {
      int i, j;
      for (i = 0; i < m; i++)
        for (j = 0; j < n; j++)
          b[i][j] = a[i][j];
    }

to this:

    void f(float **a, float **b, int m, int n)
    {
      int i, j;
      for (i = 0; i < m; i++) {
        float *ai = a[i], *bi = b[i];
        for (j = 0; j < n; j++)
          bi[j] = ai[j];
      }
    }

Compiler writers don't rely on undefined behavior out of spite.  There
_is_ careful analysis of what kind of code could be broken by it, and
typically there are also warning mechanisms (-Wstrict-aliasing,
-Wstrict-overflow, -Wunsafe-loop-optimizations) to help the programmer.

It's not a coincidence that left shifting of signed negative numbers a)
is not relied on by neither GCC nor clang; b) is the source of the wide
majority of ubsan failures for QEMU.

Paolo

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Paolo Bonzini]

On 05/11/2015 00:36, Mark Cave-Ayland wrote:
> On 04/11/15 11:05, Richard Henderson wrote:
> 
>> On 11/04/2015 11:45 AM, Paolo Bonzini wrote:
>>>>>>>>>            int32_t src = rs2 >> (word * 32);
>>>>>>>>> -        int64_t scaled = src << scale;
>>>>>>>>> +        int64_t scaled = (int64_t)src << scale;
>>>>>>>>>            int64_t from_fixed = scaled >> 16;
>> ...
>>>>
>>>> I do think we'd be better served by casting to uint64_t on that line.
>>>> Note that fpackfix requires the same correction.  And it wouldn't hurt
>>>> to cast to uint32_t in fpack16, lest we anger the self-same shifting
>>>> gods.
>>>
>>> Hmmm.. say src = -0x80000000, scale = 1;
>>>
>>> scaled     = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000
>>> from_fixed = 0xffffffff00000000 >> 16  = 0x0000ffffffff0000
>>>
>>> Now from_fixed is positive and you get 32767 instead of -32768.  In
>>> other words, we would have to cast to uint64_t on the scaled assignment,
>>> and back to int64_t on the from_fixed assignment.  I must be
>>> misunderstanding your suggestion.
>>
>>   int64_t scaled = (uint64_t)src << scale;
>>
>> I.e. one explicit conversion and one implicit conversion.
> 
> I suspect Richard knows more about this part of SPARC emulation than I
> do, so I'd be fine with a solution similar to the above if everyone
> agress. Let me know if you need me to send a SPARC pull request,
> although it will probably be quicker coming from Paolo/Richard at the
> moment.

All solutions work.  You have to tell us which you prefer among

	/* Has undefined behavior (though no compiler uses it) */
	int64_t scaled = (int64_t)src << scale;

	/* Seems like a typo */
	int64_t scaled = (uint64_t)src << scale;

	/* Ugly code */
	int64_t scaled = (uint64_t)(int64_t)src << scale;

Paolo

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Richard Henderson]

On 11/05/2015 10:12 AM, Paolo Bonzini wrote:
> 	/* Ugly code */
> 	int64_t scaled = (uint64_t)(int64_t)src << scale;

You mean

   int64_t scaled = (int64_t)((uint64_t)src << scale);


r~

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Paolo Bonzini]

On 05/11/2015 10:20, Richard Henderson wrote:
> 
>>     /* Ugly code */
>>     int64_t scaled = (uint64_t)(int64_t)src << scale;
> 
> You mean
> 
>   int64_t scaled = (int64_t)((uint64_t)src << scale);

No, that also looks like a typo.

I mean:

- unnecessary cast to int64_t to get the sign extension while avoiding
the impression of a typo

- cast to uint64_t to avoid overflow

- the shift is done in the uint64_t type

- finally there is an implicit cast to int64_t

Paolo

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Richard Henderson]

On 11/05/2015 10:25 AM, Paolo Bonzini wrote:
>
>
> On 05/11/2015 10:20, Richard Henderson wrote:
>>
>>>      /* Ugly code */
>>>      int64_t scaled = (uint64_t)(int64_t)src << scale;
>>
>> You mean
>>
>>    int64_t scaled = (int64_t)((uint64_t)src << scale);
>
> No, that also looks like a typo.
>
> I mean:
>
> - unnecessary cast to int64_t to get the sign extension while avoiding
> the impression of a typo

Huh.  This part doesn't seem a typo to me at all.

>
> - cast to uint64_t to avoid overflow
>
> - the shift is done in the uint64_t type
>
> - finally there is an implicit cast to int64_t


r~

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Paolo Bonzini]

On 05/11/2015 10:28, Richard Henderson wrote:
> On 11/05/2015 10:25 AM, Paolo Bonzini wrote:
>>
>>
>> On 05/11/2015 10:20, Richard Henderson wrote:
>>>
>>>>      /* Ugly code */
>>>>      int64_t scaled = (uint64_t)(int64_t)src << scale;
>>>
>>> You mean
>>>
>>>    int64_t scaled = (int64_t)((uint64_t)src << scale);
>>
>> No, that also looks like a typo.
>>
>> I mean:
>>
>> - unnecessary cast to int64_t to get the sign extension while avoiding
>> the impression of a typo
> 
> Huh.  This part doesn't seem a typo to me at all.

A cast _is_ obviously necessary, because src is int32_t and the result
is int64_t:

         int32_t src = rs2 >> (word * 32);
         int64_t scaled = (uint64_t)src << scale;

having uint64_t on the RHS and int64_t on the LHS definitely would be a
WTF cause for me.

Paolo

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Mark Cave-Ayland]

On 05/11/15 09:25, Paolo Bonzini wrote:

> On 05/11/2015 10:20, Richard Henderson wrote:
>>
>>>     /* Ugly code */
>>>     int64_t scaled = (uint64_t)(int64_t)src << scale;
>>
>> You mean
>>
>>   int64_t scaled = (int64_t)((uint64_t)src << scale);
> 
> No, that also looks like a typo.
> 
> I mean:
> 
> - unnecessary cast to int64_t to get the sign extension while avoiding
> the impression of a typo
> 
> - cast to uint64_t to avoid overflow
> 
> - the shift is done in the uint64_t type
> 
> - finally there is an implicit cast to int64_t

I would say that Richard's version above is the most readable to me,
however from what you're saying this would cause the compiler to produce
much less efficient code?

If this is the case then I could live with your second choice ("Seems
like a typo") with an appropriate comment if this maintains the
efficiency of generated code whilst also having well-defined behaviour
between compilers. Out of interest has anyone tried these alternatives
on clang?


ATB,

Mark.

Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Paolo Bonzini]

On 06/11/2015 16:33, Mark Cave-Ayland wrote:
>>> >>
>>>> >>>     /* Ugly code */
>>>> >>>     int64_t scaled = (uint64_t)(int64_t)src << scale;
>>> >>
>>> >> You mean
>>> >>
>>> >>   int64_t scaled = (int64_t)((uint64_t)src << scale);
>> > 
>> > No, that also looks like a typo.
>> > 
>> > I mean:
>> > 
>> > - unnecessary cast to int64_t to get the sign extension while avoiding
>> > the impression of a typo
>> > 
>> > - cast to uint64_t to avoid overflow
>> > 
>> > - the shift is done in the uint64_t type
>> > 
>> > - finally there is an implicit cast to int64_t
> I would say that Richard's version above is the most readable to me,
> however from what you're saying this would cause the compiler to produce
> much less efficient code?

No, they should all be the same.

Let's go with the "seems like a typo" version :) with a comment to say
that no, it's not a typo.

Paolo

> If this is the case then I could live with your second choice ("Seems
> like a typo") with an appropriate comment if this maintains the
> efficiency of generated code whilst also having well-defined behaviour
> between compilers. Out of interest has anyone tried these alternatives
> on clang?