Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

overly careful numeric casts #120

Open
z33ky opened this issue Jan 13, 2018 · 2 comments
Open

overly careful numeric casts #120

z33ky opened this issue Jan 13, 2018 · 2 comments
Assignees
Projects

Comments

@z33ky
Copy link
Collaborator

z33ky commented Jan 13, 2018

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
Copy link
Member

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 z33ky self-assigned this Jan 23, 2018
@z33ky
Copy link
Collaborator Author

z33ky commented Jan 30, 2018

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.

@Quaker762 Quaker762 added this to To do in TODO Feb 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
TODO
  
To do
Development

No branches or pull requests

2 participants