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
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
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...
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
...
$
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
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
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;
}
}
$
@@ -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]);
@@ -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