[3/3] gas: Add a macro test with expression argument

Message ID 20240811231107.590443-4-hjl.tools@gmail.com
State Superseded
Headers
Series Restore macro with expression argument |

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

H.J. Lu Aug. 11, 2024, 11:11 p.m. UTC
  Add a macro test with expression argument like

---
	.macro test arg1
	.byte \arg1
	.endm

	.data
 	test  0x10 + 0
 	test  0x10 + 1
---

	PR gas/32073
	* testsuite/gas/macros/arg1.d: New file.
	* testsuite/gas/macros/arg1.s: Likewise.
	* testsuite/gas/macros/macros.exp: Run arg1.

Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
---
 gas/testsuite/gas/macros/arg1.d     | 8 ++++++++
 gas/testsuite/gas/macros/arg1.s     | 7 +++++++
 gas/testsuite/gas/macros/macros.exp | 2 ++
 3 files changed, 17 insertions(+)
 create mode 100644 gas/testsuite/gas/macros/arg1.d
 create mode 100644 gas/testsuite/gas/macros/arg1.s
  

Comments

Jan Beulich Aug. 12, 2024, 7:39 a.m. UTC | #1
On 12.08.2024 01:11, H.J. Lu wrote:
> --- /dev/null
> +++ b/gas/testsuite/gas/macros/arg1.s
> @@ -0,0 +1,7 @@
> +	.macro test arg1
> +	.byte \arg1
> +	.endm
> +
> +	.data
> + 	test  0x10 + 0
> + 	test  0x10 + 1

So this is precisely what that entire change was about that you're proposing
to revert. Within binutils, instances of such broken macro invocations were
deliberately fixed up front. And the NEWS entry the commit added also is very
clear about the above no longer working (and never having been guaranteed to
work).

Just look at it the way it is textually present above: Knowing that macro
arguments don't require commas as separators, how many arguments do you see?
And no, using knowledge on the internal workings (brokenness) of the scrubber
is not allowed to determine the answer. (My answer: Three. And that's what
gas also should determine.)

If, purely from a practical / pragmatic perspective we'd really need to keep
the above working for some more time, then I expect we'll need to invent a
mode within which we can warn about such broken macros, telling people that
new behavior will be enforced in, say, the next release. Otherwise how do
you propose we ever address (without heuristics) the issues that the changes
at hand are actually aiming at addressing?

I did actually think about possible transitional states. Yet I didn't figure
any that would be halfway sane _and_ useful. For example, while we could add
a command line option to request old vs new scrubbing modes. To be useful
(for the purpose of fully transitioning sooner or later), that ought to
default to "new", though. Yet then people will need to fix their code anyway,
just (possibly) by adding the new command line option instead of touching
assembly sources.

And then: I deliberately waited for comments much longer than I would usually
have done. No-one really cared to comment on the changing behavior. And hence
I'm a little irritated that now not just a possible workaround is suggested,
but outright reverting.

