Hello, On Sat, Apr 22, 2017 at 11:46 AM, Nicolai Hähnle <nhaeh...@gmail.com> wrote: > Definitely split the changes up along a combination of where they are in the > source and what types of errors they are. I'd say the split you've chosen > looks reasonable to me.
Excellent, thanks. >> 1. the addition of a check to malloc return value; this was rather >> trivial per se, but for example while going to through the source code I >> noticed that for _reallocs_ the ‘safe’ approach is generally not >> followed (store the old pointer before the realloc so that if the >> realloc fails we don't get a memory leak), and I was wondering if a >> patchset to fortify `realloc()` would be worth it (especially >> considering that there's mostly small reallocs and if they fail the >> situation is catastrophic enough that the memory leak would be the last >> of the problems). > > I'm generally in favor of fixing our use of realloc. I occasionally do that > in code that I'm working on anyway. Good, then I'll try to spin a patchset just for the realloc safety. > Well, you've more or less already explained the difference: the asset + > return will actually return something possibly meaningful in optimized > builds, whereas with unreachable, almost anything might happen: it's > basically at the whim of the compiler. > > For example: In the case of large switch statements that end up being > lowered into jump tables, the unreachable will probably end up jumping to an > arbitrary address and likely crash, while for small switch statements, an > arbitrary non-unreachable path is likely to be executed. > > For the most part, I think the use of assert + return is there because the > code was written at a time when unreachable builtins simply didn't exist. > I'm generally in favor of using unreachable; it contains an assert, so > testing on builds with assertions enabled will catch them. > > I have one remark on patch 3. Patches 1,2,4 are: > > Reviewed-by: Nicolai Hähnle <nicolai.haeh...@amd.com> Thanks for the clarification and the review. I'll proceed with the unreachable() then if I find more situations such as this, and then individual cases where it might be appropriate can be weeded out in review. Best regards, -- Giuseppe "Oblomov" Bilotta _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
