tricore: fix runtime errors of integer overflow #2204
Conversation
imbillow
force-pushed
the
tc-fix-ub
branch
2 times, most recently
from
bca5f69 to
f811f2c
Compare
There was a problem hiding this comment.
Generally I approve this. Though I am not sure if the safe_math library is not a little too much.
I guess, it makes sense for TriCore to use it, because the LLVM implementation is not part of the LLVM-project and has therefore lower quality standards.
Though for archs of the officially supported archs we can be fairly certain that they handled these things.
So the library can't be used for them.
And even if we decide to replace every arithmetic operation in ARM, AArch64 etc. with the safe version, we would need to implement an additional automation step. It will take quite some work.
So the main question is, is it worth to add all of safe_math just for two or three archs? I tend to think no.
I would propose two options:
- Minimize the amount of code added: Maybe we can keep only the functions which are needed for TriCore and add a comment in the
safe_mathfiles where to get get the other functions if needed. - (My favorite). TriCore has a max. instructions width of 32bit, right? If yes, we can just run every byte sequence (
0x0 - 0xffffffff) once on this code here (withsafe_math). On my machine this takes roughly 5h.
If, after an overflow, CS aborts,you can implement a check for this specific case.
After all cases are fixed, we can removesafe_mathagain, because all the cases where it overflows are handled. Would this work?
| int64_t disp = MCOperand_getImm(MO); | ||
| int64_t res = (int64_t)(MI->address) + | ||
| ((0b111111111111111111111111111 << 5) | | ||
| (to_u32(disp) << 1)); |
There was a problem hiding this comment.
Run clang-format please, this has spaces in it.
There was a problem hiding this comment.
I've already run clang-format, even though I'm using version 17.0.1.
|
LGTM, thanks! Merged. |
closes #2188
Also added SafeInt(https://github.com/dcleblanc/SafeInt) library for overflow checking, it's a header file ONLY library so I guess it should be fine?