Hello, Jacek. Thanks for taking a look at my patch! 1) I will add "#if __GNUC__ >= 5" around the parts that use the new built-ins.
2) I'll remove the change to limits.h 3) I'll remove the link to my GitHub; I was hoping people would use my script to regenerate the header but it's fine if they don't because it has gotten so simple. 4) I'll remove the MSDN links. 5) I'm not sure if we want to include wtypesbase.h either. I generally like to avoid duplicating code, but if someone wants me to stop including wtypesbase.h and define the types myself then I'm happy to make that change. 6) I'll remove the _In_ and _Out_ annotations since they don't fit the mingw-w64 style. 7) I'll change inline to FORCEINLINE. > If we use FORCEINLINE functions for them, that's not needed. 8) Even with FORCEINLINE, you still sometimes need to have a non-inline definition of the function available, for example if you are doing any operations on a function pointer to one of the intsafe.h functions. (Or is that not something a user should be doing?) Therefore, intsafe.h should provide a mode for generating non-inline definitions: +#ifndef __MINGW_INTSAFE_API +#define __MINGW_INTSAFE_API inline +#endif However, the non-inline definition might be in a different translation unit that has a different definition of what a CHAR is, due to the -funsigned-char option. Therefore, for code using -funsigned-char, we change the actual function names for any function operating on a char, prepending them with "__mingw_intsafe_uchar_". Someone in a special situation might need to generate non-inline definitions for both the normal functions and the __mingw_intsafe_uchar_* variants, so I give them independent control over whether the char functions will have non-inline definitions: +#ifndef __MINGW_INTSAFE_CHAR_API +#define __MINGW_INTSAFE_CHAR_API __MINGW_INTSAFE_API +#endif I imagine a lot of developers will view this whole line of reasoning as pretty far-fetched and not likely to help anyone, and that is true. But at the same time, the changes that I made to support non-inline definitions and unsigned chars are pretty unobtrusive and should have no effect on someone who uses signed chars, so I think there is no practical cost to doing it this way. So, let me know what you think and then I'll make the next version of the patch that has all the needed changes. Thanks, Jacek! --David On 8/3/2015 7:34 AM, Jacek Caban wrote: > 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 > [email protected] > https://lists.sourceforge.net/lists/listinfo/mingw-w64-public > ------------------------------------------------------------------------------ _______________________________________________ Mingw-w64-public mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
