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

[ Thread Index | Date Index | More lists.tuxfamily.org/hatari-devel Archives ]


Am Mon, 11 May 2020 11:53:28 +0200
schrieb 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.

That's fine in situations where it makes sense to continue (e.g. when
opening a zip file failed, just continue without inserting the disk) ...
but there are also lots of places where continuing does not make that
much sense anymore, e.g. when allocation already fails during an
essential init function. If you run out of memory already there
(especially if you only tried to allocate some few bytes) - what sense
does it make to continue?

> 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.

Actually, the assert()s are already disabled in our normal builds (i.e.
the cmake "Release" builds - they set -DNDEBUG in the CFLAGS). You only
get the assert()s with --enable-debug.

> 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).

I think we all at least agree that using assert() for checking the
return value of malloc() is not a good idea.

> 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).

I think I'm with Eero here and rather would prefer to keep such
assert()s that check for logical errors (i.e. things that "should never
happen"). But we should really check for asserts that are used for
"normal" error handling, i.e. the where we use it for checking the
return value of malloc() and friends.

 Thomas



Mail converted by MHonArc 2.6.19+ http://listengine.tuxfamily.org/