gas: correct .irpc handling with empty string

Message ID 6ad96f10-087f-447c-b0e7-a649eed67c63@suse.com
State New
Headers
Series gas: correct .irpc handling with empty string |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Jan Beulich Aug. 12, 2024, 2:50 p.m. UTC
  Following 69cab370cf66 ("gas: adjust handling of quotes for .irpc") the
closing quote was mistakenly treated as the first quoted character.
---
I recall wondering about the lack of "continue" there. Yet I wrongly
concluded that since there was none, there also shouldn't be any.
  

Comments

Fangrui Song Aug. 14, 2024, 3:18 a.m. UTC | #1
On Mon, Aug 12, 2024 at 7:50 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> Following 69cab370cf66 ("gas: adjust handling of quotes for .irpc") the
> closing quote was mistakenly treated as the first quoted character.
> ---
> I recall wondering about the lack of "continue" there. Yet I wrongly
> concluded that since there was none, there also shouldn't be any.
>
> --- a/gas/macro.c
> +++ b/gas/macro.c
> @@ -1389,6 +1389,7 @@ expand_irp (int irpc, size_t idx, sb *in
>                       if (idx >= in->len)
>                         break;
>                     }
> +                 continue;
>                 }
>               sb_reset (&f.actual);
>               sb_add_char (&f.actual, in->ptr[idx]);
> --- a/gas/testsuite/gas/macros/irpc-quote.s
> +++ b/gas/testsuite/gas/macros/irpc-quote.s
> @@ -1,6 +1,6 @@
> -       .irpc c, " ab" cd " ef"
> +       .irpc c, " ab" cd " ef" ""
>         .print ">\c<"
>         .endr
> -       .irpc c, "12 " 34 "56 "
> +       .irpc c, "" "12 " 34 "56 "
>         .print ">\c<"
>         .endr

It seems that .irc c, ab cd   has always been supported while .irpc c,
"ab" "cd" is only supported by your previous patch.

This patch looks good.  If   .irpc c, ab cd    were not supported
previously, I would suggest that we just don't allow space-separated
arguments...
  
Bernhard Übelacker Aug. 14, 2024, 1:30 p.m. UTC | #2
Am 12.08.24 um 16:50 schrieb Jan Beulich:
> Following 69cab370cf66 ("gas: adjust handling of quotes for .irpc") the
> closing quote was mistakenly treated as the first quoted character.
> ---
> I recall wondering about the lack of "continue" there. Yet I wrongly
> concluded that since there was none, there also shouldn't be any.
> 
> --- a/gas/macro.c
> +++ b/gas/macro.c
> @@ -1389,6 +1389,7 @@ expand_irp (int irpc, size_t idx, sb *in
>   		      if (idx >= in->len)
>   			break;
>   		    }
> +		  continue;
>   		}
>   	      sb_reset (&f.actual);
>   	      sb_add_char (&f.actual, in->ptr[idx]);
> --- a/gas/testsuite/gas/macros/irpc-quote.s
> +++ b/gas/testsuite/gas/macros/irpc-quote.s
> @@ -1,6 +1,6 @@
> -	.irpc c, " ab" cd " ef"
> +	.irpc c, " ab" cd " ef" ""
>   	.print ">\c<"
>   	.endr
> -	.irpc c, "12 " 34 "56 "
> +	.irpc c, "" "12 " 34 "56 "
>   	.print ">\c<"
>   	.endr

Hello Jan,
I am not sure if this patch should handle the issue in the Debian bug completely.

At least I tested current git with your patch applied, but the issue seems still visible.

For convenience I tried to put a minimal reproducer together [1].

In [2] is the example output of an working version 2.42.

In [3] the output of current git with your patch.
It still shows the warning messages.
And it seems the parameter string_a is seen like it contains 0x0a.

Kind regards,
Bernhard






[1]
$ git diff --cached
diff --git a/gas/testsuite/gas/macros/irpc-nested-macro.d b/gas/testsuite/gas/macros/irpc-nested-macro.d
new file mode 100644
index 00000000000..38214ac730b
--- /dev/null
+++ b/gas/testsuite/gas/macros/irpc-nested-macro.d
@@ -0,0 +1,6 @@
+#objdump: -s -j .data
+
+.*: .*
+
+Contents of section .data:
+ 0000 61002000 62000000                    a. .b...
diff --git a/gas/testsuite/gas/macros/irpc-nested-macro.s b/gas/testsuite/gas/macros/irpc-nested-macro.s
new file mode 100644
index 00000000000..c31e027e487
--- /dev/null
+++ b/gas/testsuite/gas/macros/irpc-nested-macro.s
@@ -0,0 +1,19 @@
+/*
+rm -f irpc-nested-macro.o
+i686-w64-mingw32-as -o irpc-nested-macro.o irpc-nested-macro.s
+i686-w64-mingw32-objdump -s -j .data irpc-nested-macro.o | tee dump.out
+*/
+
+.macro macro2 string_c
+.irpc    char_c,"\string_c"
+.asciz  "\char_c"
+.endr
+.endm
+
+.macro macro1 string_a="", string_b=""
+macro2 "\string_a"
+macro2 "\string_b"
+.endm
+
+.data
+macro1 string_b="a b"
$




