[Mingw-w64-public] Patch: complete implementation of intsafe.h

[David Grayson]

Hello. Attached is a patch that adds a complete implementation of intsafe.h that I generated and tested using Ruby. It would be great if someone could merge it in.

The version of intsafe.h included in this patch can also be viewed here:

https://github.com/DavidEGrayson/intsafe/blob/1.1.0/generated/intsafe.h

That repository also has Ruby scripts that I used to help generate and test the header. Hopefully we can keep using those scripts to update intsafe.h in the future instead of editing it by hand. I believe using scripts to generate a highly-repetitive header like this is the best way to ensure consistency and avoid errors.

This patch also fixes the definitions of CHAR_MIN and CHAR_MAX in limits.h so that they have the right values when char is unsigned (-funsigned-char). This change is necessary to make intsafe.h work because I used limits defined in limits.h and stdint.h instead of redefining them.

Some statistics:

- The intsafe.h documentation from Microsoft defines 253 inline functions:
- 193 integer conversion functions
- 60 math operations (20 types with 3 operations each)
- This implementation of intsafe.h is only 1562 lines (6.2 lines per function)
- 134 function bodies
- 119 functions defined as a simple preprocessor macro pointing a compatible function
- This implementation of intsafe.h is generated from 693 lines of Ruby code
- Microsoft's bulky version from the Windows SDK takes 8570 lines (33.9 lines per function)

I made very light use of the preprocessor because I had Ruby at my disposal, and I think this resulted in very clear and easy to understand code. One thing that makes the code easier to check is the lack of casting in the main computations. Casting can suppress a lot of useful compiler warnings so I simply didn't do it, and I fixed the root causes of the warnings instead.

I have a giant auto-generated test suite to test implementations of intsafe.h, and I was constantly running it against all combinations of architecture (32-bit and 64-it), language (C and C++), and char type (signed and unsigned). The tests were run with the options "-Wall -Werror -pedantic -O1". I think I am aware of most of the issues regarding undefined behavior from integers and I think I avoided them all. Although the following conditions are not strictly necessary, I think my code conforms to them: no operation should ever overflow, and no value should ever get converted to a type that cannot represent it. I designed every function in intsafe.h so that it is guaranteed to write to its output parameter at least once before returning, in order to protect users from the undefined behavior of reading from an uninitialized variable.

I wasn't sure if I should use the always_inline attribute, so I didn't. Certain things you might do in C can cause undefined reference errors because no non-inline definition is provided by default. However, the header can easily be used to generate non-inline definitions if needed, either by the user or by the mingw-w64 developers. Since CHAR might be signed in one translation unit and unsigned in another, I took measures to prevent accidentally linking to the wrong version of a function that operates on a CHAR (see line 956).

All of the files I made for this project are in the public domain.

I am welcome to feedback, even if it is simple things like coding style or mingw-w64 conventions. I hope you guys will find this useful when porting MSVC projects over to mingw-w64, and I know I will. Thanks!

--David Grayson

[Kai Tietz]

Thanks for your contribution. I would like to see Jacek's comment on this
before it gets applied.

Kai

