| Re: [hatari-devel] fixing errors reported by GCC 10 -fanalyzer |
[ Thread Index |
Date Index
| More lists.tuxfamily.org/hatari-devel Archives
]
Hi,
On 5/11/20 12:53 PM, Nicolas Pomarède wrote:
Le 11/05/2020 à 11:36, Eero Tamminen a écrit :
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.
By logic error I mean "this code didn't work as
it should, all bets are now off". You shouldn't
continue at that point, after that point things
will get only progressively harder to debug.
If assert is on a condition that can happen and
from which you can recover, you shouldn't use an
assert.
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).
They are there to guard against new changes
accidentally breaking old code. Years later you
might not anymore remember exactly how it was
supposed to work. For example profiling call
graph collection is of that complexity.
Assert could in theory be triggered also by
something (e.g. array overwrite) corrupting
Hatari's internal state.
You could think of an assert as a unit test.
You don't remove unit tests after they stop
triggering.
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.
Asserts shouldn't be used for a potential errors,
but for "can't happen" things. While they should
be enabled for all development builds, disabling
them for releases could be OK as code changes in
those should have already been tested.
In releases they might still catch memory corruption issues though, e.g.
due to different
build environments, so it would still be
preferable for them to be enabled in releases.
(On normal Linux, malloc failure is "can't happen", but as I already
mentioned, code location isn't relevant for them, and on other platforms
they may happen, so wrapper is better for them.)
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".
- Eero