configure: fix inline asm checks

Message ID 20180607120321.2548-1-martin@martin.st
State Committed
Commit 52fd2afce8436c59c05765f3a6e95f9adb6f9f2f
Headers show
Series
  • configure: fix inline asm checks
Related show

Commit Message

Martin Storsjö June 7, 2018, 12:03 p.m.
From: John Cox <jc@kynesim.co.uk>

Commit 8c893aa3cd5 removed quotes that were required to detect
inline asm:

check_insn armv5te qadd r0, r0, r0
.../test.c:1:34: error: expected string literal in 'asm'
void foo(void){ __asm__ volatile(qadd r0, r0, r0); }

The correct code is:

void foo(void){ __asm__ volatile("qadd r0, r0, r0"); }

Commit message written by Frank Liberato <liberato@chromium.org>
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Luca Barbato June 7, 2018, 2:05 p.m. | #1
On 07/06/2018 14:03, Martin Storsjö wrote:
> From: John Cox <jc@kynesim.co.uk>
> 
> Commit 8c893aa3cd5 removed quotes that were required to detect
> inline asm:
> 
> check_insn armv5te qadd r0, r0, r0
> .../test.c:1:34: error: expected string literal in 'asm'
> void foo(void){ __asm__ volatile(qadd r0, r0, r0); }
> 
> The correct code is:
> 
> void foo(void){ __asm__ volatile("qadd r0, r0, r0"); }
> 
> Commit message written by Frank Liberato <liberato@chromium.org>
> ---
>  configure | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 5e79c0cec1..48e8536b07 100755
> --- a/configure
> +++ b/configure
> @@ -866,7 +866,7 @@ EOF
>  
>  check_insn(){
>      log check_insn "$@"
> -    check_inline_asm ${1}_inline "$2"
> +    check_inline_asm ${1}_inline "\"$2\""
>      check_as ${1}_external "$2"
>  }
>  
> 

Sure.
Diego Biurrun June 7, 2018, 9:01 p.m. | #2
On Thu, Jun 07, 2018 at 03:03:21PM +0300, Martin Storsjö wrote:
> Commit 8c893aa3cd5 removed quotes that were required to detect
> inline asm:
> 
> check_insn armv5te qadd r0, r0, r0
> .../test.c:1:34: error: expected string literal in 'asm'
> void foo(void){ __asm__ volatile(qadd r0, r0, r0); }
> 
> The correct code is:
> 
> void foo(void){ __asm__ volatile("qadd r0, r0, r0"); }
> --- a/configure
> +++ b/configure
> @@ -866,7 +866,7 @@ EOF
>  check_insn(){
>      log check_insn "$@"
> -    check_inline_asm ${1}_inline "$2"
> +    check_inline_asm ${1}_inline "\"$2\""
>      check_as ${1}_external "$2"
>  }

This does not look like the correct fix to me. The required quotes
should be part of the convenience function instead. Notice how calls
to check_insn and check_inline_asm differ in the way they quote their
arguments. There should be no need for this inconsistency.

I'll look into it.

Diego
James Almer June 8, 2018, 2:05 a.m. | #3
On 6/7/2018 6:01 PM, Diego Biurrun wrote:
> On Thu, Jun 07, 2018 at 03:03:21PM +0300, Martin Storsjö wrote:
>> Commit 8c893aa3cd5 removed quotes that were required to detect
>> inline asm:
>>
>> check_insn armv5te qadd r0, r0, r0
>> .../test.c:1:34: error: expected string literal in 'asm'
>> void foo(void){ __asm__ volatile(qadd r0, r0, r0); }
>>
>> The correct code is:
>>
>> void foo(void){ __asm__ volatile("qadd r0, r0, r0"); }
>> --- a/configure
>> +++ b/configure
>> @@ -866,7 +866,7 @@ EOF
>>  check_insn(){
>>      log check_insn "$@"
>> -    check_inline_asm ${1}_inline "$2"
>> +    check_inline_asm ${1}_inline "\"$2\""
>>      check_as ${1}_external "$2"
>>  }
> 
> This does not look like the correct fix to me. The required quotes
> should be part of the convenience function instead. Notice how calls
> to check_insn and check_inline_asm differ in the way they quote their
> arguments. There should be no need for this inconsistency.
> 
> I'll look into it.