Post by David Grayson
Hello. Attached is a patch that adds a complete implementation of
intsafe.h that I generated and tested using Ruby. It would be great if
someone could merge it in.
https://github.com/DavidEGrayson/intsafe/blob/1.1.0/generated/intsafe.h
That repository also has Ruby scripts that I used to help generate and
test the header. Hopefully we can keep using those scripts to update
intsafe.h in the future instead of editing it by hand. I believe using
scripts to generate a highly-repetitive header like this is the best way to
ensure consistency and avoid errors.
This patch also fixes the definitions of CHAR_MIN and CHAR_MAX in limits.h
so that they have the right values when char is unsigned
(-funsigned-char). This change is necessary to make intsafe.h work because
I used limits defined in limits.h and stdint.h instead of redefining them.
- 193 integer conversion functions
- 60 math operations (20 types with 3 operations each)
- This implementation of intsafe.h is only 1562 lines (6.2 lines per function)
- 134 function bodies
- 119 functions defined as a simple preprocessor macro pointing a compatible function
- This implementation of intsafe.h is generated from 693 lines of Ruby code
- Microsoft's bulky version from the Windows SDK takes 8570 lines (33.9 lines per function)
I made very light use of the preprocessor because I had Ruby at my
disposal, and I think this resulted in very clear and easy to understand
code. One thing that makes the code easier to check is the lack of casting
in the main computations. Casting can suppress a lot of useful compiler
warnings so I simply didn't do it, and I fixed the root causes of the
warnings instead.
I have a giant auto-generated test suite to test implementations of
intsafe.h, and I was constantly running it against all combinations of
architecture (32-bit and 64-it), language (C and C++), and char type
(signed and unsigned). The tests were run with the options "-Wall -Werror
-pedantic -O1". I think I am aware of most of the issues regarding
undefined behavior from integers and I think I avoided them all. Although
the following conditions are not strictly necessary, I think my code
conforms to them: no operation should ever overflow, and no value should
ever get converted to a type that cannot represent it. I designed every
function in intsafe.h so that it is guaranteed to write to its output
parameter at least once before returning, in order to protect users from
the undefined behavior of reading from an uninitialized variable.
I wasn't sure if I should use the always_inline attribute, so I didn't.
Certain things you might do in C can cause undefined reference errors
because no non-inline definition is provided by default. However, the
header can easily be used to generate non-inline definitions if needed,
either by the user or by the mingw-w64 developers. Since CHAR might be
signed in one translation unit and unsigned in another, I took measures to
prevent accidentally linking to the wrong version of a function that
operates on a CHAR (see line 956).
All of the files I made for this project are in the public domain.
I am welcome to feedback, even if it is simple things like coding style or
mingw-w64 conventions. I hope you guys will find this useful when porting
MSVC projects over to mingw-w64, and I know I will. Thanks!
--David Grayson
------------------------------------------------------------------------------
_______________________________________________
Mingw-w64-public mailing list
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public

[David Grayson]

Thank you, Kai.

Before Jacek or anyone spends too much time checking my
add/subtract/multiply operations, please note that I just learned
about GCC's integer overflow built-ins, and I will be rewriting the
math operations to use those and submitting a new version of this
patch:

https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html

This does not mean there is a bug with the previous patch, it just
means that I could implement it in a better way that makes it easier
to review (and probably more efficient).

--David

Post by Kai Tietz
Thanks for your contribution. I would like to see Jacek's comment on this
before it gets applied.
Kai

Post by David Grayson
Hello. Attached is a patch that adds a complete implementation of
intsafe.h that I generated and tested using Ruby. It would be great if
someone could merge it in.
https://github.com/DavidEGrayson/intsafe/blob/1.1.0/generated/intsafe.h
That repository also has Ruby scripts that I used to help generate and
test the header. Hopefully we can keep using those scripts to update
intsafe.h in the future instead of editing it by hand. I believe using
scripts to generate a highly-repetitive header like this is the best way to
ensure consistency and avoid errors.
This patch also fixes the definitions of CHAR_MIN and CHAR_MAX in limits.h
so that they have the right values when char is unsigned (-funsigned-char).
This change is necessary to make intsafe.h work because I used limits
defined in limits.h and stdint.h instead of redefining them.
- 193 integer conversion functions
- 60 math operations (20 types with 3 operations each)
- This implementation of intsafe.h is only 1562 lines (6.2 lines per function)
- 134 function bodies
- 119 functions defined as a simple preprocessor macro pointing a compatible function
- This implementation of intsafe.h is generated from 693 lines of Ruby code
- Microsoft's bulky version from the Windows SDK takes 8570 lines (33.9
lines per function)
I made very light use of the preprocessor because I had Ruby at my
disposal, and I think this resulted in very clear and easy to understand
code. One thing that makes the code easier to check is the lack of casting
in the main computations. Casting can suppress a lot of useful compiler
warnings so I simply didn't do it, and I fixed the root causes of the
warnings instead.
I have a giant auto-generated test suite to test implementations of
intsafe.h, and I was constantly running it against all combinations of
architecture (32-bit and 64-it), language (C and C++), and char type (signed
and unsigned). The tests were run with the options "-Wall -Werror -pedantic
-O1". I think I am aware of most of the issues regarding undefined behavior
from integers and I think I avoided them all. Although the following
conditions are not strictly necessary, I think my code conforms to them: no
operation should ever overflow, and no value should ever get converted to a
type that cannot represent it. I designed every function in intsafe.h so
that it is guaranteed to write to its output parameter at least once before
returning, in order to protect users from the undefined behavior of reading
from an uninitialized variable.
I wasn't sure if I should use the always_inline attribute, so I didn't.
Certain things you might do in C can cause undefined reference errors
because no non-inline definition is provided by default. However, the
header can easily be used to generate non-inline definitions if needed,
either by the user or by the mingw-w64 developers. Since CHAR might be
signed in one translation unit and unsigned in another, I took measures to
prevent accidentally linking to the wrong version of a function that
operates on a CHAR (see line 956).
All of the files I made for this project are in the public domain.
I am welcome to feedback, even if it is simple things like coding style or
mingw-w64 conventions. I hope you guys will find this useful when porting
MSVC projects over to mingw-w64, and I know I will. Thanks!
--David Grayson
------------------------------------------------------------------------------
_______________________________________________
Mingw-w64-public mailing list
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public

