search

Palm-Studios / sh3redux

SILENT HILL 3 Engine Remake in OpenGL and C++
GNU General Public License v3.0
164 stars 15 forks source link

overly careful numeric casts #120

Open z33ky opened 6 years ago

z33ky commented 6 years ago

I introduced a couple of compile-time and run-time range checks that make sure that numeric casts work as expected. While it's certainly useful to have these for debug builds, they are only really needed to verify input, i.e. making sure data-offsets specified in file headers are in range. And there they shouldn't just be ASSERTs IMO, but regular checks done in any build. For debug-builds we could also rely on UBSan (clang can check for unsigned over flow as well) instead of checking ourselves, which would make the code more readable. We should a build-option for that.

This is unrelated, but generally options to enable the different sanitizers might be nice. Sure, you can do that via CMAKE_{CXX,{EXE,SHARED,STATIC}_LINKER}_FLAGS, but I think it'd be easier to just have a SANITIZERS variable or something. Then we can also specifically enable unsigned-integer-overflow and also the nullability-* options for UBSan.

Quaker762 commented 6 years ago

Sounds good to me, it'll probably help us find and debug any weird arc issues (if we encounter any more of them, considering we've hit a few I imagine there could be more).

Did you want to get started on this? I imagine you have more of an idea of what to do than me.

z33ky commented 6 years ago

The undefined behavior sanitizer warns on overflow in operations, but "overflow" when casting is something different and not handled by the sanitizer: https://bugs.llvm.org/show_bug.cgi?id=21530 We can use Boost.NumericConversion for that though. Our compiler warnings already tell us about implicit casts, but we will have to ensure that static_cast is not used in favor of numeric_cast then.