Bottom line: Clearly a NAK for this testsuite addition. We must not test
accepting input in ways other than (more or less) documented. (Documentation
isn't great for macros, but the possibility of not using commas to separate
parameters and arguments can at least be derived from reading what's there.)
As to reverting the two commits while working out a possible transition path:
I'm open to that, provided some constructive comments actually surface, so
we / I have a way forward.

Jan
  
H.J. Lu Aug. 12, 2024, 11:21 a.m. UTC | #2
On Mon, Aug 12, 2024 at 12:39 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 12.08.2024 01:11, H.J. Lu wrote:
> > --- /dev/null
> > +++ b/gas/testsuite/gas/macros/arg1.s
> > @@ -0,0 +1,7 @@
> > +     .macro test arg1
> > +     .byte \arg1
> > +     .endm
> > +
> > +     .data
> > +     test  0x10 + 0
> > +     test  0x10 + 1
>
> So this is precisely what that entire change was about that you're proposing
> to revert. Within binutils, instances of such broken macro invocations were
> deliberately fixed up front. And the NEWS entry the commit added also is very
> clear about the above no longer working (and never having been guaranteed to
> work).
>
> Just look at it the way it is textually present above: Knowing that macro
> arguments don't require commas as separators, how many arguments do you see?
> And no, using knowledge on the internal workings (brokenness) of the scrubber
> is not allowed to determine the answer. (My answer: Three. And that's what
> gas also should determine.)
>
> If, purely from a practical / pragmatic perspective we'd really need to keep
> the above working for some more time, then I expect we'll need to invent a
> mode within which we can warn about such broken macros, telling people that
> new behavior will be enforced in, say, the next release. Otherwise how do
> you propose we ever address (without heuristics) the issues that the changes
> at hand are actually aiming at addressing?
>
> I did actually think about possible transitional states. Yet I didn't figure
> any that would be halfway sane _and_ useful. For example, while we could add
> a command line option to request old vs new scrubbing modes. To be useful
> (for the purpose of fully transitioning sooner or later), that ought to
> default to "new", though. Yet then people will need to fix their code anyway,
> just (possibly) by adding the new command line option instead of touching
> assembly sources.
>
> And then: I deliberately waited for comments much longer than I would usually
> have done. No-one really cared to comment on the changing behavior. And hence
> I'm a little irritated that now not just a possible workaround is suggested,
> but outright reverting.
>
> Bottom line: Clearly a NAK for this testsuite addition. We must not test
> accepting input in ways other than (more or less) documented. (Documentation
> isn't great for macros, but the possibility of not using commas to separate
> parameters and arguments can at least be derived from reading what's there.)
> As to reverting the two commits while working out a possible transition path:
> I'm open to that, provided some constructive comments actually surface, so
> we / I have a way forward.
>
> Jan

At very least, we must support the old behavior with a command-line
option which can be used with the testcase.
  
Jan Beulich Aug. 12, 2024, 12:03 p.m. UTC | #3
On 12.08.2024 13:21, H.J. Lu wrote:
> On Mon, Aug 12, 2024 at 12:39 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 12.08.2024 01:11, H.J. Lu wrote:
>>> --- /dev/null
>>> +++ b/gas/testsuite/gas/macros/arg1.s
>>> @@ -0,0 +1,7 @@
>>> +     .macro test arg1
>>> +     .byte \arg1
>>> +     .endm
>>> +
>>> +     .data
>>> +     test  0x10 + 0
>>> +     test  0x10 + 1
>>
>> So this is precisely what that entire change was about that you're proposing
>> to revert. Within binutils, instances of such broken macro invocations were
>> deliberately fixed up front. And the NEWS entry the commit added also is very
>> clear about the above no longer working (and never having been guaranteed to
>> work).
>>
>> Just look at it the way it is textually present above: Knowing that macro
>> arguments don't require commas as separators, how many arguments do you see?
>> And no, using knowledge on the internal workings (brokenness) of the scrubber
>> is not allowed to determine the answer. (My answer: Three. And that's what
>> gas also should determine.)
>>
>> If, purely from a practical / pragmatic perspective we'd really need to keep
>> the above working for some more time, then I expect we'll need to invent a
>> mode within which we can warn about such broken macros, telling people that
>> new behavior will be enforced in, say, the next release. Otherwise how do
>> you propose we ever address (without heuristics) the issues that the changes
>> at hand are actually aiming at addressing?
>>
>> I did actually think about possible transitional states. Yet I didn't figure
>> any that would be halfway sane _and_ useful. For example, while we could add
>> a command line option to request old vs new scrubbing modes. To be useful
>> (for the purpose of fully transitioning sooner or later), that ought to
>> default to "new", though. Yet then people will need to fix their code anyway,
>> just (possibly) by adding the new command line option instead of touching
>> assembly sources.
>>
>> And then: I deliberately waited for comments much longer than I would usually
>> have done. No-one really cared to comment on the changing behavior. And hence
>> I'm a little irritated that now not just a possible workaround is suggested,
>> but outright reverting.
>>
>> Bottom line: Clearly a NAK for this testsuite addition. We must not test
>> accepting input in ways other than (more or less) documented. (Documentation
>> isn't great for macros, but the possibility of not using commas to separate
>> parameters and arguments can at least be derived from reading what's there.)
>> As to reverting the two commits while working out a possible transition path:
>> I'm open to that, provided some constructive comments actually surface, so
>> we / I have a way forward.
> 
> At very least, we must support the old behavior with a command-line
> option which can be used with the testcase.

I disagree with "must". It may be desirable for a transitional phase, yes.
Yet as said - will a command line option (which people will need to add in
their build systems) really help that much? IOW is it expected to be much
more work for them to instead fix their macro use right away (which they
will need to do at some point anyway, as the option would be going away
again after a release or two)?

However, would you (and others) further insist that other bogus behavior
also be retained? (More a rhetorical question perhaps, since if we need
to keep the old scrubber optionally available, it'll likely be easier to
just keep it as is (was), and have such a command line option simply pick
between the two scrubber instances.)

Jan
  
H.J. Lu Aug. 12, 2024, 12:08 p.m. UTC | #4
On Mon, Aug 12, 2024 at 5:03 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 12.08.2024 13:21, H.J. Lu wrote:
> > On Mon, Aug 12, 2024 at 12:39 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 12.08.2024 01:11, H.J. Lu wrote:
> >>> --- /dev/null
> >>> +++ b/gas/testsuite/gas/macros/arg1.s
> >>> @@ -0,0 +1,7 @@
> >>> +     .macro test arg1
> >>> +     .byte \arg1
> >>> +     .endm
> >>> +
> >>> +     .data
> >>> +     test  0x10 + 0
> >>> +     test  0x10 + 1
> >>
> >> So this is precisely what that entire change was about that you're proposing
> >> to revert. Within binutils, instances of such broken macro invocations were
> >> deliberately fixed up front. And the NEWS entry the commit added also is very
> >> clear about the above no longer working (and never having been guaranteed to
> >> work).
> >>
> >> Just look at it the way it is textually present above: Knowing that macro
> >> arguments don't require commas as separators, how many arguments do you see?
> >> And no, using knowledge on the internal workings (brokenness) of the scrubber
> >> is not allowed to determine the answer. (My answer: Three. And that's what
> >> gas also should determine.)
> >>
> >> If, purely from a practical / pragmatic perspective we'd really need to keep
> >> the above working for some more time, then I expect we'll need to invent a
> >> mode within which we can warn about such broken macros, telling people that
> >> new behavior will be enforced in, say, the next release. Otherwise how do
> >> you propose we ever address (without heuristics) the issues that the changes
> >> at hand are actually aiming at addressing?
> >>
> >> I did actually think about possible transitional states. Yet I didn't figure
> >> any that would be halfway sane _and_ useful. For example, while we could add
> >> a command line option to request old vs new scrubbing modes. To be useful
> >> (for the purpose of fully transitioning sooner or later), that ought to
> >> default to "new", though. Yet then people will need to fix their code anyway,
> >> just (possibly) by adding the new command line option instead of touching
> >> assembly sources.
> >>
> >> And then: I deliberately waited for comments much longer than I would usually
> >> have done. No-one really cared to comment on the changing behavior. And hence
> >> I'm a little irritated that now not just a possible workaround is suggested,
> >> but outright reverting.
> >>
> >> Bottom line: Clearly a NAK for this testsuite addition. We must not test
> >> accepting input in ways other than (more or less) documented. (Documentation
> >> isn't great for macros, but the possibility of not using commas to separate
> >> parameters and arguments can at least be derived from reading what's there.)
> >> As to reverting the two commits while working out a possible transition path:
> >> I'm open to that, provided some constructive comments actually surface, so
> >> we / I have a way forward.
> >
> > At very least, we must support the old behavior with a command-line
> > option which can be used with the testcase.
>
> I disagree with "must". It may be desirable for a transitional phase, yes.
> Yet as said - will a command line option (which people will need to add in
> their build systems) really help that much? IOW is it expected to be much
> more work for them to instead fix their macro use right away (which they
> will need to do at some point anyway, as the option would be going away
> again after a release or two)?
>
> However, would you (and others) further insist that other bogus behavior
> also be retained? (More a rhetorical question perhaps, since if we need
> to keep the old scrubber optionally available, it'll likely be easier to
> just keep it as is (was), and have such a command line option simply pick
> between the two scrubber instances.)
>

The old behaviors may not be well documented.  But many important
packages depend on them.  It is unrealistic to change all of them after
GNU assembler has been working on them for so many years.  I think
it is a very bad idea to change the GNU assembler in such a way which
breaks so many packages.  We should add the missing testcases and
document these behaviors instead.
  
Jan Beulich Aug. 12, 2024, 12:36 p.m. UTC | #5
On 12.08.2024 14:08, H.J. Lu wrote:
> On Mon, Aug 12, 2024 at 5:03 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 12.08.2024 13:21, H.J. Lu wrote:
>>> On Mon, Aug 12, 2024 at 12:39 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 12.08.2024 01:11, H.J. Lu wrote:
>>>>> --- /dev/null
>>>>> +++ b/gas/testsuite/gas/macros/arg1.s
>>>>> @@ -0,0 +1,7 @@
>>>>> +     .macro test arg1
>>>>> +     .byte \arg1
>>>>> +     .endm
>>>>> +
>>>>> +     .data
>>>>> +     test  0x10 + 0
>>>>> +     test  0x10 + 1
>>>>
>>>> So this is precisely what that entire change was about that you're proposing
>>>> to revert. Within binutils, instances of such broken macro invocations were
>>>> deliberately fixed up front. And the NEWS entry the commit added also is very
>>>> clear about the above no longer working (and never having been guaranteed to
>>>> work).
>>>>
>>>> Just look at it the way it is textually present above: Knowing that macro
>>>> arguments don't require commas as separators, how many arguments do you see?
>>>> And no, using knowledge on the internal workings (brokenness) of the scrubber
>>>> is not allowed to determine the answer. (My answer: Three. And that's what
>>>> gas also should determine.)
>>>>
>>>> If, purely from a practical / pragmatic perspective we'd really need to keep
>>>> the above working for some more time, then I expect we'll need to invent a
>>>> mode within which we can warn about such broken macros, telling people that
>>>> new behavior will be enforced in, say, the next release. Otherwise how do
>>>> you propose we ever address (without heuristics) the issues that the changes
>>>> at hand are actually aiming at addressing?
>>>>
>>>> I did actually think about possible transitional states. Yet I didn't figure
>>>> any that would be halfway sane _and_ useful. For example, while we could add
>>>> a command line option to request old vs new scrubbing modes. To be useful
>>>> (for the purpose of fully transitioning sooner or later), that ought to
>>>> default to "new", though. Yet then people will need to fix their code anyway,
>>>> just (possibly) by adding the new command line option instead of touching
>>>> assembly sources.
>>>>
>>>> And then: I deliberately waited for comments much longer than I would usually
>>>> have done. No-one really cared to comment on the changing behavior. And hence
>>>> I'm a little irritated that now not just a possible workaround is suggested,
>>>> but outright reverting.
>>>>
>>>> Bottom line: Clearly a NAK for this testsuite addition. We must not test
>>>> accepting input in ways other than (more or less) documented. (Documentation
>>>> isn't great for macros, but the possibility of not using commas to separate
>>>> parameters and arguments can at least be derived from reading what's there.)
>>>> As to reverting the two commits while working out a possible transition path:
>>>> I'm open to that, provided some constructive comments actually surface, so
>>>> we / I have a way forward.
>>>
>>> At very least, we must support the old behavior with a command-line
>>> option which can be used with the testcase.
>>
>> I disagree with "must". It may be desirable for a transitional phase, yes.
>> Yet as said - will a command line option (which people will need to add in
>> their build systems) really help that much? IOW is it expected to be much
>> more work for them to instead fix their macro use right away (which they
>> will need to do at some point anyway, as the option would be going away
>> again after a release or two)?
>>
>> However, would you (and others) further insist that other bogus behavior
>> also be retained? (More a rhetorical question perhaps, since if we need
>> to keep the old scrubber optionally available, it'll likely be easier to
>> just keep it as is (was), and have such a command line option simply pick
>> between the two scrubber instances.)
> 
> The old behaviors may not be well documented.  But many important
> packages depend on them.  It is unrealistic to change all of them after
> GNU assembler has been working on them for so many years.  I think
> it is a very bad idea to change the GNU assembler in such a way which
> breaks so many packages.  We should add the missing testcases and
> document these behaviors instead.

Well, good luck with documenting everything that's going wrong. Just today,
when investigating the fallout Andreas reported, I found yet another one
(which my changes also happen to correct).

Still it may indeed be necessary to retain the original behavior. Whether
indefinitely or for some time is hard to tell. Personally I think it should
be permissible to fix issues there. (If it wasn't, why did absolutely
nobody respond early on, when I first posted the work, saving me from
putting in quite a bit of time and effort?)

Nick, I think this is fundamental enough a question that I'd like to ask
for your input.

Jan
  
H.J. Lu Aug. 12, 2024, 12:41 p.m. UTC | #6
On Mon, Aug 12, 2024 at 5:36 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 12.08.2024 14:08, H.J. Lu wrote:
> > On Mon, Aug 12, 2024 at 5:03 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 12.08.2024 13:21, H.J. Lu wrote:
> >>> On Mon, Aug 12, 2024 at 12:39 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 12.08.2024 01:11, H.J. Lu wrote:
> >>>>> --- /dev/null
> >>>>> +++ b/gas/testsuite/gas/macros/arg1.s
> >>>>> @@ -0,0 +1,7 @@
> >>>>> +     .macro test arg1
> >>>>> +     .byte \arg1
> >>>>> +     .endm
> >>>>> +
> >>>>> +     .data
> >>>>> +     test  0x10 + 0
> >>>>> +     test  0x10 + 1
> >>>>
> >>>> So this is precisely what that entire change was about that you're proposing
> >>>> to revert. Within binutils, instances of such broken macro invocations were
> >>>> deliberately fixed up front. And the NEWS entry the commit added also is very
> >>>> clear about the above no longer working (and never having been guaranteed to
> >>>> work).
> >>>>
> >>>> Just look at it the way it is textually present above: Knowing that macro
> >>>> arguments don't require commas as separators, how many arguments do you see?
> >>>> And no, using knowledge on the internal workings (brokenness) of the scrubber
> >>>> is not allowed to determine the answer. (My answer: Three. And that's what
> >>>> gas also should determine.)
> >>>>
> >>>> If, purely from a practical / pragmatic perspective we'd really need to keep
> >>>> the above working for some more time, then I expect we'll need to invent a
> >>>> mode within which we can warn about such broken macros, telling people that
> >>>> new behavior will be enforced in, say, the next release. Otherwise how do
> >>>> you propose we ever address (without heuristics) the issues that the changes
> >>>> at hand are actually aiming at addressing?
> >>>>
> >>>> I did actually think about possible transitional states. Yet I didn't figure
> >>>> any that would be halfway sane _and_ useful. For example, while we could add
> >>>> a command line option to request old vs new scrubbing modes. To be useful
> >>>> (for the purpose of fully transitioning sooner or later), that ought to
> >>>> default to "new", though. Yet then people will need to fix their code anyway,
> >>>> just (possibly) by adding the new command line option instead of touching
> >>>> assembly sources.
> >>>>
> >>>> And then: I deliberately waited for comments much longer than I would usually
> >>>> have done. No-one really cared to comment on the changing behavior. And hence
> >>>> I'm a little irritated that now not just a possible workaround is suggested,
> >>>> but outright reverting.
> >>>>
> >>>> Bottom line: Clearly a NAK for this testsuite addition. We must not test
> >>>> accepting input in ways other than (more or less) documented. (Documentation
> >>>> isn't great for macros, but the possibility of not using commas to separate
> >>>> parameters and arguments can at least be derived from reading what's there.)
> >>>> As to reverting the two commits while working out a possible transition path:
> >>>> I'm open to that, provided some constructive comments actually surface, so
> >>>> we / I have a way forward.
> >>>
> >>> At very least, we must support the old behavior with a command-line
> >>> option which can be used with the testcase.
> >>
> >> I disagree with "must". It may be desirable for a transitional phase, yes.
> >> Yet as said - will a command line option (which people will need to add in
> >> their build systems) really help that much? IOW is it expected to be much
> >> more work for them to instead fix their macro use right away (which they
> >> will need to do at some point anyway, as the option would be going away
> >> again after a release or two)?
> >>
> >> However, would you (and others) further insist that other bogus behavior
> >> also be retained? (More a rhetorical question perhaps, since if we need
> >> to keep the old scrubber optionally available, it'll likely be easier to
> >> just keep it as is (was), and have such a command line option simply pick
> >> between the two scrubber instances.)
> >
> > The old behaviors may not be well documented.  But many important
> > packages depend on them.  It is unrealistic to change all of them after
> > GNU assembler has been working on them for so many years.  I think
> > it is a very bad idea to change the GNU assembler in such a way which
> > breaks so many packages.  We should add the missing testcases and
> > document these behaviors instead.
>
> Well, good luck with documenting everything that's going wrong. Just today,
> when investigating the fallout Andreas reported, I found yet another one

Is there a binutil bug for this?

> (which my changes also happen to correct).
>
> Still it may indeed be necessary to retain the original behavior. Whether
> indefinitely or for some time is hard to tell. Personally I think it should
> be permissible to fix issues there. (If it wasn't, why did absolutely
> nobody respond early on, when I first posted the work, saving me from

Developers who use the old behavior may not be on the binutils mailing
list.  Even if they are on the list, they may not try your patches.  Personally
I alway test my binitils changes on glibc, gcc and kernel on x86-64.

> putting in quite a bit of time and effort?)
>
> Nick, I think this is fundamental enough a question that I'd like to ask
> for your input.
>
> Jan
  
Sam James Aug. 12, 2024, 12:45 p.m. UTC | #7
Jan Beulich <jbeulich@suse.com> writes:

> On 12.08.2024 13:21, H.J. Lu wrote:
>> On Mon, Aug 12, 2024 at 12:39 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 12.08.2024 01:11, H.J. Lu wrote:
>>>> --- /dev/null
>>>> +++ b/gas/testsuite/gas/macros/arg1.s
>>>> @@ -0,0 +1,7 @@
>>>> +     .macro test arg1
>>>> +     .byte \arg1
>>>> +     .endm
>>>> +
>>>> +     .data
>>>> +     test  0x10 + 0
>>>> +     test  0x10 + 1
>>>
>>> So this is precisely what that entire change was about that you're proposing
>>> to revert. Within binutils, instances of such broken macro invocations were
>>> deliberately fixed up front. And the NEWS entry the commit added also is very
>>> clear about the above no longer working (and never having been guaranteed to
>>> work).
>>>
>>> Just look at it the way it is textually present above: Knowing that macro
>>> arguments don't require commas as separators, how many arguments do you see?
>>> And no, using knowledge on the internal workings (brokenness) of the scrubber
>>> is not allowed to determine the answer. (My answer: Three. And that's what
>>> gas also should determine.)
>>>
>>> If, purely from a practical / pragmatic perspective we'd really need to keep
>>> the above working for some more time, then I expect we'll need to invent a
>>> mode within which we can warn about such broken macros, telling people that
>>> new behavior will be enforced in, say, the next release. Otherwise how do
>>> you propose we ever address (without heuristics) the issues that the changes
>>> at hand are actually aiming at addressing?
>>>
>>> I did actually think about possible transitional states. Yet I didn't figure
>>> any that would be halfway sane _and_ useful. For example, while we could add
>>> a command line option to request old vs new scrubbing modes. To be useful
>>> (for the purpose of fully transitioning sooner or later), that ought to
>>> default to "new", though. Yet then people will need to fix their code anyway,
>>> just (possibly) by adding the new command line option instead of touching
>>> assembly sources.
>>>
>>> And then: I deliberately waited for comments much longer than I would usually
>>> have done. No-one really cared to comment on the changing behavior. And hence
>>> I'm a little irritated that now not just a possible workaround is suggested,
>>> but outright reverting.
>>>
>>> Bottom line: Clearly a NAK for this testsuite addition. We must not test
>>> accepting input in ways other than (more or less) documented. (Documentation
>>> isn't great for macros, but the possibility of not using commas to separate
>>> parameters and arguments can at least be derived from reading what's there.)
>>> As to reverting the two commits while working out a possible transition path:
>>> I'm open to that, provided some constructive comments actually surface, so
>>> we / I have a way forward.
>> 
>> At very least, we must support the old behavior with a command-line
>> option which can be used with the testcase.
>
> I disagree with "must". It may be desirable for a transitional phase, yes.
> Yet as said - will a command line option (which people will need to add in
> their build systems) really help that much? IOW is it expected to be much
> more work for them to instead fix their macro use right away (which they
> will need to do at some point anyway, as the option would be going away
> again after a release or two)?
>
> However, would you (and others) further insist that other bogus behavior
> also be retained? (More a rhetorical question perhaps, since if we need
> to keep the old scrubber optionally available, it'll likely be easier to
> just keep it as is (was), and have such a command line option simply pick
> between the two scrubber instances.)

The only thing I 'insist' on is doing this in an orderly way where we're
clear on what the future behaviour is, so I know what to report to
upstreams & include in commit messages fixing them, including temporary
workarounds.

I have my own views on backwards compatibility but it's not really what
I'm talking about here. I'm mostly interested in:
1) is this intentional (looks like yes, but unclear if that's true for
all cases);
2) is there a temporary workaround to tell people (like a cmdline option);
3) assessing the scale of breakage (it's unclear so far how many of
these issues are the same or not);
4) reporting issues upstream;
5) fixing issues upstream;

It's hard to actually fix anything until we're clear on how much of it
is the new behaviour. I leave discussions on handling historical
codebases to others. But something's wrong if we have GCC failing to
build on non-obscure targets and glibc failing on amd64? It's not a
matter of simply a handful of niche projects getting it wrong, which is
how we ended up with H.J. sending the revert series.

I'd suggest reverting, adding a command line option to opt-in, ask
people to do test-runs en-masse with that, we say clearly where to
report issues at first (maybe on sw bz to be assessed), and go from there.

Upstreams won't be interested in reports right now as it's unclear what
the official position for binutils is going forward.

thanks,
sam
  
Jan Beulich Aug. 12, 2024, 1:02 p.m. UTC | #8
On 12.08.2024 14:45, Sam James wrote:
> Jan Beulich <jbeulich@suse.com> writes:
>> On 12.08.2024 13:21, H.J. Lu wrote:
>>> On Mon, Aug 12, 2024 at 12:39 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 12.08.2024 01:11, H.J. Lu wrote:
>>>>> --- /dev/null
>>>>> +++ b/gas/testsuite/gas/macros/arg1.s
>>>>> @@ -0,0 +1,7 @@
>>>>> +     .macro test arg1
>>>>> +     .byte \arg1
>>>>> +     .endm
>>>>> +
>>>>> +     .data
>>>>> +     test  0x10 + 0
>>>>> +     test  0x10 + 1
>>>>
>>>> So this is precisely what that entire change was about that you're proposing
>>>> to revert. Within binutils, instances of such broken macro invocations were
>>>> deliberately fixed up front. And the NEWS entry the commit added also is very
>>>> clear about the above no longer working (and never having been guaranteed to
>>>> work).
>>>>
>>>> Just look at it the way it is textually present above: Knowing that macro
>>>> arguments don't require commas as separators, how many arguments do you see?
>>>> And no, using knowledge on the internal workings (brokenness) of the scrubber
>>>> is not allowed to determine the answer. (My answer: Three. And that's what
>>>> gas also should determine.)
>>>>
>>>> If, purely from a practical / pragmatic perspective we'd really need to keep
>>>> the above working for some more time, then I expect we'll need to invent a
>>>> mode within which we can warn about such broken macros, telling people that
>>>> new behavior will be enforced in, say, the next release. Otherwise how do
>>>> you propose we ever address (without heuristics) the issues that the changes
>>>> at hand are actually aiming at addressing?
>>>>
>>>> I did actually think about possible transitional states. Yet I didn't figure
>>>> any that would be halfway sane _and_ useful. For example, while we could add
>>>> a command line option to request old vs new scrubbing modes. To be useful
>>>> (for the purpose of fully transitioning sooner or later), that ought to
>>>> default to "new", though. Yet then people will need to fix their code anyway,
>>>> just (possibly) by adding the new command line option instead of touching
>>>> assembly sources.
>>>>
>>>> And then: I deliberately waited for comments much longer than I would usually
>>>> have done. No-one really cared to comment on the changing behavior. And hence
>>>> I'm a little irritated that now not just a possible workaround is suggested,
>>>> but outright reverting.
>>>>
>>>> Bottom line: Clearly a NAK for this testsuite addition. We must not test
>>>> accepting input in ways other than (more or less) documented. (Documentation
>>>> isn't great for macros, but the possibility of not using commas to separate
>>>> parameters and arguments can at least be derived from reading what's there.)
>>>> As to reverting the two commits while working out a possible transition path:
>>>> I'm open to that, provided some constructive comments actually surface, so
>>>> we / I have a way forward.
>>>
>>> At very least, we must support the old behavior with a command-line
>>> option which can be used with the testcase.
>>
>> I disagree with "must". It may be desirable for a transitional phase, yes.
>> Yet as said - will a command line option (which people will need to add in
>> their build systems) really help that much? IOW is it expected to be much
>> more work for them to instead fix their macro use right away (which they
>> will need to do at some point anyway, as the option would be going away
>> again after a release or two)?
>>
>> However, would you (and others) further insist that other bogus behavior
>> also be retained? (More a rhetorical question perhaps, since if we need
>> to keep the old scrubber optionally available, it'll likely be easier to
>> just keep it as is (was), and have such a command line option simply pick
>> between the two scrubber instances.)
> 
> The only thing I 'insist' on is doing this in an orderly way where we're
> clear on what the future behaviour is, so I know what to report to
> upstreams & include in commit messages fixing them, including temporary
> workarounds.
> 
> I have my own views on backwards compatibility but it's not really what
> I'm talking about here. I'm mostly interested in:
> 1) is this intentional (looks like yes, but unclear if that's true for
> all cases);

It's all intentional, but there are going to be bugs. Andreas likely
reported a case which needs correcting.

> 2) is there a temporary workaround to tell people (like a cmdline option);

There's no command line option. But people can of course use well-
formed code. Which would likely cover all of the macro use problems
that were reported so far.

> 3) assessing the scale of breakage (it's unclear so far how many of
> these issues are the same or not);
> 4) reporting issues upstream;
> 5) fixing issues upstream;
> 
> It's hard to actually fix anything until we're clear on how much of it
> is the new behaviour. I leave discussions on handling historical
> codebases to others. But something's wrong if we have GCC failing to
> build on non-obscure targets and glibc failing on amd64? It's not a
> matter of simply a handful of niche projects getting it wrong, which is
> how we ended up with H.J. sending the revert series.

