|
From: Martin S. <ma...@ma...> - 2020-04-04 20:35:55
|
Don't assume that __MINGW32__ implies x86; Windows runs on ARM/ARM64
as well, and there are mingw toolchains that target those architectures.
This mirrors how the MSVC part of the same expressions are written,
as (defined(_WIN32) && defined(_M_IX86)) and
(defined(_WIN64) && defined(_M_X64)) - not relying on _WIN32/_WIN64
or __MINGW32__/__MINGW64__ alone to indicate architecture.
---
include/valgrind.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/valgrind.h b/include/valgrind.h
index c8b24a38e..e8195c1ce 100644
--- a/include/valgrind.h
+++ b/include/valgrind.h
@@ -131,11 +131,11 @@
# define PLAT_x86_darwin 1
#elif defined(__APPLE__) && defined(__x86_64__)
# define PLAT_amd64_darwin 1
-#elif (defined(__MINGW32__) && !defined(__MINGW64__)) \
+#elif (defined(__MINGW32__) && defined(__i386__)) \
|| defined(__CYGWIN32__) \
|| (defined(_WIN32) && defined(_M_IX86))
# define PLAT_x86_win32 1
-#elif defined(__MINGW64__) \
+#elif (defined(__MINGW64__) && defined(__x86_64__)) \
|| (defined(_WIN64) && defined(_M_X64))
# define PLAT_amd64_win64 1
#elif defined(__linux__) && defined(__i386__)
--
2.17.1
|
|
From: Martin S. <ma...@ma...> - 2020-04-27 13:16:14
|
On Sat, 4 Apr 2020, Martin Storsjö wrote: > Don't assume that __MINGW32__ implies x86; Windows runs on ARM/ARM64 > as well, and there are mingw toolchains that target those architectures. > > This mirrors how the MSVC part of the same expressions are written, > as (defined(_WIN32) && defined(_M_IX86)) and > (defined(_WIN64) && defined(_M_X64)) - not relying on _WIN32/_WIN64 > or __MINGW32__/__MINGW64__ alone to indicate architecture. > --- > include/valgrind.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/valgrind.h b/include/valgrind.h > index c8b24a38e..e8195c1ce 100644 > --- a/include/valgrind.h > +++ b/include/valgrind.h > @@ -131,11 +131,11 @@ > # define PLAT_x86_darwin 1 > #elif defined(__APPLE__) && defined(__x86_64__) > # define PLAT_amd64_darwin 1 > -#elif (defined(__MINGW32__) && !defined(__MINGW64__)) \ > +#elif (defined(__MINGW32__) && defined(__i386__)) \ > || defined(__CYGWIN32__) \ > || (defined(_WIN32) && defined(_M_IX86)) > # define PLAT_x86_win32 1 > -#elif defined(__MINGW64__) \ > +#elif (defined(__MINGW64__) && defined(__x86_64__)) \ > || (defined(_WIN64) && defined(_M_X64)) > # define PLAT_amd64_win64 1 > #elif defined(__linux__) && defined(__i386__) > -- > 2.17.1 Ping, any comments on this? It's not directly related to valgrind functionality, but glib includes this header, and this header breaks compilation on non-x86 windows platforms in its current form. // Martin |
|
From: Philippe W. <phi...@sk...> - 2020-04-27 16:05:34
|
Hello Martin, Thanks for the patch and the ping. Patches have a trend to be forgotten when sent (only) to mailing list, so it is best to enter a bug in valgrind bugzilla. Thanks Philippe On Mon, 2020-04-27 at 16:16 +0300, Martin Storsjö wrote: > On Sat, 4 Apr 2020, Martin Storsjö wrote: > > > Don't assume that __MINGW32__ implies x86; Windows runs on ARM/ARM64 > > as well, and there are mingw toolchains that target those architectures. > > > > This mirrors how the MSVC part of the same expressions are written, > > as (defined(_WIN32) && defined(_M_IX86)) and > > (defined(_WIN64) && defined(_M_X64)) - not relying on _WIN32/_WIN64 > > or __MINGW32__/__MINGW64__ alone to indicate architecture. > > --- > > include/valgrind.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/include/valgrind.h b/include/valgrind.h > > index c8b24a38e..e8195c1ce 100644 > > --- a/include/valgrind.h > > +++ b/include/valgrind.h > > @@ -131,11 +131,11 @@ > > # define PLAT_x86_darwin 1 > > #elif defined(__APPLE__) && defined(__x86_64__) > > # define PLAT_amd64_darwin 1 > > -#elif (defined(__MINGW32__) && !defined(__MINGW64__)) \ > > +#elif (defined(__MINGW32__) && defined(__i386__)) \ > > || defined(__CYGWIN32__) \ > > || (defined(_WIN32) && defined(_M_IX86)) > > # define PLAT_x86_win32 1 > > -#elif defined(__MINGW64__) \ > > +#elif (defined(__MINGW64__) && defined(__x86_64__)) \ > > || (defined(_WIN64) && defined(_M_X64)) > > # define PLAT_amd64_win64 1 > > #elif defined(__linux__) && defined(__i386__) > > -- > > 2.17.1 > > Ping, any comments on this? > > It's not directly related to valgrind functionality, but glib includes > this header, and this header breaks compilation on non-x86 windows > platforms in its current form. > > // Martin > _______________________________________________ > Valgrind-developers mailing list > Val...@li... > https://lists.sourceforge.net/lists/listinfo/valgrind-developers |
|
From: Bart V. A. <bva...@ac...> - 2020-04-27 17:59:00
|
On 2020-04-04 13:35, Martin Storsjö wrote: > Don't assume that __MINGW32__ implies x86; Windows runs on ARM/ARM64 > as well, and there are mingw toolchains that target those architectures. > > This mirrors how the MSVC part of the same expressions are written, > as (defined(_WIN32) && defined(_M_IX86)) and > (defined(_WIN64) && defined(_M_X64)) - not relying on _WIN32/_WIN64 > or __MINGW32__/__MINGW64__ alone to indicate architecture. > --- > include/valgrind.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/valgrind.h b/include/valgrind.h > index c8b24a38e..e8195c1ce 100644 > --- a/include/valgrind.h > +++ b/include/valgrind.h > @@ -131,11 +131,11 @@ > # define PLAT_x86_darwin 1 > #elif defined(__APPLE__) && defined(__x86_64__) > # define PLAT_amd64_darwin 1 > -#elif (defined(__MINGW32__) && !defined(__MINGW64__)) \ > +#elif (defined(__MINGW32__) && defined(__i386__)) \ > || defined(__CYGWIN32__) \ > || (defined(_WIN32) && defined(_M_IX86)) > # define PLAT_x86_win32 1 > -#elif defined(__MINGW64__) \ > +#elif (defined(__MINGW64__) && defined(__x86_64__)) \ > || (defined(_WIN64) && defined(_M_X64)) > # define PLAT_amd64_win64 1 > #elif defined(__linux__) && defined(__i386__) How about changing __MINGW64__ into __MINGW32__? I think that will make this patch easier to read. From commit d406e8725c09: "mingw64 also defines __MINGW32__". Please also add a comment that the MinGW64 compiler defines __MINGW32__. Thanks, Bart. |
|
From: Martin S. <ma...@ma...> - 2020-04-27 18:04:33
|
On Mon, 27 Apr 2020, Bart Van Assche wrote: > On 2020-04-04 13:35, Martin Storsjö wrote: >> Don't assume that __MINGW32__ implies x86; Windows runs on ARM/ARM64 >> as well, and there are mingw toolchains that target those architectures. >> >> This mirrors how the MSVC part of the same expressions are written, >> as (defined(_WIN32) && defined(_M_IX86)) and >> (defined(_WIN64) && defined(_M_X64)) - not relying on _WIN32/_WIN64 >> or __MINGW32__/__MINGW64__ alone to indicate architecture. >> --- >> include/valgrind.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/include/valgrind.h b/include/valgrind.h >> index c8b24a38e..e8195c1ce 100644 >> --- a/include/valgrind.h >> +++ b/include/valgrind.h >> @@ -131,11 +131,11 @@ >> # define PLAT_x86_darwin 1 >> #elif defined(__APPLE__) && defined(__x86_64__) >> # define PLAT_amd64_darwin 1 >> -#elif (defined(__MINGW32__) && !defined(__MINGW64__)) \ >> +#elif (defined(__MINGW32__) && defined(__i386__)) \ >> || defined(__CYGWIN32__) \ >> || (defined(_WIN32) && defined(_M_IX86)) >> # define PLAT_x86_win32 1 >> -#elif defined(__MINGW64__) \ >> +#elif (defined(__MINGW64__) && defined(__x86_64__)) \ >> || (defined(_WIN64) && defined(_M_X64)) >> # define PLAT_amd64_win64 1 >> #elif defined(__linux__) && defined(__i386__) > > How about changing __MINGW64__ into __MINGW32__? I think that will make > this patch easier to read. From commit d406e8725c09: "mingw64 also > defines __MINGW32__". Please also add a comment that the MinGW64 > compiler defines __MINGW32__. That's fine with me as well, but there is the same kind of tautology for the MSVC variant of ifdefs right next to it, "defined(_WIN64) && defined(_M_X64)". Should I keep that as is or do the same kind of cleanup for that as well? // Martin |
|
From: Bart V. A. <bva...@ac...> - 2020-04-27 18:09:51
|
On 2020-04-27 11:04, Martin Storsjö wrote: > On Mon, 27 Apr 2020, Bart Van Assche wrote: > >> On 2020-04-04 13:35, Martin Storsjö wrote: >>> Don't assume that __MINGW32__ implies x86; Windows runs on ARM/ARM64 >>> as well, and there are mingw toolchains that target those architectures. >>> >>> This mirrors how the MSVC part of the same expressions are written, >>> as (defined(_WIN32) && defined(_M_IX86)) and >>> (defined(_WIN64) && defined(_M_X64)) - not relying on _WIN32/_WIN64 >>> or __MINGW32__/__MINGW64__ alone to indicate architecture. >>> --- >>> include/valgrind.h | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/include/valgrind.h b/include/valgrind.h >>> index c8b24a38e..e8195c1ce 100644 >>> --- a/include/valgrind.h >>> +++ b/include/valgrind.h >>> @@ -131,11 +131,11 @@ >>> # define PLAT_x86_darwin 1 >>> #elif defined(__APPLE__) && defined(__x86_64__) >>> # define PLAT_amd64_darwin 1 >>> -#elif (defined(__MINGW32__) && !defined(__MINGW64__)) \ >>> +#elif (defined(__MINGW32__) && defined(__i386__)) \ >>> || defined(__CYGWIN32__) \ >>> || (defined(_WIN32) && defined(_M_IX86)) >>> # define PLAT_x86_win32 1 >>> -#elif defined(__MINGW64__) \ >>> +#elif (defined(__MINGW64__) && defined(__x86_64__)) \ >>> || (defined(_WIN64) && defined(_M_X64)) >>> # define PLAT_amd64_win64 1 >>> #elif defined(__linux__) && defined(__i386__) >> >> How about changing __MINGW64__ into __MINGW32__? I think that will >> make this patch easier to read. From commit d406e8725c09: "mingw64 >> also defines __MINGW32__". Please also add a comment that the MinGW64 >> compiler defines __MINGW32__. > > That's fine with me as well, but there is the same kind of tautology for > the MSVC variant of ifdefs right next to it, "defined(_WIN64) && > defined(_M_X64)". Should I keep that as is or do the same kind of > cleanup for that as well? If that additional change is made, please add an additional comment that explains that _WIN32 is also defined when targeting 64-bit mode. Thanks, Bart. |
|
From: Martin S. <ma...@ma...> - 2020-04-27 18:32:02
|
Don't assume that __MINGW32__ implies x86; Windows runs on ARM/ARM64
as well, and there are mingw toolchains that target those architectures.
This mirrors how the MSVC parts of the same expressions are written,
as (defined(_WIN32) && defined(_M_IX86)) and
(defined(_WIN64) && defined(_M_X64)) - not relying on _WIN32/_WIN64
or __MINGW32__/__MINGW64__ alone to indicate architecture.
Change the __MINGW64__ and _WIN64 ifdefs into plain __MINGW32__
and _WIN32 as well, for clarity - these defines mostly imply
platform.
---
include/valgrind.h | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/valgrind.h b/include/valgrind.h
index c8b24a38e..00e985dca 100644
--- a/include/valgrind.h
+++ b/include/valgrind.h
@@ -131,12 +131,13 @@
# define PLAT_x86_darwin 1
#elif defined(__APPLE__) && defined(__x86_64__)
# define PLAT_amd64_darwin 1
-#elif (defined(__MINGW32__) && !defined(__MINGW64__)) \
+#elif (defined(__MINGW32__) && defined(__i386__)) \
|| defined(__CYGWIN32__) \
|| (defined(_WIN32) && defined(_M_IX86))
# define PLAT_x86_win32 1
-#elif defined(__MINGW64__) \
- || (defined(_WIN64) && defined(_M_X64))
+#elif (defined(__MINGW32__) && defined(__x86_64__)) \
+ || (defined(_WIN32) && defined(_M_X64))
+/* __MINGW32__ and _WIN32 are defined in 64 bit mode as well. */
# define PLAT_amd64_win64 1
#elif defined(__linux__) && defined(__i386__)
# define PLAT_x86_linux 1
--
2.17.1
|
|
From: Bart V. A. <bva...@ac...> - 2020-04-27 18:55:10
|
On 2020-04-27 11:31, Martin Storsjö wrote: > Don't assume that __MINGW32__ implies x86; Windows runs on ARM/ARM64 > as well, and there are mingw toolchains that target those architectures. This patch has been applied on the master branch. Thanks for the patch! Bart. |
|
From: Martin S. <ma...@ma...> - 2020-04-27 18:57:13
|
On Mon, 27 Apr 2020, Bart Van Assche wrote: > On 2020-04-27 11:31, Martin Storsjö wrote: >> Don't assume that __MINGW32__ implies x86; Windows runs on ARM/ARM64 >> as well, and there are mingw toolchains that target those architectures. > > This patch has been applied on the master branch. Thanks for the patch! Thank you! // Martin |