GNOME Bugzilla – Bug 756903
Add some helpers for bounds-checked unsigned integer arithemtic
Last modified: 2015-10-27 14:24:03 UTC
GCC5 recently gained support for compiler intrinsics to do efficient bounds-checked arithmetic and Clang has had the same for a while. Meanwhile, it would be nice if we could put an end to doubts about the 'best way' to write bounds-checked integer math code. These macros should help with both of those things and will be used in an upcoming patch to add support for a bounds-checked getter to GBytes.
Created attachment 313806 [details] [review] macros: add dummy __has_builtin() Add a dummy definition for Clang's __has_builtin() macro. This will allow us to use __has_builtin() unconditionally, in the same way as we already do for __has_feature().
Created attachment 313807 [details] [review] GLib: add bounds-checked unsigned int arithmetic Add some helpers for builds-checked unsigned integer arithmetic to GLib. These will be based on compiler intrinsics where they are available, falling back to standard manual checks otherwise. The fallback case needs to be implemented as a function (which we do inline) because we cannot rely on statement expressions. We also implement the intrinsics case as an inline in order to avoid people accidentally writing non-portable code which depends on static evaluation of the builtin. For now there is only support for addition and multiplication for guint, guint64 and gsize. It may make sense to add support for subtraction or for the signed equivalents of those types in the future if we find a use for that.
Created attachment 313808 [details] [review] tests: test bounds-checked int arithmetic Add some simple testcases for the new bounds-checked integer arithmetic helpers. Include a second build of the testcase to make sure we test the fallback code even if we are on a compiler that supports the intrinsics.
See also: bug 503096, which adds a bunch of inline functions with fallbacks.
To be fair, I kind of like the approach in bug 503096 the most, even if it adds a metric ton of symbols.
(In reply to Emmanuele Bassi (:ebassi) from comment #5) > To be fair, I kind of like the approach in bug 503096 the most Which part of it exactly? Looking function-y rather than macro-y? Supporting both int and uint? Having int and long versions in addition to int32 and int64 versions? Having clamped versions in addition to the give-the-wrong-answer-and-return-false versions? Having subtraction support? (I'd say "don't care", "probably want", "probably don't want", "don't know", and "don't know", respectively.)
Review of attachment 313806 [details] [review]: This seems useful in general.
Review of attachment 313807 [details] [review]: Pending working out whether to take the approach in this bug or in bug #503096, this patch looks good to me.
Review of attachment 313808 [details] [review]: Also looks fine to me.
(In reply to Dan Winship from comment #6) > (In reply to Emmanuele Bassi (:ebassi) from comment #5) > > To be fair, I kind of like the approach in bug 503096 the most > > Which part of it exactly? Looking function-y rather than macro-y? Supporting > both int and uint? Having int and long versions in addition to int32 and > int64 versions? Having clamped versions in addition to the > give-the-wrong-answer-and-return-false versions? Having subtraction support? All of them. :-) - Type safe inlined functions instead of macros: ✓ (I want the compiler to kick me if I'm doing something stupid) - Supporting signed and unsigned integers: ✓ - Supporting more types: ✓ (though this is the one thing I can live without, as long as we have support for sized integers) - Safe clamped versions: ✓ (saves a branch to handle failure) - More arithmetic operators: ✓ (I'll admit that overflowing addition is rarer, but still very much possible in existing code, see bug 755514)
(In reply to Emmanuele Bassi (:ebassi) from comment #10) > - Safe clamped versions: ✓ (saves a branch to handle failure) What is the use case? In the GBytes example, overflow is an error, and we want to return NULL in that case. And clamping seems wrong in bug 755514 too; if x*y>MAXUINT, then you just can't represent that object, and you should bail out, not allocate a buffer that isn't actually large enough. Right?
Let's unify the discussion on this topic. It's my fault that I opened this without checking existing reports. *** This bug has been marked as a duplicate of bug 503096 ***