gas: Fix \+ expansion for .irp and .irpc

  From: Fangrui Song <maskray@gcc.gnu.org>

.irp and .irpc receive a null macro_entry.  \+ causes a crash after the
recent \+ support.  Restore the previous behavior.

Signed-off-by: Fangrui Song <maskray@gcc.gnu.org>
---
 gas/macro.c                      | 2 +-
 gas/testsuite/gas/macros/count.l | 2 ++
 gas/testsuite/gas/macros/count.s | 4 ++++
 3 files changed, 7 insertions(+), 1 deletion(-)
  

On 15.05.2024 09:04, Fangrui Song wrote:
> From: Fangrui Song <maskray@gcc.gnu.org>
> 
> .irp and .irpc receive a null macro_entry.  \+ causes a crash after the
> recent \+ support.  Restore the previous behavior.

I did actually wonder when seeing the patch, but then came to the (wrong)
conclusion that - without any NULL passed to macro_expand_body() - there
would be no issue. It's imo bad practice to use 0 where NULL is meant.

In any event, the change is okay, maybe with ...

> --- a/gas/macro.c
> +++ b/gas/macro.c
> @@ -854,7 +854,7 @@ macro_expand_body (sb *in, sb *out, formal_entry *formals,
>  	      sprintf (buffer, "%u", macro_number);
>  	      sb_add_string (out, buffer);
>  	    }
> -	  else if (src < in->len && in->ptr[src] == '+')
> +	  else if (src < in->len && in->ptr[src] == '+' && macro)

... this switched to

	  else if (macro && src < in->len && in->ptr[src] == '+')

Jan
  

On Wed, May 15, 2024 at 12:12 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 15.05.2024 09:04, Fangrui Song wrote:
> > From: Fangrui Song <maskray@gcc.gnu.org>
> >
> > .irp and .irpc receive a null macro_entry.  \+ causes a crash after the
> > recent \+ support.  Restore the previous behavior.
>
> I did actually wonder when seeing the patch, but then came to the (wrong)
> conclusion that - without any NULL passed to macro_expand_body() - there
> would be no issue. It's imo bad practice to use 0 where NULL is meant.
>
> In any event, the change is okay, maybe with ...
>
> > --- a/gas/macro.c
> > +++ b/gas/macro.c
> > @@ -854,7 +854,7 @@ macro_expand_body (sb *in, sb *out, formal_entry *formals,
> >             sprintf (buffer, "%u", macro_number);
> >             sb_add_string (out, buffer);
> >           }
> > -       else if (src < in->len && in->ptr[src] == '+')
> > +       else if (src < in->len && in->ptr[src] == '+' && macro)
>
> ... this switched to
>
>           else if (macro && src < in->len && in->ptr[src] == '+')
>
> Jan

Ack.

While here, I will add .irpc coverage as well.
  

> Date: Wed, 15 May 2024 00:04:02 -0700
> From: Fangrui Song <maskray@google.com>

> From: Fangrui Song <maskray@gcc.gnu.org>
> 
> .irp and .irpc receive a null macro_entry.  \+ causes a crash after the
> recent \+ support.  Restore the previous behavior.

This patch (your commit b1d28350499d) caused the gas macro
test again to fail for cris-* e.g. cris-elf.  Please have a
look.  Thanks.

brgds, H-P
  

On Wed, May 15, 2024 at 5:47 PM Hans-Peter Nilsson <hp@axis.com> wrote:
>
> > Date: Wed, 15 May 2024 00:04:02 -0700
> > From: Fangrui Song <maskray@google.com>
>
> > From: Fangrui Song <maskray@gcc.gnu.org>
> >
> > .irp and .irpc receive a null macro_entry.  \+ causes a crash after the
> > recent \+ support.  Restore the previous behavior.
>
> This patch (your commit b1d28350499d) caused the gas macro
> test again to fail for cris-* e.g. cris-elf.  Please have a
> look.  Thanks.
>
> brgds, H-P

Sorry for the breakage. Does this work for you?

We could also split count.s, but perhaps the additional coverage is
not important for these targets.

