Page MenuHomePhabricator

[Support] Added overflow checking add, sub and mul.
ClosedPublic

Authored by nand on Jul 30 2019, 5:38 PM.

Details

Summary

Added AddOverflow, SubOverflow and MulOverflow to compute truncated results and return a flag indicating whether overflow occured.

Diff Detail

Event Timeline

nand created this revision.Jul 30 2019, 5:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2019, 5:38 PM
jfb added a comment.Jul 30 2019, 8:18 PM

Can you confirm that the tests pass with UBSan enabled?

llvm/include/llvm/Support/MathExtras.h
862

I think you want to have T1 X, T2 Y and enable_if::<is_same<T1, T2>::value since you're trying to avoid promotions entirely.

I'd also rather return a pair or a struct with named values instead of having a reference return value, and make the result nodiscard.

870

We have bit_cast in ADT/bit.h if that's what you meant to use. I think you meant static_cast thought :)

llvm/unittests/Support/MathExtrasTest.cpp
474

You probably want signed char instead of char.

lebedev.ri added inline comments.
llvm/include/llvm/Support/MathExtras.h
783–786

Do you want to also refactor the functions that already used that checking at the same time?

nand marked 4 inline comments as done.Jul 31 2019, 7:00 AM
nand added inline comments.
llvm/include/llvm/Support/MathExtras.h
783–786

Unfortunately, I would like to avoid any refactoring in this diff.

862

I would like to keep these functions simple so at some point we might make them straightforward wrappers over __builtin_add_overflow etc, reason why I would like to keep this signature. Promotions can be handled on the callee's side. For my use case, I always want to explicitly specify the template parameter type.

Regarding struct returns, that sounds like a use case for a class such as CheckedInt/SafeInt etc, which doesn't seem to be required yet. A future implementation might use these methods though.

870

If the sizes match, doesn't static cast simply move the number bit by bit to the result? I have adjusted the comment.

nand updated this revision to Diff 212573.Jul 31 2019, 7:01 AM
nand marked an inline comment as done.

Updated comments about bitcast

nand added a comment.Jul 31 2019, 7:40 AM

Just checked: tests pass with UBSan.

nand updated this revision to Diff 212597.Jul 31 2019, 9:17 AM

s/char/signed char/

nand marked 2 inline comments as done.Jul 31 2019, 9:17 AM

replaced char with signed char

jfb added inline comments.Jul 31 2019, 10:03 AM
llvm/include/llvm/Support/MathExtras.h
862

OK seeing this as the base helper SGTM.

You probably still want nodiscard however.

nand marked an inline comment as done.Jul 31 2019, 10:10 AM
nand added inline comments.
llvm/include/llvm/Support/MathExtras.h
862

This method is the only way to compute the result of overflow - if we know overflow happens, we might not want to look at the return flag at all.

jfb accepted this revision.Jul 31 2019, 10:13 AM
jfb added inline comments.
llvm/include/llvm/Support/MathExtras.h
862

That sounds good!

This revision is now accepted and ready to land.Jul 31 2019, 10:13 AM