Hi David, That's a nice work, thanks!
On 08/02/15 21:17, David Grayson wrote: > Hello. Attached is version 2.0.0 of the patch, which is very > different and only supports GCC 5 and above, because it uses new > built-in functions. This version is only 331 lines long (down from > ~1600). It is easy for anyone to check it because it makes no > assumptions about the sizes, signedness, or compatibility of the types > involved. > > The can also read the header and find my scripts for generating it and > testing it here: > > https://github.com/DavidEGrayson/intsafe/blob/2.0.0/generated/intsafe.h > > I asked about signed multiplication on > http://codereview.stackexchange.com/q/98791/75322 and I got a great > suggestion to use these new built-ins, which are documented here: > > https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html > > These functions were added in GCC 5. I wasn't sure if it was > important to support older versions of GCC. If it is important, we > can keep working on the previous patch (version 1.1.0) and ignore this > one. I'm not sure how important it is. The header is a new feature for mingw-w64, so it may be okay to not support older compilers (at least problem with backward compatibility is not an issue here), but I'm not sure. Kai, do you have an opinion? In any case, if we use it, we should make sure that older compilers may at least include the header. It means #ifdefing places that use those new __builtiin functions. > > These functions vastly simplify my implementation of intsafe.h because > every single intsafe.h function can be written as a simple wrapper > around __builtin_add_overflow, __builtin_sub_overflow, or > __builtin_mul_overflow. Therefore, I can generate the function bodies > with the preprocessor, making the header way shorter. > > The change to limits.h (to fix CHAR_MIN and CHAR_MAX when > -funsigned-char is provided) is no longer really needed, but I still > think it would be a nice change so I left it in this patch. > > Every function in this patch sets the result to 0 if there was an > overflow. This isn't strictly necessary. However, poorly-written > code might accidentally use the output value of an intsafe.h function > even if the function return an error code. An overflow might indicate > that your system is under attack, and we should give the attacker as > little control over the execution of the code as possible. I have a few comments to the code. diff --git a/mingw-w64-headers/crt/limits.h b/mingw-w64-headers/crt/limits.h index 73dca2a..f0145df 100644 --- a/mingw-w64-headers/crt/limits.h +++ b/mingw-w64-headers/crt/limits.h @@ -24,8 +24,13 @@ #define SCHAR_MAX 127 #define UCHAR_MAX 0xff +#ifdef __CHAR_UNSIGNED__ +#define CHAR_MIN 0 +#define CHAR_MAX UCHAR_MAX +#else #define CHAR_MIN SCHAR_MIN #define CHAR_MAX SCHAR_MAX +#endif I'm not sure about this, but it probably makes sense. IMO it would be nice to have it in a separated commit and commented by Kai. + * + * This file was auto-generated from https://github.com/DavidEGrayson/intsafe I do worry a bit about putting such comments here. As far as I understand it, once it's in the tree, we will be changing it directly in header, not modifying your scripts, is that right? This comment could suggest developer that he should regenerate it instead. Could you please rephrase it? + * + * This file is an implementation of Microsoft's intsafe.h header, which + * provides inline functions for safe integer conversions and math operations: + * + * https://msdn.microsoft.com/en-us/library/windows/desktop/ff521693 + * + * The full list of math functions is only available here: + * + * https://msdn.microsoft.com/en-us/library/windows/desktop/ff521701 We experienced in Wine that MS URLs are not permanent and disappear after some time. I'd suggest not putting them in headers. Developers will find them easily. +#include <wtypesbase.h> PSDK version doesn't include the file and instead redeclares here required types. It's probably done this way to make intsafe.h as light as possible. I'm not sure we want to follow that. +#ifndef __MINGW_INTSAFE_API +#define __MINGW_INTSAFE_API inline >From our past experience, I'd suggest FORCEINLINE. inline is not enough for >unoptimized builds. +#ifndef __MINGW_INTSAFE_CHAR_API +#define __MINGW_INTSAFE_CHAR_API __MINGW_INTSAFE_API #endif Why do you need separated macro for that? +#define __MINGW_INTSAFE_CONV(name, type_src, type_dest) \ + HRESULT name(_In_ type_src operand, _Out_ type_dest * result) \ + __MINGW_INTSAFE_BODY(add, operand, 0) + +#define __MINGW_INTSAFE_MATH(name, type, operation) \ + HRESULT name(_In_ type x, _In_ type y, _Out_ type * result) \ + __MINGW_INTSAFE_BODY(operation, x, y) We usually don't use stuff like _In_ and _Out_ in mingw-w64, but they won't hurt so it's fine. +/* If CHAR is unsigned, use different symbol names. +The avoids the risk of linking to the wrong function when different +translation units with different types of chars are linked together. */ +#ifdef __CHAR_UNSIGNED__ +#define UShortToChar __mingw_intsafe_uchar_UShortToChar +#define WordToChar __mingw_intsafe_uchar_WordToChar +#define ShortToChar __mingw_intsafe_uchar_ShortToChar +#define UIntToChar __mingw_intsafe_uchar_UIntToChar +#define ULongToChar __mingw_intsafe_uchar_ULongToChar +#define DWordToChar __mingw_intsafe_uchar_DWordToChar +#define IntToChar __mingw_intsafe_uchar_IntToChar +#define LongToChar __mingw_intsafe_uchar_LongToChar +#endif If we use FORCEINLINE functions for them, that's not needed. Thanks, Jacek ------------------------------------------------------------------------------ _______________________________________________ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