--- i/gas/testsuite/gas/macros/macros.exp
+++ w/gas/testsuite/gas/macros/macros.exp
@@ -102,4 +102,7 @@ if [string match "" [lindex [gas_run ../all/excl.s
"-o /dev/null" ""] 0]] {
 gas_test_error "exit.s" "" ".exitm outside of a macro"

 run_list_test altmacro
+
+# ONLY_STANDARD_ESCAPES targets reject \+ in .irp/.irpc.
+setup_xfail "avr-*" "cris-*" "crisv32-*" "msp430-*" "z80-*"
 run_list_test count
  

> From: Fangrui Song <maskray@google.com>
> Date: Wed, 15 May 2024 18:30:26 -0700

> On Wed, May 15, 2024 at 5:47 PM Hans-Peter Nilsson <hp@axis.com> wrote:
> >
> > > Date: Wed, 15 May 2024 00:04:02 -0700
> > > From: Fangrui Song <maskray@google.com>
> >
> > > From: Fangrui Song <maskray@gcc.gnu.org>
> > >
> > > .irp and .irpc receive a null macro_entry.  \+ causes a crash after the
> > > recent \+ support.  Restore the previous behavior.
> >
> > This patch (your commit b1d28350499d) caused the gas macro
> > test again to fail for cris-* e.g. cris-elf.  Please have a
> > look.  Thanks.
> >
> > brgds, H-P
> 
> Sorry for the breakage. Does this work for you?

No.  This test has passed for cris-elf in the past.

IMO you should instead move your recent additions to that
test to a *new* test.  Never *add* to a test-case!

brgds, H-P
  

On 2024-05-16, Hans-Peter Nilsson wrote:
>> From: Fangrui Song <maskray@google.com>
>> Date: Wed, 15 May 2024 18:30:26 -0700
>
>> On Wed, May 15, 2024 at 5:47 PM Hans-Peter Nilsson <hp@axis.com> wrote:
>> >
>> > > Date: Wed, 15 May 2024 00:04:02 -0700
>> > > From: Fangrui Song <maskray@google.com>
>> >
>> > > From: Fangrui Song <maskray@gcc.gnu.org>
>> > >
>> > > .irp and .irpc receive a null macro_entry.  \+ causes a crash after the
>> > > recent \+ support.  Restore the previous behavior.
>> >
>> > This patch (your commit b1d28350499d) caused the gas macro
>> > test again to fail for cris-* e.g. cris-elf.  Please have a
>> > look.  Thanks.
>> >
>> > brgds, H-P
>>
>> Sorry for the breakage. Does this work for you?
>
>No.  This test has passed for cris-elf in the past.
>
>IMO you should instead move your recent additions to that
>test to a *new* test.  Never *add* to a test-case!
>
>brgds, H-P

Splitting the test requires some work...

How about this patch?

 From 7d92e6019f1ec9d467b1017dc681cfe011e67e3e Mon Sep 17 00:00:00 2001
From: Fangrui Song <maskray@gcc.gnu.org>
Date: Wed, 15 May 2024 19:31:18 -0700
Subject: [PATCH] gas: Fix \+ test for ONLY_STANDARD_ESCAPES targets

---
  gas/testsuite/gas/macros/count-repeat.d | 2 ++
  gas/testsuite/gas/macros/count-repeat.l | 3 +++
  gas/testsuite/gas/macros/count-repeat.s | 8 ++++++++
  gas/testsuite/gas/macros/count.l        | 2 --
  gas/testsuite/gas/macros/count.s        | 7 -------
  gas/testsuite/gas/macros/macros.exp     | 3 +++
  6 files changed, 16 insertions(+), 9 deletions(-)
  create mode 100644 gas/testsuite/gas/macros/count-repeat.d
  create mode 100644 gas/testsuite/gas/macros/count-repeat.l
  create mode 100644 gas/testsuite/gas/macros/count-repeat.s

diff --git a/gas/testsuite/gas/macros/count-repeat.d b/gas/testsuite/gas/macros/count-repeat.d
new file mode 100644
index 00000000000..93660420300
--- /dev/null
+++ b/gas/testsuite/gas/macros/count-repeat.d
@@ -0,0 +1,2 @@
+#name: Macro counters (count-repeat.d)
+# Tests \+ in .irp/.irpc/.rept
diff --git a/gas/testsuite/gas/macros/count-repeat.l b/gas/testsuite/gas/macros/count-repeat.l
new file mode 100644
index 00000000000..d4728e22c3a
--- /dev/null
+++ b/gas/testsuite/gas/macros/count-repeat.l
@@ -0,0 +1,3 @@
+\+
+\+
+\+
diff --git a/gas/testsuite/gas/macros/count-repeat.s b/gas/testsuite/gas/macros/count-repeat.s
new file mode 100644
index 00000000000..e09666db627
--- /dev/null
+++ b/gas/testsuite/gas/macros/count-repeat.s
@@ -0,0 +1,8 @@
+	.rept 1
+	.print "\+"
+	.endr
+	.print "\+"
+	.endr
+	.irpc i,1
+	.print "\+"
+	.endr
diff --git a/gas/testsuite/gas/macros/count.l b/gas/testsuite/gas/macros/count.l
index ca666ea0195..3418b0bab00 100644
--- a/gas/testsuite/gas/macros/count.l
+++ b/gas/testsuite/gas/macros/count.l
@@ -8,5 +8,3 @@
  1
  4
  2
-\+
-\+
diff --git a/gas/testsuite/gas/macros/count.s b/gas/testsuite/gas/macros/count.s
index 4a5b078ba60..c752ca8367d 100644
--- a/gas/testsuite/gas/macros/count.s
+++ b/gas/testsuite/gas/macros/count.s
@@ -17,10 +17,3 @@
  
  	mac1 2
  	mac2 3
-
-	.irp i,1
-	.print "\+"
-	.endr
-	.irpc i,1
-	.print "\+"
-	.endr
diff --git a/gas/testsuite/gas/macros/macros.exp b/gas/testsuite/gas/macros/macros.exp
index 3108f3fcd56..d5a18e3023c 100644
--- a/gas/testsuite/gas/macros/macros.exp
+++ b/gas/testsuite/gas/macros/macros.exp
@@ -103,3 +103,6 @@ gas_test_error "exit.s" "" ".exitm outside of a macro"
  
  run_list_test altmacro
  run_list_test count
+# ONLY_STANDARD_ESCAPES targets reject \+ in .irp/.irpc/.rept.
+setup_xfail "avr-*" "cris-*" "crisv32-*" "msp430-*" "z80-*"
+run_list_test count-repeat
  

> Date: Wed, 15 May 2024 19:42:01 -0700
> From: Fangrui Song <maskray@google.com>

> On 2024-05-16, Hans-Peter Nilsson wrote:
> >> From: Fangrui Song <maskray@google.com>
> >> Date: Wed, 15 May 2024 18:30:26 -0700
> >
> >> On Wed, May 15, 2024 at 5:47 PM Hans-Peter Nilsson <hp@axis.com> wrote:
> >> >
> >> > > Date: Wed, 15 May 2024 00:04:02 -0700
> >> > > From: Fangrui Song <maskray@google.com>
> >> >
> >> > > From: Fangrui Song <maskray@gcc.gnu.org>
> >> > >
> >> > > .irp and .irpc receive a null macro_entry.  \+ causes a crash after the
> >> > > recent \+ support.  Restore the previous behavior.
> >> >
> >> > This patch (your commit b1d28350499d) caused the gas macro
> >> > test again to fail for cris-* e.g. cris-elf.  Please have a
> >> > look.  Thanks.
> >> >
> >> > brgds, H-P
> >>
> >> Sorry for the breakage. Does this work for you?
> >
> >No.  This test has passed for cris-elf in the past.
> >
> >IMO you should instead move your recent additions to that
> >test to a *new* test.  Never *add* to a test-case!
> >
> >brgds, H-P
> 
> Splitting the test requires some work...
> 
> How about this patch?

Hmm... but shouldn't '.print "\+"' *work*?  I mean, it works
for cris-elf in the macro (with the new semantics), so it
should work "outside" as well (here in .irp), i.e. emit a
'+'?  There's no special semantics for '+' for cris-elf;
it's not a line-separator or comment-character or anything.
Did I misunderstand something here; isn't this a general bug
(guessing similar to the one Nick fixed) rather than a
target quirk?

Iff I *did* misunderstand something and there's no bug: from
the looks of it, I'd say ok from a maintenance view.

(No, I did not test it, but I don't actually think you asked
that, because you know the following.  Still, JFTR: in
binutils there's generally no target access problem to test
your own patches.  If you're new to cross-testing (surely
not?) just add "--target cris-elf", build and make check as
compared to your native build.)