------------------------------------------------------------------------------
_______________________________________________
Mingw-w64-public mailing list
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public

------------------------------------------------------------------------------

[David Grayson]

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.

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.

--David Grayson

Post by David Grayson
Hello. Attached is a patch that adds a complete implementation of intsafe.h that I generated and tested using Ruby. It would be great if someone could merge it in.
https://github.com/DavidEGrayson/intsafe/blob/1.1.0/generated/intsafe.h
That repository also has Ruby scripts that I used to help generate and test the header. Hopefully we can keep using those scripts to update intsafe.h in the future instead of editing it by hand. I believe using scripts to generate a highly-repetitive header like this is the best way to ensure consistency and avoid errors.
This patch also fixes the definitions of CHAR_MIN and CHAR_MAX in limits.h so that they have the right values when char is unsigned (-funsigned-char). This change is necessary to make intsafe.h work because I used limits defined in limits.h and stdint.h instead of redefining them.
- 193 integer conversion functions
- 60 math operations (20 types with 3 operations each)
- This implementation of intsafe.h is only 1562 lines (6.2 lines per function)
- 134 function bodies
- 119 functions defined as a simple preprocessor macro pointing a compatible function
- This implementation of intsafe.h is generated from 693 lines of Ruby code
- Microsoft's bulky version from the Windows SDK takes 8570 lines (33.9 lines per function)
I made very light use of the preprocessor because I had Ruby at my disposal, and I think this resulted in very clear and easy to understand code. One thing that makes the code easier to check is the lack of casting in the main computations. Casting can suppress a lot of useful compiler warnings so I simply didn't do it, and I fixed the root causes of the warnings instead.
I have a giant auto-generated test suite to test implementations of intsafe.h, and I was constantly running it against all combinations of architecture (32-bit and 64-it), language (C and C++), and char type (signed and unsigned). The tests were run with the options "-Wall -Werror -pedantic -O1". I think I am aware of most of the issues regarding undefined behavior from integers and I think I avoided them all. Although the following conditions are not strictly necessary, I think my code conforms to them: no operation should ever overflow, and no value should ever get converted to a type that cannot represent it. I designed every function in intsafe.h so that it is guaranteed to write to its output parameter at least once before returning, in order to protect users from the undefined behavior of reading from an uninitialized variable.
I wasn't sure if I should use the always_inline attribute, so I didn't. Certain things you might do in C can cause undefined reference errors because no non-inline definition is provided by default. However, the header can easily be used to generate non-inline definitions if needed, either by the user or by the mingw-w64 developers. Since CHAR might be signed in one translation unit and unsigned in another, I took measures to prevent accidentally linking to the wrong version of a function that operates on a CHAR (see line 956).
All of the files I made for this project are in the public domain.
I am welcome to feedback, even if it is simple things like coding style or mingw-w64 conventions. I hope you guys will find this useful when porting MSVC projects over to mingw-w64, and I know I will. Thanks!
--David Grayson

