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

[Jan Beulich]

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