brgds, H-P
  

On 2024-05-16, Hans-Peter Nilsson wrote:
>> Date: Wed, 15 May 2024 19:42:01 -0700
>> From: Fangrui Song <maskray@google.com>
>
>> On 2024-05-16, Hans-Peter Nilsson wrote:
>> >> From: Fangrui Song <maskray@google.com>
>> >> Date: Wed, 15 May 2024 18:30:26 -0700
>> >
>> >> On Wed, May 15, 2024 at 5:47 PM Hans-Peter Nilsson <hp@axis.com> wrote:
>> >> >
>> >> > > Date: Wed, 15 May 2024 00:04:02 -0700
>> >> > > From: Fangrui Song <maskray@google.com>
>> >> >
>> >> > > From: Fangrui Song <maskray@gcc.gnu.org>
>> >> > >
>> >> > > .irp and .irpc receive a null macro_entry.  \+ causes a crash after the
>> >> > > recent \+ support.  Restore the previous behavior.
>> >> >
>> >> > This patch (your commit b1d28350499d) caused the gas macro
>> >> > test again to fail for cris-* e.g. cris-elf.  Please have a
>> >> > look.  Thanks.
>> >> >
>> >> > brgds, H-P
>> >>
>> >> Sorry for the breakage. Does this work for you?
>> >
>> >No.  This test has passed for cris-elf in the past.
>> >
>> >IMO you should instead move your recent additions to that
>> >test to a *new* test.  Never *add* to a test-case!
>> >
>> >brgds, H-P
>>
>> Splitting the test requires some work...
>>
>> How about this patch?
>
>Hmm... but shouldn't '.print "\+"' *work*?  I mean, it works
>for cris-elf in the macro (with the new semantics), so it
>should work "outside" as well (here in .irp), i.e. emit a
>'+'?  There's no special semantics for '+' for cris-elf;
>it's not a line-separator or comment-character or anything.
>Did I misunderstand something here; isn't this a general bug
>(guessing similar to the one Nick fixed) rather than a
>target quirk?
>
>Iff I *did* misunderstand something and there's no bug: from
>the looks of it, I'd say ok from a maintenance view.