[Jacek Caban]

Hi David,

That's a nice work, thanks!

Post by David Grayson
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.
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
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.

Post by David Grayson
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

Post by David Grayson
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



------------------------------------------------------------------------------

[David Grayson]

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.

Post by Jacek Caban
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

Post by Jacek Caban
Hi David,
That's a nice work, thanks!

Post by David Grayson
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.
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
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.

Post by David Grayson
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
+ *
+ * https://msdn.microsoft.com/en-us/library/windows/desktop/ff521693
+ *
+ *
+ * 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

Post by David Grayson
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
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public

------------------------------------------------------------------------------

[Jacek Caban]

Post by Jacek Caban
+#ifndef __MINGW_INTSAFE_API
+#define __MINGW_INTSAFE_API inline
+#endif
+#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.

I'd personally call build system that mixes char types as broken, but
it's valid so we can support it. How about simply using static inline
functions for __MINGW_INTSAFE_CHAR_AP? This will avoid the problem of
having different declarations in different compile units.

Thanks,
Jacek

------------------------------------------------------------------------------

[David Grayson]

Thanks, Jacek. I have included a new patch with all of your suggestions (version 2.1.0). As always, it's in the public domain.

Post by Jacek Caban
I'd personally call build system that mixes char types as broken, but
it's valid so we can support it. How about simply using static inline
functions for __MINGW_INTSAFE_CHAR_AP? This will avoid the problem of
having different declarations in different compile units.

Using static inline means that you won't get reliable results when comparing two function pointers that came from different translation units. But that is such a minor point, so I can see why you would prefer to use "static inline" instead of introducing this "__mingw_intsafe_uchar_" prefix on the function variants that take a CHAR and treat it as unsigned. I changed your suggestion slightly: in this latest patch, I only use "static inline" if __CHAR_UNSIGNED__ is defined, because I didn't want to impact/surprise the users of signed chars at all.

Thanks for all the quick feedback!

--David

[David Grayson]

Post by David Grayson
Thanks, Jacek. I have included a new patch with all of your suggestions (version 2.1.0). As always, it's in the public domain.

Please ignore the last patch. Here is a new one (2.2.0) that includes 79 (!) more conversion functions I just discovered in a hard-to-find place in the MSDN documentation. I think these were all added in Windows 7.

I think the most complete lists of intsafe.h functions can be found at these links:

https://msdn.microsoft.com/en-us/library/windows/desktop/ff521655
https://msdn.microsoft.com/en-us/library/windows/desktop/ff521701

The documentation is severely broken: if you try to traverse up one level in the heirarchy (click on "intsafe.h functions") and then you click on a link to go back down to the page you were on, you might notice that tons of functions are missing (like Int8ToUInt8 and LongSub). This is why I have often been confused about what functions are supposed to be in intsafe.h.

The PSDK version of intsafe.h seems to have even more functions hiding in it that are not documented at all (like LongPtrToDWordLong), but I am hesitant to implement undocumented functions from a copyrighted header.

Anyway, I should have done more research before submitting the last patch. Sorry for the mailing list noise.

--David

[Jacek Caban]

Hi David,

Post by David Grayson

Post by David Grayson
Thanks, Jacek. I have included a new patch with all of your
suggestions (version 2.1.0). As always, it's in the public domain.

