Re: [hatari-devel] undefined behaviour fixes

[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.
>
> >
> >> On Fri, Jan 10, 2025 at 11:37 PM Thomas Huth <th.huth@xxxxxxxxx> wrote:
> >>>
> >>> Am Sat, 11 Jan 2025 07:30:35 +0000
> >>> schrieb Thomas Huth <th.huth@xxxxxxxxx>:
> >>> ...
> >>>>
> >>>>> Because I still think it is bad coding style to shift negative values and similar in my opinion we should only set -fwrapv where really necessary. So we should probably only do that in the CPU core, like in my previous patch. As I said previously, the parts of Hatari that are used in Previous including the DSP do not trigger any warning. So Hatari is aside from the CPU core likely to be clean and should stay like that.
> >>>>
> >>>> It's not. There is at least one more spot in the sound code:
> >>>>
> >>>>   src/sound.c:389:18: runtime error: left shift of negative value -893
> >>>>
> >>>> Maybe Nicolas could have a look at it? ... those filters
> >>>> are not really my turf.
> >>>
> >>> I guess the fix would be as easy as:
> >>>
> >>> diff --git a/src/sound.c b/src/sound.c
> >>> --- a/src/sound.c
> >>> +++ b/src/sound.c
> >>> @@ -386,8 +386,8 @@ ymsample    Subsonic_IIR_HPF_Left(ymsample x0)
> >>>          if ( YM2149_HPF_Filter == YM2149_HPF_FILTER_NONE )
> >>>                  return x0;
> >>>
> >>> -       y1 += ((x0 - x1)<<15) - (y0<<6);  /*  64*y0  */
> >>> -       y0 = y1>>15;
> >>> +       y1 += ((x0 - x1) * 32768) - (y0 * 64);  /*  64*y0  */
> >>> +       y0 = y1 / 32768;
> >>>          x1 = x0;
> >>>
> >>>          return y0;
> >>>
> >>> ?
> >>>
> >>>   Thomas
> >>>
> >>>
> >>
> >>
> >
> >
>
>
>