Well, I will certainly admit that I didn't expect the macro issue to
be this widespread. Yet at the same time trying to be yet more careful
won't work either - I can't really fix all affected targets in all of
Linux, glibc, gcc, and who knows what not. It was enough work already
to get binutils alone sorted. And I also can't reasonably wait, or
I'll be waiting for years.

> I'd suggest reverting, adding a command line option to opt-in, ask
> people to do test-runs en-masse with that, we say clearly where to
> report issues at first (maybe on sw bz to be assessed), and go from there.

And what would make people even try the new mode, which likely hardly
anyone would be aware of if it wasn't (right now) default behavior?
"Ask people" is what simply isn't going to work, from my experience.
Just as much as - I said this before - expecting feedback to the
patch submission didn't really work out. And I'm not talking of patch
review, feedback on the intended change in behavior would already
have helped (provided it would have been constructive and not just
"no, you can't do that").

> Upstreams won't be interested in reports right now as it's unclear what
> the official position for binutils is going forward.

I'm afraid I don't really understand this part. Who's "upstreams" here?

Jan
  
Sam James Aug. 12, 2024, 1:19 p.m. UTC | #9
Jan Beulich <jbeulich@suse.com> writes:

> On 12.08.2024 14:45, Sam James wrote:
> [...]
>> I have my own views on backwards compatibility but it's not really what
>> I'm talking about here. I'm mostly interested in:
>> 1) is this intentional (looks like yes, but unclear if that's true for
>> all cases);
>
> It's all intentional, but there are going to be bugs. Andreas likely
> reported a case which needs correcting.
>