Please ignore the last patch. Here is a new one (2.2.0) that includes
79 (!) more conversion functions I just discovered in a hard-to-find
place in the MSDN documentation. I think these were all added in
Windows 7.
https://msdn.microsoft.com/en-us/library/windows/desktop/ff521655
https://msdn.microsoft.com/en-us/library/windows/desktop/ff521701
The documentation is severely broken: if you try to traverse up one
level in the heirarchy (click on "intsafe.h functions") and then you
click on a link to go back down to the page you were on, you might
notice that tons of functions are missing (like Int8ToUInt8 and
LongSub). This is why I have often been confused about what functions
are supposed to be in intsafe.h.
The PSDK version of intsafe.h seems to have even more functions hiding
in it that are not documented at all (like LongPtrToDWordLong), but I
am hesitant to implement undocumented functions from a copyrighted
header.
Anyway, I should have done more research before submitting the last
patch. Sorry for the mailing list noise.

I'm sorry for the delay, I was sure I already replied. The patch looks
mostly good and ready to be committed IMO. There is one minor thing:

-#ifndef _INTSAFE_H_INCLUDED_
-#define _INTSAFE_H_INCLUDED_
+#pragma once

We use #ifndef guards all over the place. PSDK headers usually use both.
That's find to add the pragma, but please don't delete existing guards.

Also while I was reviewing the patch, I noticed that MS version doesn't
set output value to 0, but instead to values like ~0. Documentation
specifies this case as invalid value, so whatever we use is fine IMHO.

Thanks for the work,
Jacek


------------------------------------------------------------------------------

[Alexandre Pereira Nunes]

Post by Jacek Caban
Hi David,
I'm sorry for the delay, I was sure I already replied. The patch looks
-#ifndef _INTSAFE_H_INCLUDED_
-#define _INTSAFE_H_INCLUDED_
+#pragma once
We use #ifndef guards all over the place. PSDK headers usually use both.
That's find to add the pragma, but please don't delete existing guards.
Also while I was reviewing the patch, I noticed that MS version doesn't
set output value to 0, but instead to values like ~0. Documentation
specifies this case as invalid value, so whatever we use is fine IMHO.
Thanks for the work,
Jacek

"The GNU C preprocessor is programmed to notice when a header file uses
this particular construct and handle it efficiently. If a header file is
contained entirely in a `#ifndef' conditional, then it records that fact.
If a subsequent `#include' specifies the same file, and the macro in the
`#ifndef' is already defined, then the file is entirely skipped, without
even reading it.

There is also an explicit directive to tell the preprocessor that it need
not include a file more than once. This is called `#pragma once', and was
used *in addition to* the `#ifndef' conditional around the contents of the
header file. `#pragma once' is now obsolete and should not be used at all."


... not sure what/if changed since then.

[David Grayson]

Post by Jacek Caban
I'm sorry for the delay, I was sure I already replied.

That's fine, I'm not paying you. :-)

Post by Jacek Caban
We use #ifndef guards all over the place. PSDK headers usually use both.
That's find to add the pragma, but please don't delete existing guards.

OK, I have attached a new version of the patch (2.2.1, public domain) that just uses ifndef instead of the pragma. I can see now that it could be a compatibility issue if their header defines "_INTSAFE_H_INCLUDED_" and our header does not, so it seems good to use the ifndef. I don't see a need to use both.

Alexandre: As far as I can tell, '#pragma once' is not deprecated. It is documented here:

https://gcc.gnu.org/onlinedocs/cpp/Alternatives-to-Wrapper-_0023ifndef.html

Post by Jacek Caban
Also while I was reviewing the patch, I noticed that MS version doesn't
set output value to 0, but instead to values like ~0. Documentation
specifies this case as invalid value, so whatever we use is fine IMHO.

Yeah, I noticed that too. I suppose I should have been upfront about all the issues like that. Originally I thought that their version sometimes uses 0 and sometimes uses -1, but now it looks like they consistently use -1 (which is ~0 for unsigned types). If I understand the C/C++ standards correctly, I think it would be safe for us to change the error case to "*result = -1;" and I can look into that if you want.

Here is a quick summary of the remaining possible compatibility issues between our version and the PSDK version of intsafe.h:

- We include wtypesbase.h to define our types
- Our error result is always 0, they use ~0.
- They have some undocumented functions in intsafe.h that we don't have.
- Our version won't work on clang or older versions of gcc because it uses these builtins, but at least it can be included.

