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

[Jan Beulich]

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