OK, thanks.

>> 2) is there a temporary workaround to tell people (like a cmdline option);
>
> There's no command line option. But people can of course use well-
> formed code. Which would likely cover all of the macro use problems
> that were reported so far.
>

Right.

>> 3) assessing the scale of breakage (it's unclear so far how many of
>> these issues are the same or not);
>> 4) reporting issues upstream;
>> 5) fixing issues upstream;
>> 
>> It's hard to actually fix anything until we're clear on how much of it
>> is the new behaviour. I leave discussions on handling historical
>> codebases to others. But something's wrong if we have GCC failing to
>> build on non-obscure targets and glibc failing on amd64? It's not a
>> matter of simply a handful of niche projects getting it wrong, which is
>> how we ended up with H.J. sending the revert series.
>
> Well, I will certainly admit that I didn't expect the macro issue to
> be this widespread. Yet at the same time trying to be yet more careful
> won't work either - I can't really fix all affected targets in all of
> Linux, glibc, gcc, and who knows what not. It was enough work already
> to get binutils alone sorted. And I also can't reasonably wait, or
> I'll be waiting for years.
>

Yes, of course not asking you to do that. But having amd64 glibc/linux
broken is the other end of the extreme.

>> I'd suggest reverting, adding a command line option to opt-in, ask
>> people to do test-runs en-masse with that, we say clearly where to
>> report issues at first (maybe on sw bz to be assessed), and go from there.
>
> And what would make people even try the new mode, which likely hardly
> anyone would be aware of if it wasn't (right now) default behavior?
> "Ask people" is what simply isn't going to work, from my experience.
> Just as much as - I said this before - expecting feedback to the
> patch submission didn't really work out. And I'm not talking of patch
> review, feedback on the intended change in behavior would already
> have helped (provided it would have been constructive and not just
> "no, you can't do that").

