Re: [hatari-devel] fixing errors reported by GCC 10 -fanalyzer

[Nicolas Pomarède <npomarede@xxxxxxxxxxxx>]

Le 11/05/2020 à 11:36, Eero Tamminen a écrit :
> Hi,
>
> On 5/11/20 8:08 AM, Thomas Huth wrote:
> > Am Sun, 10 May 2020 23:19:24 +0200
> > schrieb Nicolas Pomarède <npomarede@xxxxxxxxxxxx>:
> ...
> > > NOTE : assert() is used in several places ; as the condition/error
> > > tested by assert() can also happen when assert is compiled to no-code
> > > "{}", maybe assert() should be replaced by proper printf+exit (or
> > > return if possible) ?
>
> Only reason to disable asserts should be somebody trying to
> make very small Hatari binary for something like Firebee.
>
> There aren't so many of them that they would increase code
> size significantly (I hope), and they (or allocs) shouldn't
> be in performance critical places.
> (Except maybe the one assert in newcpu.c::do_interrupt()
> and couple in cycInt.c).
>
>
> Asserts are there to catch unexpected logic errors.  Although they 
> should be enabled by
> default, I think it's OK do disable them in
> corner-cases.
>
>
> > Actually, I think the best thing that we can do, is to introduce
> > another wrapper around malloc() in our code base, that prints an error
> > message and exits the program completely in case of errors, and we then
> > should use that wrapper for allocating small amounts of memory instead
> > of using malloc + error handling everywhere around in the code.
>
> The point of asserts is to show places where logic
> errors happen. Malloc failure isn't a logic error,
> so I think hiding the error location with
> a wrapper is fine.

I think logic errors are better handled by a real printf + possible 
return value to try to continue anyway instead of exiting the program.

If someone is not sure about the logic of his code, assert() can be 
used, but IMO they should be removed later when logic is fixed.
Keeping assert() in years old code in case logic is still wrong looks 
bad programing to me :) (it's not about the few bytes of code assert adds).

Also, how do we know these assert() won't be removed by some linux 
distro or windows/mac binaries, depending on global compiler flags we 
don't control ? And instead of catching a possible error with assert(), 
the program will just continue and maybe crash later.

That's why I prefer having custom messages in case of unexpected 
situation that users can report (assert message don't always have all 
the details to find the cause).

There're approx 40 assert() in hatari code ; for example in video.c :

assert(OVERSCAN_TOP < MAX_OVERSCAN_BOTTOM)

This might cause issue if this condition is met, but in that case I 
think the assert should be removed and a proper review of the code's 
logic should be made to ensure this condition is not possible (in that 
case, it's not possible). If that assert is met, then memory was 
certainly corrupted at other place in case "OVERSCAN_TOP >= 
MAX_OVERSCAN_BOTTOM".

Nicolas