Changing all the calls from check_insn name 'insn' to check_insn name
'"insn"' would probably fix the check_inline_asm tests, but may break
the check_as tests.

I don't have an arm or qemu system to confirm the above, though.

> 
> Diego
> _______________________________________________
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
>
Diego Biurrun June 8, 2018, 10:30 a.m. | #4
On Thu, Jun 07, 2018 at 11:05:26PM -0300, James Almer wrote:
> On 6/7/2018 6:01 PM, Diego Biurrun wrote:
> > On Thu, Jun 07, 2018 at 03:03:21PM +0300, Martin Storsjö wrote:
> >> Commit 8c893aa3cd5 removed quotes that were required to detect
> >> inline asm:
> >>
> >> check_insn armv5te qadd r0, r0, r0
> >> .../test.c:1:34: error: expected string literal in 'asm'
> >> void foo(void){ __asm__ volatile(qadd r0, r0, r0); }
> >>
> >> The correct code is:
> >>
> >> void foo(void){ __asm__ volatile("qadd r0, r0, r0"); }
> >> --- a/configure
> >> +++ b/configure
> >> @@ -866,7 +866,7 @@ EOF
> >>  check_insn(){
> >>      log check_insn "$@"
> >> -    check_inline_asm ${1}_inline "$2"
> >> +    check_inline_asm ${1}_inline "\"$2\""
> >>      check_as ${1}_external "$2"
> >>  }
> > 
> > This does not look like the correct fix to me. The required quotes
> > should be part of the convenience function instead. Notice how calls
> > to check_insn and check_inline_asm differ in the way they quote their
> > arguments. There should be no need for this inconsistency.
> > 
> > I'll look into it.
> 
> Changing all the calls from check_insn name 'insn' to check_insn name
> '"insn"' would probably fix the check_inline_asm tests, but may break
> the check_as tests.

Complicating the function calls is not the right way to go. The helper
function should take care of the required quoting and not rely on the
callers to pass arguments in nested quotes.

Diego
Martin Storsjö June 25, 2018, 11:41 a.m. | #5
On Fri, 8 Jun 2018, Diego Biurrun wrote:

> On Thu, Jun 07, 2018 at 11:05:26PM -0300, James Almer wrote:
>> On 6/7/2018 6:01 PM, Diego Biurrun wrote:
>> > On Thu, Jun 07, 2018 at 03:03:21PM +0300, Martin Storsjö wrote:
>> >> Commit 8c893aa3cd5 removed quotes that were required to detect
>> >> inline asm:
>> >>
>> >> check_insn armv5te qadd r0, r0, r0
>> >> .../test.c:1:34: error: expected string literal in 'asm'
>> >> void foo(void){ __asm__ volatile(qadd r0, r0, r0); }
>> >>
>> >> The correct code is:
>> >>
>> >> void foo(void){ __asm__ volatile("qadd r0, r0, r0"); }
>> >> --- a/configure
>> >> +++ b/configure
>> >> @@ -866,7 +866,7 @@ EOF
>> >>  check_insn(){
>> >>      log check_insn "$@"
>> >> -    check_inline_asm ${1}_inline "$2"
>> >> +    check_inline_asm ${1}_inline "\"$2\""
>> >>      check_as ${1}_external "$2"
>> >>  }
>> > 
>> > This does not look like the correct fix to me. The required quotes
>> > should be part of the convenience function instead. Notice how calls
>> > to check_insn and check_inline_asm differ in the way they quote their
>> > arguments. There should be no need for this inconsistency.
>> > 
>> > I'll look into it.
>> 
>> Changing all the calls from check_insn name 'insn' to check_insn name
>> '"insn"' would probably fix the check_inline_asm tests, but may break
>> the check_as tests.
>
> Complicating the function calls is not the right way to go. The helper
> function should take care of the required quoting and not rely on the
> callers to pass arguments in nested quotes.

Ping; whoever is waiting for the other, please pick the thread up again.

