[6/8] Define _WIN32 while preprocessing for armasm

Message ID 1508013322-19428-6-git-send-email-martin@martin.st
State Committed
Headers show

Commit Message

Martin Storsjö Oct. 14, 2017, 8:35 p.m.
---
 gas-preprocessor.pl | 1 +
 1 file changed, 1 insertion(+)

Comments

Janne Grunau Oct. 18, 2017, 6:24 a.m. | #1
On 2017-10-14 23:35:20 +0300, Martin Storsjö wrote:
> ---
>  gas-preprocessor.pl | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/gas-preprocessor.pl b/gas-preprocessor.pl
> index 456ee24..63b0ab3 100755
> --- a/gas-preprocessor.pl
> +++ b/gas-preprocessor.pl
> @@ -98,6 +98,7 @@ if ($as_type eq "armasm") {
>  
>      $preprocess_c_cmd[0] = "cpp";
>      push(@preprocess_c_cmd, "-undef");
> +    push(@preprocess_c_cmd, "-D_WIN32");
>  
>      @preprocess_c_cmd = grep ! /^-nologo$/, @preprocess_c_cmd;
>      # Remove -ignore XX parameter pairs from preprocess_c_cmd

this looks a little suspicious. Some code expect _WIN32 to be defined 
but msvc apparently doesn't define it. Unless there's a reason to expect 
this predefined in the preprocessor I'd prefer it fixed in the code 
instead.
I could live with this if it's expected to be required for more than 
just libav's asm.

Janne
Martin Storsjö Oct. 18, 2017, 6:43 a.m. | #2
On Wed, 18 Oct 2017, Janne Grunau wrote:

> On 2017-10-14 23:35:20 +0300, Martin Storsjö wrote:
>> ---
>>  gas-preprocessor.pl | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/gas-preprocessor.pl b/gas-preprocessor.pl
>> index 456ee24..63b0ab3 100755
>> --- a/gas-preprocessor.pl
>> +++ b/gas-preprocessor.pl
>> @@ -98,6 +98,7 @@ if ($as_type eq "armasm") {
>>
>>      $preprocess_c_cmd[0] = "cpp";
>>      push(@preprocess_c_cmd, "-undef");
>> +    push(@preprocess_c_cmd, "-D_WIN32");
>>
>>      @preprocess_c_cmd = grep ! /^-nologo$/, @preprocess_c_cmd;
>>      # Remove -ignore XX parameter pairs from preprocess_c_cmd
>
> this looks a little suspicious. Some code expect _WIN32 to be defined 
> but msvc apparently doesn't define it.

MSVC does define it normally, but we're using the plain "cpp" binary here, 
which in cross building setups is the local compiler's preprocessor - 
hence the -undef above to get rid of whatever other definitions that 
preprocessor sets.

> Unless there's a reason to expect this predefined in the preprocessor I'd prefer it fixed in the code 
> instead.
> I could live with this if it's expected to be required for more than 
> just libav's asm.

Well without this, there's no other define for detecting windows in the 
preprocessor, other than !__ELF__, !__MACH__ or !__APPLE__. When building 
with clang for windows, _WIN32 is defined (and we're using it in 
libavutil/aarch64/asm.S in one place), but here we're using an agnostic 
preprocessor.

// Martin
Janne Grunau Oct. 18, 2017, 7:04 a.m. | #3
On 2017-10-18 09:43:11 +0300, Martin Storsjö wrote:
> On Wed, 18 Oct 2017, Janne Grunau wrote:
> 
> >On 2017-10-14 23:35:20 +0300, Martin Storsjö wrote:
> >>---
> >> gas-preprocessor.pl | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >>diff --git a/gas-preprocessor.pl b/gas-preprocessor.pl
> >>index 456ee24..63b0ab3 100755
> >>--- a/gas-preprocessor.pl
> >>+++ b/gas-preprocessor.pl
> >>@@ -98,6 +98,7 @@ if ($as_type eq "armasm") {
> >>
> >>     $preprocess_c_cmd[0] = "cpp";
> >>     push(@preprocess_c_cmd, "-undef");
> >>+    push(@preprocess_c_cmd, "-D_WIN32");
> >>
> >>     @preprocess_c_cmd = grep ! /^-nologo$/, @preprocess_c_cmd;
> >>     # Remove -ignore XX parameter pairs from preprocess_c_cmd
> >
> >this looks a little suspicious. Some code expect _WIN32 to be defined but
> >msvc apparently doesn't define it.
> 
> MSVC does define it normally, but we're using the plain "cpp" binary here,
> which in cross building setups is the local compiler's preprocessor - hence
> the -undef above to get rid of whatever other definitions that preprocessor
> sets.

Please add this explanation either to the commit message or as comment.  
Patch ok

Janne

Patch

diff --git a/gas-preprocessor.pl b/gas-preprocessor.pl
index 456ee24..63b0ab3 100755
--- a/gas-preprocessor.pl
+++ b/gas-preprocessor.pl
@@ -98,6 +98,7 @@  if ($as_type eq "armasm") {
 
     $preprocess_c_cmd[0] = "cpp";
     push(@preprocess_c_cmd, "-undef");
+    push(@preprocess_c_cmd, "-D_WIN32");
 
     @preprocess_c_cmd = grep ! /^-nologo$/, @preprocess_c_cmd;
     # Remove -ignore XX parameter pairs from preprocess_c_cmd