[2]
$ rm -f irpc-nested-macro.o
$ i686-w64-mingw32-as -o irpc-nested-macro.o irpc-nested-macro.s
$ i686-w64-mingw32-objdump -s -j .data irpc-nested-macro.o | tee dump_2.42-4+11.5.out

irpc-nested-macro.o:     file format pe-i386

Contents of section .data:
  0000 61002000 62000000                    a. .b...
$ i686-w64-mingw32-as --version
GNU assembler (GNU Binutils) 2.42
...
$




[3]
$ rm -f irpc-nested-macro.o
$ ../../../as-new -o irpc-nested-macro.o irpc-nested-macro.s
irpc-nested-macro.s: Assembler messages:
irpc-nested-macro.s:19: Warning: end of file in string; '"' inserted
irpc-nested-macro.s:14:   Info: macro invoked from here
irpc-nested-macro.s:19:    Info: macro invoked from here
irpc-nested-macro.s:9: Warning: unterminated string; newline inserted
irpc-nested-macro.s:14:   Info: macro invoked from here
irpc-nested-macro.s:19:    Info: macro invoked from here
$ i686-w64-mingw32-objdump -s -j .data irpc-nested-macro.o | tee dump_git+patch.out

irpc-nested-macro.o:     file format pe-i386

Contents of section .data:
  0000 0a006100 20006200                    ..a. .b.
$ ../../../as-new --version
GNU assembler (GNU Binutils) 2.43.50.20240814
...
$
  
Jan Beulich Aug. 14, 2024, 1:34 p.m. UTC | #3
On 14.08.2024 15:30, Bernhard Übelacker wrote:
> Am 12.08.24 um 16:50 schrieb Jan Beulich:
>> Following 69cab370cf66 ("gas: adjust handling of quotes for .irpc") the
>> closing quote was mistakenly treated as the first quoted character.
>> ---
>> I recall wondering about the lack of "continue" there. Yet I wrongly
>> concluded that since there was none, there also shouldn't be any.
>>
>> --- a/gas/macro.c
>> +++ b/gas/macro.c
>> @@ -1389,6 +1389,7 @@ expand_irp (int irpc, size_t idx, sb *in
>>   		      if (idx >= in->len)
>>   			break;
>>   		    }
>> +		  continue;
>>   		}
>>   	      sb_reset (&f.actual);
>>   	      sb_add_char (&f.actual, in->ptr[idx]);
>> --- a/gas/testsuite/gas/macros/irpc-quote.s
>> +++ b/gas/testsuite/gas/macros/irpc-quote.s
>> @@ -1,6 +1,6 @@
>> -	.irpc c, " ab" cd " ef"
>> +	.irpc c, " ab" cd " ef" ""
>>   	.print ">\c<"
>>   	.endr
>> -	.irpc c, "12 " 34 "56 "
>> +	.irpc c, "" "12 " 34 "56 "
>>   	.print ">\c<"
>>   	.endr
> 
> Hello Jan,
> I am not sure if this patch should handle the issue in the Debian bug completely.
> 
> At least I tested current git with your patch applied, but the issue seems still visible.

I'm surprised, because I actually tried with the full example you pointed at,
and the resulting .o was the same again as it was with 2.42. I'll double-check
though ...

Jan
  
Jan Beulich Aug. 14, 2024, 1:53 p.m. UTC | #4
On 14.08.2024 15:30, Bernhard Übelacker wrote:
> $ rm -f irpc-nested-macro.o
> $ ../../../as-new -o irpc-nested-macro.o irpc-nested-macro.s
> irpc-nested-macro.s: Assembler messages:
> irpc-nested-macro.s:19: Warning: end of file in string; '"' inserted
> irpc-nested-macro.s:14:   Info: macro invoked from here
> irpc-nested-macro.s:19:    Info: macro invoked from here
> irpc-nested-macro.s:9: Warning: unterminated string; newline inserted
> irpc-nested-macro.s:14:   Info: macro invoked from here
> irpc-nested-macro.s:19:    Info: macro invoked from here

No warnings here, and ...

> $ i686-w64-mingw32-objdump -s -j .data irpc-nested-macro.o | tee dump_git+patch.out
> 
> irpc-nested-macro.o:     file format pe-i386
> 
> Contents of section .data:
>   0000 0a006100 20006200                    ..a. .b.

... no 0a00 at the beginning of the output.

> $ ../../../as-new --version
> GNU assembler (GNU Binutils) 2.43.50.20240814

Since the date alone isn't precise enough, could you double check that the
commit in question was actually included?

Jan
  
