| Message ID | 1508013322-19428-6-git-send-email-martin@martin.st |
|---|---|
| State | Committed |
| Headers | show |
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
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
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
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