Re: [hatari-devel] undefined behaviour fixes

[Thomas Huth <th.huth@xxxxxxxxx>]

Am Sun, 12 Jan 2025 09:20:58 +0100
schrieb Andreas Grabher <andreas_g86@xxxxxxxxxx>:

> > Am 11.01.2025 um 08:36 schrieb Thomas Huth <th.huth@xxxxxxxxx>:
> > 
> > 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
> >   
> 
> Your fix looks good. That is certainly the most readable way to fix it. But you might want to check this for performance issues. I am not sure if the compiler will optimise this to using shifts. Division would be quite a bit slower than a shifting operation.
> 
> Alternatively you can have a look at the appended patch. I also checked other places, where signed data types are shifted and fixed those too. I am quite sure my patch will have the same performance as the existing code.

 Hi Andreas,

thanks, but I'm not sure whether that patch is 100% correct ... there are
also some spots that shift values to the right, and there it makes a
difference whether the value is signed or not ...

Anyway, let's only change things here where the undefined behavior
sanitizer really complains. I now went with the multiplications instead,
since the "/*  64*y0  */" comment was talking about multiplications anyway
already - so these comments are now not necessary anymore.

 Thomas