Re: [hatari-devel] undefined behaviour fixes

[Thomas Huth <th.huth@xxxxxxxxx>]

Am Wed, 15 Jan 2025 15:40:07 -0800
schrieb David Savinkoff <dsavnkff@xxxxxxxxx>:

> On Sun, Jan 12, 2025 at 5:34 AM Nicolas Pomarède <npomarede@xxxxxxxxxxxx> wrote:
> >
> > Le 12/01/2025 à 13:56, Thomas Huth a écrit :  
> > > Am Sat, 11 Jan 2025 13:10:51 -0800
> > > schrieb David Savinkoff <dsavnkff@xxxxxxxxx>:
> > >  
> > >> Hi,
> > >> This filter's function call is passed an int16_t and returns an int16_t.
> > >> The filter calculates using int32_t so that there is no possibility of
> > >> shifting too much.  
> > >
> > > This is not about shifting too much, it's about shifting signed (negative)
> > > values which is undefined behavior in C.
> > >  
> > >> This is why there is no warning.  
> > >
> > > There is a warning when compiling with "-fsanitize=undefined".
> > >  
> > >> I love the 1 clock cycle barrel shifter in non-legacy CPUs.  
> > >
> > > I think modern C compilers are smart enough to convert multiplications by
> > > 32768 into the corresponding shift assembly instruction.
> > >  
> >
> > Hi
> >
> > After reading several specs / use cases here and there, it seems indeed
> > a better way to not use shift operators when the involved variable is
> > negative.
> >
> > C specs warns about several "undefined" behaviour that we can't handle
> > in our code, but on the other hand any modern C compiler should know
> > when a multiplication / division is a power of 2 and adjust the
> > resulting asm code to use arithmetic shift if the variable is signed.
> >
> > So, I'm also in favor of dropping << / >> in sound.c ; it will certainly
> > be optimised to "ASR/ASL" equivalent by the compiler, and even if not,
> > this part in sound.c is rather small performance-wise compared to the
> > whole cpu and video emulation, so I'm not even sure loss of performance
> > here would be visible in the end.
> >
> > Nicolas
> >  
> 
> Hi,
> Using signed multiplies in this code allows this code to work with
> one's complement, and signed magnitude CPUs. I thought Hatari
> is small endian / big endian, and two's complement only.
> Note that signed integer, arithmetic left/right shifting of negative
> numbers is fully supported with two's complement math.

 Hi David,

the point is that shifting negative numbers is undefined behavior according
to the C standard. So theoretically, the compiler is allowed to do anything
here, like optimizing the code away. Fortunately, no modern compiler is
doing this right now, but to be on the save side, it's better to avoid this
pattern in the code.

 Thomas