diff mbox series

[FFmpeg-devel] configure: Set MSVC as_default later.

Message ID 20210115221247.1418-1-Reimar.Doeffinger@gmx.de
State New
Headers show
Series [FFmpeg-devel] configure: Set MSVC as_default later. | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Reimar Döffinger Jan. 15, 2021, 10:12 p.m. UTC
From: Reimar Döffinger <Reimar.Doeffinger@gmx.de>

It would get immediately overridden to $cc, which in case
of gas-preprocessor missing would result in it trying
to use cl.exe for asm files instead of erroring out.
This is because cl.exe does not fail but just print a warning
when it is given a file it does not know what to do with it...
---
 configure | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Martin Storsjö Jan. 15, 2021, 10:25 p.m. UTC | #1
On Fri, 15 Jan 2021, Reimar.Doeffinger@gmx.de wrote:

> From: Reimar Döffinger <Reimar.Doeffinger@gmx.de>
>
> It would get immediately overridden to $cc, which in case
> of gas-preprocessor missing would result in it trying
> to use cl.exe for asm files instead of erroring out.
> This is because cl.exe does not fail but just print a warning
> when it is given a file it does not know what to do with it...

As this setup seems to work fine in the setups I've tried, can you think 
of why it's overwritten with $cc in your cases?

With the line
     : ${as_default:=$cc}
it only sets as_default to $cc if $as_default is empty.


> ---
> configure | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/configure b/configure
> index 12b41cde1c..d3b665f6f9 100755
> --- a/configure
> +++ b/configure
> @@ -4271,14 +4271,6 @@ case "$toolchain" in
>         ld_default="$source_path/compat/windows/mslink"
>         nm_default="dumpbin.exe -symbols"
>         ar_default="lib.exe"
> -        case "$arch" in
> -        aarch64|arm64)
> -            as_default="armasm64.exe"
> -            ;;
> -        arm*)
> -            as_default="armasm.exe"
> -            ;;
> -        esac
>         target_os_default="win32"
>         # Use a relative path for TMPDIR. This makes sure all the
>         # ffconf temp files are written with a relative path, avoiding
> @@ -4720,6 +4712,14 @@ probe_cc(){
>         _ld_path='-libpath:'
>     elif $_cc -nologo- 2>&1 | grep -q Microsoft || { $_cc -v 2>&1 | grep -q clang && $_cc -? > /dev/null 2>&1; }; then
>         _type=msvc
> +        case "$arch" in
> +        aarch64|arm64)
> +            as_default="armasm64.exe"
> +            ;;
> +        arm*)
> +            as_default="armasm.exe"
> +            ;;
> +        esac
>         if $_cc -nologo- 2>&1 | grep -q Microsoft; then

You can't really do that here. Probe_cc only should set the existing set 
of _type/_ident/_ldflags/_cflags* etc variables, which are picked up by 
the caller of probe_cc. probe_cc is called separately for both host and 
target compilers, so if e.g. cross compiling, with MSVC as host compiler, 
with a different compiler for the target, this wouldn't do the right 
thing.

// Martin
Reimar Döffinger Jan. 15, 2021, 11:56 p.m. UTC | #2
> On 15 Jan 2021, at 23:25, Martin Storsjö <martin@martin.st> wrote:
> 
> On Fri, 15 Jan 2021, Reimar.Doeffinger@gmx.de wrote:
> 
>> From: Reimar Döffinger <Reimar.Doeffinger@gmx.de>
>> 
>> It would get immediately overridden to $cc, which in case
>> of gas-preprocessor missing would result in it trying
>> to use cl.exe for asm files instead of erroring out.
>> This is because cl.exe does not fail but just print a warning
>> when it is given a file it does not know what to do with it...
> 
> As this setup seems to work fine in the setups I've tried, can you think of why it's overwritten with $cc in your cases?
> 
> With the line
>    : ${as_default:=$cc}
> it only sets as_default to $cc if $as_default is empty.

Actually after a few debug prints it’s clear what actually happens:
$arch is not set at that point unless specified on command-line.
Not sure if it’s reasonable to just check arch_default as fallback or such?

> You can't really do that here. Probe_cc only should set the existing set of _type/_ident/_ldflags/_cflags* etc variables, which are picked up by the caller of probe_cc. probe_cc is called separately for both host and target compilers, so if e.g. cross compiling, with MSVC as host compiler, with a different compiler for the target, this wouldn't do the right thing.

Then the armcc logic in there is broken I guess?
Martin Storsjö Jan. 16, 2021, 8:20 p.m. UTC | #3
On Sat, 16 Jan 2021, Reimar Döffinger wrote:

>
>
>> On 15 Jan 2021, at 23:25, Martin Storsjö <martin@martin.st> wrote:
>> 
>> On Fri, 15 Jan 2021, Reimar.Doeffinger@gmx.de wrote:
>> 
>>> From: Reimar Döffinger <Reimar.Doeffinger@gmx.de>
>>> 
>>> It would get immediately overridden to $cc, which in case
>>> of gas-preprocessor missing would result in it trying
>>> to use cl.exe for asm files instead of erroring out.
>>> This is because cl.exe does not fail but just print a warning
>>> when it is given a file it does not know what to do with it...
>> 
>> As this setup seems to work fine in the setups I've tried, can you think of why it's overwritten with $cc in your cases?
>> 
>> With the line
>>    : ${as_default:=$cc}
>> it only sets as_default to $cc if $as_default is empty.
>
> Actually after a few debug prints it’s clear what actually happens:
> $arch is not set at that point unless specified on command-line.
> Not sure if it’s reasonable to just check arch_default as fallback or such?

Yeah that sounds sensible - maybe just "case ${arch-${arch_default}}" 
would work?

>> You can't really do that here. Probe_cc only should set the existing set of _type/_ident/_ldflags/_cflags* etc variables, which are picked up by the caller of probe_cc. probe_cc is called separately for both host and target compilers, so if e.g. cross compiling, with MSVC as host compiler, with a different compiler for the target, this wouldn't do the right thing.
>
> Then the armcc logic in there is broken I guess?

I would say so, yes - ideally that should be changed behave like the other 
vars like _cflags_* and picked up by caller, only for the target compiler 
case.

// Martin
diff mbox series

Patch

diff --git a/configure b/configure
index 12b41cde1c..d3b665f6f9 100755
--- a/configure
+++ b/configure
@@ -4271,14 +4271,6 @@  case "$toolchain" in
         ld_default="$source_path/compat/windows/mslink"
         nm_default="dumpbin.exe -symbols"
         ar_default="lib.exe"
-        case "$arch" in
-        aarch64|arm64)
-            as_default="armasm64.exe"
-            ;;
-        arm*)
-            as_default="armasm.exe"
-            ;;
-        esac
         target_os_default="win32"
         # Use a relative path for TMPDIR. This makes sure all the
         # ffconf temp files are written with a relative path, avoiding
@@ -4720,6 +4712,14 @@  probe_cc(){
         _ld_path='-libpath:'
     elif $_cc -nologo- 2>&1 | grep -q Microsoft || { $_cc -v 2>&1 | grep -q clang && $_cc -? > /dev/null 2>&1; }; then
         _type=msvc
+        case "$arch" in
+        aarch64|arm64)
+            as_default="armasm64.exe"
+            ;;
+        arm*)
+            as_default="armasm.exe"
+            ;;
+        esac
         if $_cc -nologo- 2>&1 | grep -q Microsoft; then
             _ident=$($_cc 2>&1 | head -n1 | tr -d '\r')
         else