[clang] [compiler-rt] [Sanitizer] add signed-integer-wrap sanitizer (PR #80089)

Fangrui Song via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 20 13:16:00 PST 2024


MaskRay wrote:

> > I believe we can move forward by reusing `-fsanitize=signed-integer-overflow`, which adds least complexity to Clang and is very reasonable.
> 
> I see a few problems with changing `-fsanitize=signed-integer-overflow`:
> 
> 1. Clang no longer matches GCC's SIO functionality
> 2. Existing codebases (albeit, very few) may be affected by what is essentially a breaking change.
> 
> Should a compiler not at least put some effort into properly representing the semantics at hand?

While I understand adding a new way (to do similar things instead of improving the existing feature) can be tempting, as it avoids "disrupting" current users,
it can lead to software bloat and increased complexity for everyone.
There would be more options for users to read,. Enhancement to one mode might not be ported to the other (similar) one.

I feel that "not breaking existing users" is sometimes exaggerated to argue for not improving an existing feature.
Frankly, we provide ways for users to unblock them. And in this case it seems quite trivial. Add `-fno-sanitize=signed-integer-overflow` or drop the positive option.

I noticed in CodeSearch that this BUILD.gn seems used quite a bit. The author seems aware that with overflow checks, -fwrapv would not make sense.
```
+    if (is_clang && !is_ubsan && !is_ubsan_security) {
+      cflags += [ "-fwrapv" ]
+    }
```

> 3. The C spec language semantics all hint at `signed-integer-overflow` being a misnomer for the properties of the arithmetic we are sanitizing when `-fwrapv` is enabled. (really, `unsigned-integer-overflow` is also poorly named as unsigned arithmetic can't overflow [6.2.5.9](https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf))

If there are semantics improvement, unrelated to "let not break users for -fwrapv", I think a new mode is still worth considering.
I haven't read this patch closely, but I believe the patch's intention is just to introduce a new mode working with -fwrapv for the Linux kernel, at the cost of Clang complexity.

> @MaskRay, there's been lots of good review on this PR with folks more or less liking the direction of it. I'd like to find some common ground on this so we can move it forward. If you really think changing the SIO sanitizer is the way to go I'll probably close this PR and open a new one as it represents a wholly different idea.

I agree that a new PR could be useful.


https://github.com/llvm/llvm-project/pull/80089


More information about the cfe-commits mailing list