Paolo Bonzini <pbonz...@redhat.com> writes: > On 04/11/2015 15:07, Markus Armbruster wrote: >> Paolo Bonzini <pbonz...@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.