Post by Jacek Caban
Thanks for the work,
Jacek

Thanks, Jacek!

--David

[Jacek Caban]

Hi David,

Post by David Grayson
Here is a quick summary of the remaining possible compatibility issues
- We include wtypesbase.h to define our types
- Our error result is always 0, they use ~0.
- They have some undocumented functions in intsafe.h that we don't have.
- Our version won't work on clang or older versions of gcc because it
uses these builtins, but at least it can be included.

The patch looks good to me.

Thanks,
Jacek

------------------------------------------------------------------------------

[David Grayson]

Post by Jacek Caban
The patch looks good to me.

I think it's good too, but it looks like it (intsafe_2.2.1.patch)
hasn't been committed to the git repository. Can someone commit it?

--David

------------------------------------------------------------------------------

[JonY]

Post by David Grayson

Post by Jacek Caban
The patch looks good to me.

I think it's good too, but it looks like it (intsafe_2.2.1.patch)
hasn't been committed to the git repository. Can someone commit it?

Does it make sense to add the warn_unused_result attribute? I'll commit
soon if nobody has yet.

[David Grayson]

Thanks, Jon! If you want to add warn_unused_result, that seems fine
to me. --David

Post by JonY

Post by David Grayson

Post by Jacek Caban
The patch looks good to me.

I think it's good too, but it looks like it (intsafe_2.2.1.patch)
hasn't been committed to the git repository. Can someone commit it?

Does it make sense to add the warn_unused_result attribute? I'll commit
soon if nobody has yet.
------------------------------------------------------------------------------
_______________________________________________
Mingw-w64-public mailing list
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public

------------------------------------------------------------------------------

[Jacek Caban]

Post by David Grayson

Post by Jacek Caban
The patch looks good to me.

I think it's good too, but it looks like it (intsafe_2.2.1.patch)
hasn't been committed to the git repository. Can someone commit it?

Sorry for the delay, I was hoping to see Kai's comment, but he doesn't
seem around lately. I guess he is on vacations. I committed the patch,
hoping he won't mind.

Thanks for the contribution,
Jacek

------------------------------------------------------------------------------

[Kai Tietz]

Hi Jacek,

no I don't mind at all. The reason I comment on this thread is a
recent report about issues with current master's direct-x support. it
might be related to this.

Are you aware of this report?

Cheers,
Kai

Post by Jacek Caban

Post by David Grayson

Post by Jacek Caban
The patch looks good to me.

I think it's good too, but it looks like it (intsafe_2.2.1.patch)
hasn't been committed to the git repository. Can someone commit it?

Sorry for the delay, I was hoping to see Kai's comment, but he doesn't
seem around lately. I guess he is on vacations. I committed the patch,
hoping he won't mind.
Thanks for the contribution,
Jacek
------------------------------------------------------------------------------
_______________________________________________
Mingw-w64-public mailing list
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public

------------------------------------------------------------------------------

[Jacek Caban]

Hi Kai,

No, I'm not aware of the report. I can't see it in our bug tracker,
where can I find it?

Cheers,
Jacek

Post by Kai Tietz
Hi Jacek,
no I don't mind at all. The reason I comment on this thread is a
recent report about issues with current master's direct-x support. it
might be related to this.
Are you aware of this report?
Cheers,
Kai

Post by Jacek Caban

Post by David Grayson

Post by Jacek Caban
The patch looks good to me.

I think it's good too, but it looks like it (intsafe_2.2.1.patch)
hasn't been committed to the git repository. Can someone commit it?

Sorry for the delay, I was hoping to see Kai's comment, but he doesn't
seem around lately. I guess he is on vacations. I committed the patch,
hoping he won't mind.
Thanks for the contribution,
Jacek
------------------------------------------------------------------------------
_______________________________________________
Mingw-w64-public mailing list
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public

------------------------------------------------------------------------------
_______________________________________________
Mingw-w64-public mailing list
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public

------------------------------------------------------------------------------