// Martin
Luca Barbato June 25, 2018, 2:14 p.m. | #6
On 25/06/2018 13:41, Martin Storsjö wrote:
> On Fri, 8 Jun 2018, Diego Biurrun wrote:
> 
>> On Thu, Jun 07, 2018 at 11:05:26PM -0300, James Almer wrote:
>>> On 6/7/2018 6:01 PM, Diego Biurrun wrote:
>>> > On Thu, Jun 07, 2018 at 03:03:21PM +0300, Martin Storsjö wrote:
>>> >> Commit 8c893aa3cd5 removed quotes that were required to detect
>>> >> inline asm:
>>> >>
>>> >> check_insn armv5te qadd r0, r0, r0
>>> >> .../test.c:1:34: error: expected string literal in 'asm'
>>> >> void foo(void){ __asm__ volatile(qadd r0, r0, r0); }
>>> >>
>>> >> The correct code is:
>>> >>
>>> >> void foo(void){ __asm__ volatile("qadd r0, r0, r0"); }
>>> >> --- a/configure
>>> >> +++ b/configure
>>> >> @@ -866,7 +866,7 @@ EOF
>>> >>  check_insn(){
>>> >>      log check_insn "$@"
>>> >> -    check_inline_asm ${1}_inline "$2"
>>> >> +    check_inline_asm ${1}_inline "\"$2\""
>>> >>      check_as ${1}_external "$2"
>>> >>  }
>>> > > This does not look like the correct fix to me. The required quotes
>>> > should be part of the convenience function instead. Notice how calls
>>> > to check_insn and check_inline_asm differ in the way they quote their
>>> > arguments. There should be no need for this inconsistency.
>>> > > I'll look into it.
>>>
>>> Changing all the calls from check_insn name 'insn' to check_insn name
>>> '"insn"' would probably fix the check_inline_asm tests, but may break
>>> the check_as tests.
>>
>> Complicating the function calls is not the right way to go. The helper
>> function should take care of the required quoting and not rely on the
>> callers to pass arguments in nested quotes.
> 
> Ping; whoever is waiting for the other, please pick the thread up again.
> 

I'd wait another day and merge the proposed fix if nothing better
materializes.

lu
Diego Biurrun June 29, 2018, 10:38 a.m. | #7
On Mon, Jun 25, 2018 at 02:41:30PM +0300, Martin Storsjö wrote:
> On Fri, 8 Jun 2018, Diego Biurrun wrote:
> 
> > On Thu, Jun 07, 2018 at 11:05:26PM -0300, James Almer wrote:
> > > On 6/7/2018 6:01 PM, Diego Biurrun wrote:
> > > > On Thu, Jun 07, 2018 at 03:03:21PM +0300, Martin Storsjö wrote:
> > > >> Commit 8c893aa3cd5 removed quotes that were required to detect
> > > >> inline asm:
> > > >>
> > > >> check_insn armv5te qadd r0, r0, r0
> > > >> .../test.c:1:34: error: expected string literal in 'asm'
> > > >> void foo(void){ __asm__ volatile(qadd r0, r0, r0); }
> > > >>
> > > >> The correct code is:
> > > >>
> > > >> void foo(void){ __asm__ volatile("qadd r0, r0, r0"); }
> > > >> --- a/configure
> > > >> +++ b/configure
> > > >> @@ -866,7 +866,7 @@ EOF
> > > >>  check_insn(){
> > > >>      log check_insn "$@"
> > > >> -    check_inline_asm ${1}_inline "$2"
> > > >> +    check_inline_asm ${1}_inline "\"$2\""
> > > >>      check_as ${1}_external "$2"
> > > >>  }
> > > > > This does not look like the correct fix to me. The required
> > > quotes
> > > > should be part of the convenience function instead. Notice how calls
> > > > to check_insn and check_inline_asm differ in the way they quote their
> > > > arguments. There should be no need for this inconsistency.
> > > > > I'll look into it.
> > > 
> > > Changing all the calls from check_insn name 'insn' to check_insn name
> > > '"insn"' would probably fix the check_inline_asm tests, but may break
> > > the check_as tests.
> > 
> > Complicating the function calls is not the right way to go. The helper
> > function should take care of the required quoting and not rely on the
> > callers to pass arguments in nested quotes.
> 
> Ping; whoever is waiting for the other, please pick the thread up again.

Feel free to push this, I'll come up with a better fix later.

Diego

Patch

diff --git a/configure b/configure
index 5e79c0cec1..48e8536b07 100755
--- a/configure
+++ b/configure
@@ -866,7 +866,7 @@  EOF
 
 check_insn(){
     log check_insn "$@"
-    check_inline_asm ${1}_inline "$2"
+    check_inline_asm ${1}_inline "\"$2\""
     check_as ${1}_external "$2"
 }