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

[Sam James]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 377 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20240812/ea10d26c/attachment.sig>