Bernhard Übelacker Aug. 14, 2024, 3:26 p.m. UTC | #5
Am 14.08.24 um 15:53 schrieb Jan Beulich:
> On 14.08.2024 15:30, Bernhard Übelacker wrote:
>> $ rm -f irpc-nested-macro.o
>> $ ../../../as-new -o irpc-nested-macro.o irpc-nested-macro.s
>> irpc-nested-macro.s: Assembler messages:
>> irpc-nested-macro.s:19: Warning: end of file in string; '"' inserted
>> irpc-nested-macro.s:14:   Info: macro invoked from here
>> irpc-nested-macro.s:19:    Info: macro invoked from here
>> irpc-nested-macro.s:9: Warning: unterminated string; newline inserted
>> irpc-nested-macro.s:14:   Info: macro invoked from here
>> irpc-nested-macro.s:19:    Info: macro invoked from here
> 
> No warnings here, and ...
> 
>> $ i686-w64-mingw32-objdump -s -j .data irpc-nested-macro.o | tee dump_git+patch.out
>>
>> irpc-nested-macro.o:     file format pe-i386
>>
>> Contents of section .data:
>>    0000 0a006100 20006200                    ..a. .b.
> 
> ... no 0a00 at the beginning of the output.
> 
>> $ ../../../as-new --version
>> GNU assembler (GNU Binutils) 2.43.50.20240814
> 
> Since the date alone isn't precise enough, could you double check that the
> commit in question was actually included?
> 
> Jan


Hello Jan,
my test was at the master branch at a4c88987f manually applying your patch
by copying it out of the mailing list archive.
But this failed - I ignored the testsuite part
and the "continue" ended up in the wrong function.
Most probably caused by my copying removed all the tabs.

I just now see your patch got committed
and I re-tested again with d15180ebed.
This generates the expected bytes again and no warnings.

Sorry for the noise, I will try better to take
care next time I manually apply patches.

Kind regards,
Bernhard





$ git checkout a4c88987f07
HEAD ist jetzt bei a4c88987f07 gdb: remove unnecessary code relating to limited-length arrays
$ cat ../tmp-irpc-handling-manual.patch
--- a/gas/macro.c
+++ b/gas/macro.c
@@ -1389,6 +1389,7 @@ expand_irp (int irpc, size_t idx, sb *in
               if (idx >= in->len)
             break;
             }
+                 continue;
         }
           sb_reset (&f.actual);
           sb_add_char (&f.actual, in->ptr[idx]);
--- a/gas/testsuite/gas/macros/irpc-quote.s
+++ b/gas/testsuite/gas/macros/irpc-quote.s
@@ -1,6 +1,6 @@
-       .irpc c, " ab" cd " ef"
+       .irpc c, " ab" cd " ef" ""
     .print ">\c<"
     .endr
-       .irpc c, "12 " 34 "56 "
+       .irpc c, "" "12 " 34 "56 "
     .print ">\c<"
     .endr
$ git log --oneline | head -n1
d15180ebed2 LoongArch: Fix DT_RELR and relaxation interaction
$ ./configure --target=i686-w64-mingw32 --disable-gdb
$ make -j16
...
$ patch -p1 < ../minimal-test.patch
patching file gas/testsuite/gas/macros/irpc-nested-macro.d
patching file gas/testsuite/gas/macros/irpc-nested-macro.s
$ cd gas/testsuite/gas/macros
$ patch -p1 < ../tmp-irpc-handling-manual.patch
patching file gas/macro.c
Hunk #1 succeeded at 1234 with fuzz 2 (offset -155 lines).
patching file gas/testsuite/gas/macros/irpc-quote.s
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file gas/testsuite/gas/macros/irpc-quote.s.rej
$ git diff
diff --git a/gas/macro.c b/gas/macro.c
index a35e1356bbf..4b316fa7b0b 100644
--- a/gas/macro.c
+++ b/gas/macro.c
@@ -1234,6 +1234,7 @@ macro_expand (size_t idx, sb *in, macro_entry *m, sb *out)
               del_formal (*pf);
               *pf = f;
             }
+                 continue;
         }
      }
  
$
  

Patch

--- a/gas/macro.c
+++ b/gas/macro.c
@@ -1389,6 +1389,7 @@  expand_irp (int irpc, size_t idx, sb *in
 		      if (idx >= in->len)
 			break;
 		    }
+		  continue;
 		}
 	      sb_reset (&f.actual);
 	      sb_add_char (&f.actual, in->ptr[idx]);
--- a/gas/testsuite/gas/macros/irpc-quote.s
+++ b/gas/testsuite/gas/macros/irpc-quote.s
@@ -1,6 +1,6 @@ 
-	.irpc c, " ab" cd " ef"
+	.irpc c, " ab" cd " ef" ""
 	.print ">\c<"
 	.endr
-	.irpc c, "12 " 34 "56 "
+	.irpc c, "" "12 " 34 "56 "
 	.print ">\c<"
 	.endr