dmd
dmd copied to clipboard
dlang
constfold: multiplication should check for overflow
Thanks for your pull request, @ljmf00!
Bugzilla references
| Auto-close | Bugzilla | Severity | Description |
|---|---|---|---|
| ✓ | 23934 | enhancement | Literal integer multiplication overflow is allowed in CTFE |
Testing this PR locally
If you don't have a local development environment setup, you can use Digger to test this PR:
dub run digger -- build "master + dmd#15266"
Huh? D specification says that signed integers overflow as two complement.
Yes, but perhaps, is this good to rely on? I don't think so. I didn't know we have integer overflow as defined behavior but this will easily make certain compiler implementations for architectures that raise exceptions out of spec, unless we want to make them slow. Integer overflow is very platform dependent and even though we define it (which is a bit impractical on exotic architectures), its bad to not explicitly tell we are relying on integer overflow.
This would require a language change, because currently overflow is defined behavior, and not defined as an error. The issue is invalid.
this will easily make certain compiler implementations for architectures that raise exceptions out of spec, unless we want to make them slow.
C is known to support 7-bit char, variable sizes for short int long, different signed number encodings etc. As the world converged on 8-bit bytes, and 2's complement 32-bit int, D explicitly decided to simplify and keep integers consistent. If you want to target an exotic architecture, either stick with C or use D with a compiler extension.
C has actually just dropped wording supporting anything other than two's complement arithmetic.
It's very peculiar for the compiler to not be able to warn about these cases. It would help solving issues that are too complex to detect manually.
CC @JohanEngelen maybe we can adapt this as a warning, at least in the Weka compiler. I think this is a point in favor to our compiler extension linter, along with the unused symbols. Also missing the + integer overflow.
@WalterBright is there any way to be able to have an option here? This already hurt us at Weka on compile-time constants and we would like the compiler to inform us about this. At least have this in a way its possible to lint it via like https://github.com/ljmf00/ldclint , by adding a flag to the node like "CTFE poisoned"?
I would really love to discuss ideas about this and its a topic to possibly discuss on an industry meeting @mdparker .
CC @EyalIO @JohanEngelen .
@ljmf00 Did you already check whether weka compiles with this error in? (e.g. that current code nor runtime depends on using defined overflow in ctfe calculation)
@ljmf00 Did you already check whether weka compiles with this error in? (e.g. that current code nor runtime depends on using defined overflow in ctfe calculation)
From what I've seen, druntime relies on this in some functions. There's some work to fix here. I need to check, but I won't get any progress here until we have a solid decision on how we go forward with it, because it might be worthless work time if this doesn't happen to be upstreamed. I would be happy to discuss alternatives for this other than the proposed change.
I know that @EyalIO found some issues because CTFE values were poisoned with two complement overflows but I don't have the details of how severe it is, but I assume it's at least, a very easy fix, if the compiler warns about such an obvious code smell.
I understand that an error is not valid, given what spec says. But, ultimately, the argument of saying this shouldn't be an warning because overflow is defined behavior is not very valid to me. There's literally integer overflow sanitizers for a reason and the reason to use it is certainly not to support 7-bit architecture but rather because it introduces actual potentially very severe bugs.