I at least would do such testing en-masse when asked. I already try to
do it when I see a time it'd be useful.

I can't speak for others. I do try to watch the ML and test changes when
they look obviously "observable" (or after they land). I think the
Fedora people have some tooling to do mass rebuilds with experimental
toolchain changes as well.

I didn't read the deep gas thread about the lexer cleanup.

>
>> Upstreams won't be interested in reports right now as it's unclear what
>> the official position for binutils is going forward.
>
> I'm afraid I don't really understand this part. Who's "upstreams" here?
>

Sorry, I guess it's confusing here in the context of binutils. I mean
maintainers of software that's broken.

It's not clear to me if I should actually be reporting anything anywhere
yet, given https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116339#c11.

(Is the bug valid? If so, let's reopen it and let the libgcc/arm
maintainers sort it out. If it's not valid, what are we doing here,
because it's inconsistent with my understanding of this discussion?)

That's part of the problem. You're saying the changes are intentional,
but also that you don't know what quirks we are/aren't going to
implement, so we're left in limbo with master as-is? I guess we're
waiting for Nick's input. That is the argument in favour of reverting
it, because the answer right now isn't even clearly "go fix gcc"?

thanks,
sam
  
Sam James Aug. 12, 2024, 1:24 p.m. UTC | #10
Jan Beulich <jbeulich@suse.com> writes:

> On 12.08.2024 14:45, Sam James wrote:
> [...]

Just to say explicitly: I'm grateful you're working on a particularly
gnarly bit of gas, breakage does happen, and I'm happy to do whatever
testing on our package set if it helps (offer is recurring).

I just need to explain the situation accurately to those with now-broken
software, and I'd also like to avoid confused bug reports showing up in
e.g. gcc, kernel MLs while we get our house in order.
  
Jan Beulich Aug. 12, 2024, 1:45 p.m. UTC | #11
On 12.08.2024 15:19, Sam James wrote:
> Jan Beulich <jbeulich@suse.com> writes:
>> On 12.08.2024 14:45, Sam James wrote:
>>> Upstreams won't be interested in reports right now as it's unclear what
>>> the official position for binutils is going forward.
>>
>> I'm afraid I don't really understand this part. Who's "upstreams" here?
>>
> 
> Sorry, I guess it's confusing here in the context of binutils. I mean
> maintainers of software that's broken.
> 
> It's not clear to me if I should actually be reporting anything anywhere
> yet, given https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116339#c11.
> 
> (Is the bug valid? If so, let's reopen it and let the libgcc/arm
> maintainers sort it out. If it's not valid, what are we doing here,
> because it's inconsistent with my understanding of this discussion?)
> 
> That's part of the problem. You're saying the changes are intentional,
> but also that you don't know what quirks we are/aren't going to
> implement, so we're left in limbo with master as-is? I guess we're
> waiting for Nick's input. That is the argument in favour of reverting
> it, because the answer right now isn't even clearly "go fix gcc"?

Well. I would have done the revert already, if it wasn't overwhelmingly
likely that then the discussion here dies out almost immediately.

Jan
  
Andreas K. Huettel Aug. 13, 2024, 7:27 a.m. UTC | #12
Am Montag, 12. August 2024, 15:02:36 CEST schrieb Jan Beulich:
> 
> > Upstreams won't be interested in reports right now as it's unclear what
> > the official position for binutils is going forward.
> 
> I'm afraid I don't really understand this part. Who's "upstreams" here?
> 

All great testing by Sam aside, this is what usually happens:

* Gentoo packages new binutils as one of the first places, users install and use it

* Software starts to fail to build (remember we're a source distro)

* Gentoo package maintainers file bugs to upstream code authors (i.e. the packages
  containing the now broken code)

* Upstream maintainers:
  - Huh. This always worked. Show me the specs.
  - Go away Gentoo weirdo.
  - Can't you just build it with an older binutils?
  - ENOTFEDORA
  - We only support flatpak.
  - ...

* Now we need to convince them why the current code is broken, and show
  them as simply as possible how to fix it.

Wanna help?
  
Andreas K. Huettel Aug. 13, 2024, 11:05 a.m. UTC | #13
> 
> * Now we need to convince them why the current code is broken, and show
>   them as simply as possible how to fix it.
> 

PS. to provide a scale for how big such an effort can get, here's the (of course
technically unrelated) gentoo tracker for clang-16 / gcc-14 modern c porting:

https://bugs.gentoo.org/870412

As of this moment it depends on 700 open bugs (for Gentoo alone), and 1376 closed
ones.
  
Fangrui Song Aug. 14, 2024, 7:37 a.m. UTC | #14
On Mon, Aug 12, 2024 at 6:45 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 12.08.2024 15:19, Sam James wrote:
> > Jan Beulich <jbeulich@suse.com> writes:
> >> On 12.08.2024 14:45, Sam James wrote:
> >>> Upstreams won't be interested in reports right now as it's unclear what
> >>> the official position for binutils is going forward.
> >>
> >> I'm afraid I don't really understand this part. Who's "upstreams" here?
> >>
> >
> > Sorry, I guess it's confusing here in the context of binutils. I mean
> > maintainers of software that's broken.
> >
> > It's not clear to me if I should actually be reporting anything anywhere
> > yet, given https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116339#c11.
> >
> > (Is the bug valid? If so, let's reopen it and let the libgcc/arm
> > maintainers sort it out. If it's not valid, what are we doing here,
> > because it's inconsistent with my understanding of this discussion?)
> >
> > That's part of the problem. You're saying the changes are intentional,
> > but also that you don't know what quirks we are/aren't going to
> > implement, so we're left in limbo with master as-is? I guess we're
> > waiting for Nick's input. That is the argument in favour of reverting
> > it, because the answer right now isn't even clearly "go fix gcc"?
>
> Well. I would have done the revert already, if it wasn't overwhelmingly
> likely that then the discussion here dies out almost immediately.
>
> Jan


> Sam: Just to say explicitly: I'm grateful you're working on a particularly gnarly bit of gas, breakage does happen, and I'm happy to do whatever testing on our package set if it helps (offer is recurring).

+1 to both!
While the whitespace scrub disruption (to gcc/glibc/Linux kernel) was
unfortunate, I believe there's still room for assembler improvement.

(I did not comment on the scrubber patch series as I have very little
understanding of that code....)

Some major assembly users (Linux kernel, glibc, edk2, ffmpeg, libx265,
etc) likely encompass code patterns found in other packages.
I personally believe if assembler changes make these major users
happy, almost all packages will be happy as well.
(I fixed quite a lot of corner cases in LLVM integrated assembler and
these packages got my attention. Most packages just aren't
nitpicking.)

---

After I posted a lengthy reply at
https://sourceware.org/bugzilla/show_bug.cgi?id=32073#c23
about certain assembly code patterns, I did more analysis of Linux
kernel's x86_64/arm/arm64/powerpc/riscv builds.

If I drop the `isOperator` code of LLVM integrated assembler (don't
parse "a + b" in "M a + b" as a single argument),
I would have breakage with arm/arm64/powerpc/riscv (`defconfig`).

The kernel's intricate macro usage can be explored in the log files:
https://gist.github.com/MaskRay/cf565413205bde2dd2cc634714bad994

---

My current feeling is that we could retain a special case for "SPACE
BIN_OP SPACE" and not treat it as a separate argument.

  m 5 + 6  # 5+6 is a whole as + is a binary operator

If this still feels too hacky, we could only enable this when a comma
separator appears anywhere on this line.

Note, linux/arch/arm64/lib/crc32.S has a SPACE BIN_OP SPACE use case
before a comma is seen.

   nops (662b-661b) / 4



We could report errors for space-separated arguments in more contexts.
To minimize disruption, we can restrict this change to altmacro mode
initially, issuing warnings in noaltmacro mode.

m 5 6    # currently ok; needs analysis about potential disruption.
m (5) 6  # error
m 5 (6)  # currently ok
  

Patch

diff --git a/gas/testsuite/gas/macros/arg1.d b/gas/testsuite/gas/macros/arg1.d
new file mode 100644
index 00000000000..84e9cf7e6c8
--- /dev/null
+++ b/gas/testsuite/gas/macros/arg1.d
@@ -0,0 +1,8 @@ 
+#objdump: -s -j .data
+#name expression argument
+
+.*: .*
+
+Contents of section .data:
+ 0000 1011                                 ..              
+#pass
diff --git a/gas/testsuite/gas/macros/arg1.s b/gas/testsuite/gas/macros/arg1.s
new file mode 100644
index 00000000000..8905f99e73b
--- /dev/null
+++ b/gas/testsuite/gas/macros/arg1.s
@@ -0,0 +1,7 @@ 
+	.macro test arg1
+	.byte \arg1
+	.endm
+
+	.data
+ 	test  0x10 + 0
+ 	test  0x10 + 1
diff --git a/gas/testsuite/gas/macros/macros.exp b/gas/testsuite/gas/macros/macros.exp
index bb5d4abf25b..3e84902c65f 100644
--- a/gas/testsuite/gas/macros/macros.exp
+++ b/gas/testsuite/gas/macros/macros.exp
@@ -75,6 +75,8 @@  if { ![istarget tic30-*-*] } {
     run_list_test app6 ""
 }
 
+run_dump_test arg1
+
 run_list_test badarg ""
 
 switch -glob $target_triplet {