\+ is expanded by .macro directives. .irp .irpc .rept don't expand \=
and gas/read.c:5635 specifies that ONLY_STANDARD_ESCAPES targets will
reject unrecognized escaped characters.

% ~/Dev/binutils-gdb/out/cris/gas/as-new c.s
c.s: Assembler messages:
c.s:1: Error: bad escaped character in string
?
% ~/Dev/binutils-gdb/out/z80/gas/as-new c.s
c.s: Assembler messages:
c.s:1: Error: bad escaped character in string
?

>(No, I did not test it, but I don't actually think you asked
>that, because you know the following.  Still, JFTR: in
>binutils there's generally no target access problem to test
>your own patches.  If you're new to cross-testing (surely
>not?) just add "--target cris-elf", build and make check as
>compared to your native build.)
>
>brgds, H-P

I have experience testing targets popular on desktop/server but I have
never learned the full list of targets...

I guess there is probably no way other than enumerating every target and
doing a build...  I have done the following for cris-elf and z80-elf.

   (mkdir out/cris && cd out/cris
   ../../configure --target=cris-elf && make -j 50 all-gas && make -j 50 check-gas RUNTESTFLAGS='macros.exp')

   # some non-macro tests failed (e.g. FAIL: elf section2 list), but they are unrelated to this patch.

   (mkdir out/z80 && cd out/z80
   ../../configure --target=z80-elf && make -j 50 all-gas && make -j 50 check-gas RUNTESTFLAGS='macros.exp')
  

