diff mbox series

[Valgrind-developers] mingw: Fix arch detection ifdefs for non-x86 mingw platforms

Message ID 20200404203541.628-1-martin@martin.st
State New
Headers show
Series [Valgrind-developers] mingw: Fix arch detection ifdefs for non-x86 mingw platforms | expand

Commit Message

Martin Storsjo April 4, 2020, 8:35 p.m. UTC 
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(-)

-- 
2.17.1



_______________________________________________
Valgrind-developers mailing list
Valgrind-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/valgrind-developers

Comments

Martin Storsjo April 27, 2020, 1:16 p.m. UTC | #1 
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
Valgrind-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/valgrind-developers
Philippe Waroquiers April 27, 2020, 4:05 p.m. UTC | #2 
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
> Valgrind-developers@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/valgrind-developers
Bart Van Assche April 27, 2020, 5:58 p.m. UTC | #3 
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.
Martin Storsjo April 27, 2020, 6:04 p.m. UTC | #4 
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
_______________________________________________
Valgrind-developers mailing list
Valgrind-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/valgrind-developers
Bart Van Assche April 27, 2020, 6:09 p.m. UTC | #5 
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.
diff mbox series

Patch

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__)