On 16.05.2024 04:42, Fangrui Song wrote:
> --- /dev/null
> +++ b/gas/testsuite/gas/macros/count-repeat.s
> @@ -0,0 +1,8 @@
> +	.rept 1
> +	.print "\+"
> +	.endr
> +	.print "\+"
> +	.endr
> +	.irpc i,1
> +	.print "\+"
> +	.endr

I can't help thinking that this can't possibly assemble (or if it does,
there's a bug elsewhere), for (presumably) missing an .irp line to
match the middle .endr.

Just to mention it - I'm intending to extend \+ support to at least .irp
and .irpc; whether that can also reasonably be done for .rept I haven't
checked yet.

Jan
  

On Wed, May 15, 2024 at 10:53 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 16.05.2024 04:42, Fangrui Song wrote:
> > --- /dev/null
> > +++ b/gas/testsuite/gas/macros/count-repeat.s
> > @@ -0,0 +1,8 @@
> > +     .rept 1
> > +     .print "\+"
> > +     .endr
> > +     .print "\+"
> > +     .endr
> > +     .irpc i,1
> > +     .print "\+"
> > +     .endr

> I can't help thinking that this can't possibly assemble (or if it does,
> there's a bug elsewhere), for (presumably) missing an .irp line to
> match the middle .endr.
>
> Just to mention it - I'm intending to extend \+ support to at least .irp
> and .irpc; whether that can also reasonably be done for .rept I haven't
> checked yet.
>
> Jan

I agree, extending \+ operator .irp, .irpc, and .rept will make a lot of sense.

We will finally get a for loop   (unsigned i = 0; i != count; i++)

.rept 3
.print "\+"   # 0 1 2
.endp

Previously, I resorted to .irpc i,0123456789   \i   .endr when the
loop count is <= 10, but there is no elegant way to reference \i when
the loop count is > 10.
  

On Wed, May 15, 2024 at 11:54 PM Fangrui Song <maskray@google.com> wrote:
>
> On Wed, May 15, 2024 at 10:53 PM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 16.05.2024 04:42, Fangrui Song wrote:
> > > --- /dev/null
> > > +++ b/gas/testsuite/gas/macros/count-repeat.s
> > > @@ -0,0 +1,8 @@
> > > +     .rept 1
> > > +     .print "\+"
> > > +     .endr
> > > +     .print "\+"
> > > +     .endr
> > > +     .irpc i,1
> > > +     .print "\+"
> > > +     .endr
>
> > I can't help thinking that this can't possibly assemble (or if it does,
> > there's a bug elsewhere), for (presumably) missing an .irp line to
> > match the middle .endr.
> >
> > Just to mention it - I'm intending to extend \+ support to at least .irp
> > and .irpc; whether that can also reasonably be done for .rept I haven't
> > checked yet.
> >
> > Jan
>
> I agree, extending \+ operator .irp, .irpc, and .rept will make a lot of sense.
>
> We will finally get a for loop   (unsigned i = 0; i != count; i++)
>
> .rept 3
> .print "\+"   # 0 1 2
> .endp
>
> Previously, I resorted to .irpc i,0123456789   \i   .endr when the
> loop count is <= 10, but there is no elegant way to reference \i when
> the loop count is > 10.

What should \+ expand to for nested loops?

.rept 2
  .rept 2
    .print "n\+"
  .endr
.endr
  

On 16.05.2024 09:00, Fangrui Song wrote:
> On Wed, May 15, 2024 at 11:54 PM Fangrui Song <maskray@google.com> wrote:
>>
>> On Wed, May 15, 2024 at 10:53 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 16.05.2024 04:42, Fangrui Song wrote:
>>>> --- /dev/null
>>>> +++ b/gas/testsuite/gas/macros/count-repeat.s
>>>> @@ -0,0 +1,8 @@
>>>> +     .rept 1
>>>> +     .print "\+"
>>>> +     .endr
>>>> +     .print "\+"
>>>> +     .endr
>>>> +     .irpc i,1
>>>> +     .print "\+"
>>>> +     .endr
>>
>>> I can't help thinking that this can't possibly assemble (or if it does,
>>> there's a bug elsewhere), for (presumably) missing an .irp line to
>>> match the middle .endr.
>>>
>>> Just to mention it - I'm intending to extend \+ support to at least .irp
>>> and .irpc; whether that can also reasonably be done for .rept I haven't
>>> checked yet.
>>>
>>> Jan
>>
>> I agree, extending \+ operator .irp, .irpc, and .rept will make a lot of sense.
>>
>> We will finally get a for loop   (unsigned i = 0; i != count; i++)
>>
>> .rept 3
>> .print "\+"   # 0 1 2
>> .endp
>>
>> Previously, I resorted to .irpc i,0123456789   \i   .endr when the
>> loop count is <= 10, but there is no elegant way to reference \i when
>> the loop count is > 10.
> 
> What should \+ expand to for nested loops?
> 
> .rept 2
>   .rept 2
>     .print "n\+"
>   .endr
> .endr

For macros I expect (didn't try yet) it's the outermost one, so I'd expect
.irp / .irpc / .rept would want to match that. I'll want to see how it ends
up most logically, yet without going through ugly extra hoops.

Jan
  

On 16.05.2024 09:53, Jan Beulich wrote:
> On 16.05.2024 09:00, Fangrui Song wrote:
>> On Wed, May 15, 2024 at 11:54 PM Fangrui Song <maskray@google.com> wrote:
>>>
>>> On Wed, May 15, 2024 at 10:53 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 16.05.2024 04:42, Fangrui Song wrote:
>>>>> --- /dev/null
>>>>> +++ b/gas/testsuite/gas/macros/count-repeat.s
>>>>> @@ -0,0 +1,8 @@
>>>>> +     .rept 1
>>>>> +     .print "\+"
>>>>> +     .endr
>>>>> +     .print "\+"
>>>>> +     .endr
>>>>> +     .irpc i,1
>>>>> +     .print "\+"
>>>>> +     .endr
>>>
>>>> I can't help thinking that this can't possibly assemble (or if it does,
>>>> there's a bug elsewhere), for (presumably) missing an .irp line to
>>>> match the middle .endr.
>>>>
>>>> Just to mention it - I'm intending to extend \+ support to at least .irp
>>>> and .irpc; whether that can also reasonably be done for .rept I haven't
>>>> checked yet.
>>>>
>>>> Jan
>>>
>>> I agree, extending \+ operator .irp, .irpc, and .rept will make a lot of sense.
>>>
>>> We will finally get a for loop   (unsigned i = 0; i != count; i++)
>>>
>>> .rept 3
>>> .print "\+"   # 0 1 2
>>> .endp
>>>
>>> Previously, I resorted to .irpc i,0123456789   \i   .endr when the
>>> loop count is <= 10, but there is no elegant way to reference \i when
>>> the loop count is > 10.
>>
>> What should \+ expand to for nested loops?
>>
>> .rept 2
>>   .rept 2
>>     .print "n\+"
>>   .endr
>> .endr
> 
> For macros I expect (didn't try yet) it's the outermost one, so I'd expect
> .irp / .irpc / .rept would want to match that. I'll want to see how it ends
> up most logically, yet without going through ugly extra hoops.

IOW in the end it may well be a matter of properly documenting things.

Jan
  

> Date: Wed, 15 May 2024 20:32:30 -0700
> From: Fangrui Song <maskray@google.com>

> and gas/read.c:5635 specifies that ONLY_STANDARD_ESCAPES targets will
> reject unrecognized escaped characters.

Ah, the semantics of ONLY_STANDARD_ESCAPES was what I was
missing - even though you correctly mention it in the
test-case!  (If there had been a bug, it would have been ok
to say that any bug here was pre-existing and you correctly
handle that by xfailing it. :-)

>    # some non-macro tests failed (e.g. FAIL: elf section2 list), but they are unrelated to this patch.

For me, at a09771e687ba z80-elf and cris-elf pass that test
and also run gas, binutils and ld tests without errors.

Thanks for resolving this so quickly!

brgds, H-P