commit 3fa12f5bd65d8383db0bc78317c1e13cc53a8f50 Author: GDB Administrator <gdbadmin@sourceware.org> Date: Sun Aug 11 00:00:36 2024 +0000 Automatic date update in version.in failed to build Linux kernel 6.10.4: rch/x86/crypto/sha256-avx2-asm.S: Assembler messages: arch/x86/crypto/sha256-avx2-asm.S:595: Error: too many positional arguments arch/x86/crypto/sha256-avx2-asm.S:600: Error: too many positional arguments arch/x86/crypto/sha256-avx2-asm.S:605: Error: too many positional arguments arch/x86/crypto/sha256-avx2-asm.S:610: Error: too many positional arguments arch/x86/crypto/sha256-avx2-asm.S:621: Error: too many positional arguments arch/x86/crypto/sha256-avx2-asm.S:626: Error: too many positional arguments arch/x86/crypto/sha256-avx2-asm.S:654: Error: too many positional arguments arch/x86/crypto/sha256-avx2-asm.S:655: Error: too many positional arguments make[5]: *** [scripts/Makefile.build:362: arch/x86/crypto/sha256-avx2-asm.o] Error 1
A simple testcase: .macro test arg1 .long \arg1 .endm test 0x101 + 1
While this use of macros may be in widespread use, that doesn't make it correct (and the gas/NEWS entry actually mentions a macro invocation of this same form). Macro arguments (and parameters) don't require commas as separators. Hence without relying on the internal workings of the gas scrubber, one can't assume that the provided example passes a single argument; instead (following admittedly not great documentation) it's rather 3 arguments which are being passed. Just that gas didn't handle this correctly in the past. Let me give a slightly more complex example: m 101 + (1) (15) (a) How many arguments? The change in question had been pending comments for a long time. Sadly, as is happening more often than not, comments actually surface only once a change was committed and suddenly issues with (broken) existing code show up. Parenthesizing or quoting the argument(s) helps. In case of macros with a single argument, using :vararg in the macro definition is also a way to approach things. At least transitionally we could also invent a mode where macro invocations with some comma separation assumes all arguments are comma separated, thus continuing to ignore whitespace elsewhere. That'll help everywhere, though. For macros with a single argument we could further effectively guess that :vararg is in effect, yet of course that's likely to break some test in the gas testsuite (at least I hope we have such a test there). Which of course in turn could be worked around by adding a new "using new gas behavior" command line option.
(In reply to Jan Beulich from comment #2) > While this use of macros may be in widespread use, that doesn't make it > correct > (and the gas/NEWS entry actually mentions a macro invocation of this same > form). Macro arguments (and parameters) don't require commas as separators. > Hence without relying on the internal workings of the gas scrubber, one > can't assume that the provided example passes a single argument; instead > (following admittedly not great documentation) it's rather 3 arguments which > are being passed. Just that gas didn't handle this correctly in the past. > Let me give a slightly more complex example: > > m 101 + (1) (15) (a) > > How many arguments? > > The change in question had been pending comments for a long time. Sadly, as > is happening more often than not, comments actually surface only once a > change was committed and suddenly issues with (broken) existing code show up. > I get that, and I've been the victim of it many a time in projects. But I would also generally expect some heavy consumers of gas to be checked as well. glibc doesn't build for me, nor does the kernel. Did you try build any projects using patched gas? (It's very possible you did, it's not an accusation; the kernel obviously has a million config options, and glibc has its own idiosyncrasies as to whether assembly is used.) It is what it is though. I'll gather a list of broken cases and report them (possibly here first if I'm unsure on if it's wrong). The only reason I care particularly here is because both glibc and the kernel don't build for me. If it was just a few userland applications like ffmpeg, so be it, we've dealt with that before.
Testcase from glibc: $ cat svml_d_acos2_core-sse2.S #define JUMPTARGET(name) *name##@GOTPCREL(%rip) .macro WRAPPER_IMPL_SSE2 callee call JUMPTARGET(\callee) .endm .text WRAPPER_IMPL_SSE2 acos $ gcc -c svml_d_acos2_core-sse2.S svml_d_acos2_core-sse2.S: Assembler messages: svml_d_acos2_core-sse2.S:4: Error: invalid character '\' in operand 1 svml_d_acos2_core-sse2.S:7: Info: macro invoked from here
(In reply to Andreas Schwab from comment #4) > $ cat svml_d_acos2_core-sse2.S > #define JUMPTARGET(name) *name##@GOTPCREL(%rip) > > .macro WRAPPER_IMPL_SSE2 callee > call JUMPTARGET(\callee) > .endm > .text > WRAPPER_IMPL_SSE2 acos Which cpp expands to .macro WRAPPER_IMPL_SSE2 callee call *\ callee@GOTPCREL(%rip) .endm .text WRAPPER_IMPL_SSE2 acos Wow. Where's the blank coming from? I didn't know the pre-processor may insert random whitespace. And the pre-processed result is identical to that of the (imo) bogus call JUMPTARGET(\ callee) How's the consumer supposed to be telling apart which one it was originally? Yet then documentation is unclear on whether there may be whitespace between the \ and the parameter name. We could of course make macro expansion skip whitespace when a valid parameter name follows. Yet I fear there could be other anomalies as a result.
It also breaks GCC builds for ARM, AVR, PRU and others.
(In reply to Jan Beulich from comment #2) > While this use of macros may be in widespread use, that doesn't make it > correct "correct" is a strong word when a particular behaviour of GAS involved. It traditionally tried to follow the targets native assembler behaviour with whatever quirks. In time it developed its own quirks and people came to rely on them. Nothing in that area is "correct" or "incorrect", it just is. So, changing the assembler to have fewer quirks or have more reliable or orthogonal behaviour needs to take the real world into account, because that's where its raison d'etre is. If that means adding hacks on top of the cleaner implementation (or moving them from somewhere), then that is what needs doing. And if it means not even having a cleaner implementation to start with, well, then that's the right solution :-/ (In this case, as you say, adding quirks is probably the right thing: single-arg macros consume everything until line-end (or whatever is was before), and macro-arg prefix char consumes whitespace until identifier)
(In reply to Jan Beulich from comment #5) > Yet then documentation is unclear on whether there may be whitespace between > the \ and the parameter name. We could of course make macro expansion skip > whitespace when a valid parameter name follows. Yet I fear there could be > other anomalies as a result. I've looked into what the options are of fixing this particular issue. Dealing with the one question of "should blanks be skipped here" quickly turns into a series of such questions, perhaps one for every individual transformation that is done while expanding a macro: - Is \( a token, or can there be whitespace? - Are \@ and \+ tokens, or can there be whitespace? - There's also \& with a comment alluding to preprocessor variables. - Is & used for macro parameter references permitted to be followed by whitespace? If so, what about the optional trailing & ? - Same for @. My intuitive answers to these wouldn't all be the same. For example I'd be more inclined to not permit whitespace in @name@ references. Yet I don't even know the origin of that kind of construct, so how should I be able to tell? Plus of course there's then also the question of whether whitespace should survive when processing something that looks like a param ref, but really isn't (as can easily be the case in nested macros).
(In reply to Jan Beulich from comment #8) > > I've looked into what the options are of fixing this particular issue. > Dealing with the one question of "should blanks be skipped here" quickly > turns into a series of such questions, perhaps one for every individual > transformation that is done while expanding a macro: > - Is \( a token, or can there be whitespace? > - Are \@ and \+ tokens, or can there be whitespace? > - There's also \& with a comment alluding to preprocessor variables. > - Is & used for macro parameter references permitted to be followed by > whitespace? If so, what about the optional trailing & ? > - Same for @. > My intuitive answers to these wouldn't all be the same. For example I'd be > more inclined to not permit whitespace in @name@ references. Yet I don't > even know the origin of that kind of construct, so how should I be able to > tell? I fear the only non-contentious answer to all such questions is: "act in the same way as currently" :-/ I.e. try it out and emulate the behaviour.
(In reply to Michael Matz from comment #9) > I fear the only non-contentious answer to all such questions is: "act in the > same > way as currently" :-/ I.e. try it out and emulate the behaviour. In which case it would also be impossible to fix anomalies there. In turn meaning that hardly any bug in parsing of input can actually be fixed. Not a good state to be in. How would you respond to a bug report in such an area then? "We know it's broken, but it can't be fixed"?
(In reply to Jan Beulich from comment #10) > (In reply to Michael Matz from comment #9) > > I fear the only non-contentious answer to all such questions is: "act in the > > same > > way as currently" :-/ I.e. try it out and emulate the behaviour. > > In which case it would also be impossible to fix anomalies there. In turn > meaning that hardly any bug in parsing of input can actually be fixed. Not a > good state to be in. How would you respond to a bug report in such an area > then? "We know it's broken, but it can't be fixed"? We should treat the longstanding usages as features.
(In reply to Jan Beulich from comment #10) > In which case it would also be impossible to fix anomalies there. In turn > meaning that hardly any bug in parsing of input can actually be fixed. Not a > good state to be in. How would you respond to a bug report in such an area > then? "We know it's broken, but it can't be fixed"? Where does the notion of using whitespace for argument separation in macro invocations (as opposed to definitions) come from? I can see it was you actually who documented this feature in the GAS manual back in 2022 only. But if GAS does accept whitespace here, then isn't it just a GAS regression that needs fixing instead? Is there any substantial piece of software out there known to rely on this previously undocumented feature (and why)? How does the use of whitespace for argument separation mix with default values anyway?
(In reply to Maciej W. Rozycki from comment #12) > Where does the notion of using whitespace for argument separation in > macro invocations (as opposed to definitions) come from? I have no idea, and I never really liked it. But I've seen it in actual use, not just once. My vague guess is that comma separation actually was introduced only later, to allow macro uses to look more like insns. > I can see it was you actually who documented this feature in the GAS > manual back in 2022 only. But if GAS does accept whitespace here, then > isn't it just a GAS regression that needs fixing instead? Regression? Afaict this (bad) mode of argument (and parameter) separation has been there forever. It just may not have been properly spelled out in the doc. > Is there any > substantial piece of software out there known to rely on this previously > undocumented feature (and why)? I know we've switched to (consistently) comma separation in the Xen hypervisor only a year or so ago. The origin of Xen is Linux; I didn't extensively check recent Linux, but at least on x86 there are such macro uses. And I can certainly easily spot macro definitions across various architectures where parameters are separated by just blanks. > How does the use of whitespace for > argument separation mix with default values anyway? Not very well, as you apparently inferred. Yet iirc defaults for macro arguments hadn't been there all the time.
(In reply to Maciej W. Rozycki from comment #12) > (In reply to Jan Beulich from comment #10) > > In which case it would also be impossible to fix anomalies there. In turn > > meaning that hardly any bug in parsing of input can actually be fixed. Not a > > good state to be in. How would you respond to a bug report in such an area > > then? "We know it's broken, but it can't be fixed"? > Where does the notion of using whitespace for argument separation in > macro invocations (as opposed to definitions) come from? It was already in the 1999 sourceware import, so, let's say "forever". The scrubber removes whitespace between tokens of different classes, but retains whitespace between token of same class, which sometimes makes it so that whitespace in macro invocation does or does not separate arguments, the difference visible in e.g.: invoke 1 2 invoke 1 + 2 3 (support invoke is a two-arg macro, both of the above invocations would be correct and pass two args). That's where the confusion comes from, but whitespace always was a argument separator in macro invocations (for better or worse), and is used in the wild :-/
(In reply to Michael Matz from comment #14) > invoke 1 2 > invoke 1 + 2 3 > > (support invoke is a two-arg macro, fat fingering on my part doesn't help :-/ 'suppose invoke is a ...' was what I meant.
The master branch has been updated by H.J. Lu <hjl@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=65d41fb015af0048347a25d0665bb1263eb982b0 commit 65d41fb015af0048347a25d0665bb1263eb982b0 Author: H.J. Lu <hjl.tools@gmail.com> Date: Mon Aug 12 08:43:20 2024 -0700 Revert "gas: drop scrubber states 14 and 15" This reverts commit 7dd0dfbde7ee31167a3b2e192a575493d26b7b0a. This is a prerequisite for the PR gas/32073 fix.
The master branch has been updated by H.J. Lu <hjl@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=87582defe75340e0003d4700568322379e9bc9f6 commit 87582defe75340e0003d4700568322379e9bc9f6 Author: H.J. Lu <hjl.tools@gmail.com> Date: Mon Aug 12 08:43:21 2024 -0700 Revert "gas: have scrubber retain more whitespace" This reverts commit 6ae8a30d44f016cafb46a75843b5109316eb1996. This fixes PR gas/32073.
The master branch has been updated by H.J. Lu <hjl@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=a86cec1a6f5127fc66737f95c2ba7938a755b63f commit a86cec1a6f5127fc66737f95c2ba7938a755b63f Author: H.J. Lu <hjl.tools@gmail.com> Date: Mon Aug 12 08:43:22 2024 -0700 gas: Add macro tests for PR gas/32073 1. Add a macro test for expression argument with inner white spaces and a white space before argument added by C preprocessor. 2. Add a x86-64 specific macro test. PR gas/32073 * testsuite/gas/i386/x86-64-macro-1.d: New file. * testsuite/gas/i386/x86-64-macro-1.s: Likewise. * testsuite/gas/i386/x86-64.exp: Run x86-64-macro-1. * 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>
(In reply to Michael Matz from comment #14) > The scrubber removes whitespace between tokens of different classes, but > retains > whitespace between token of same class, which sometimes makes it so that > whitespace in macro invocation does or does not separate arguments, the > difference visible in e.g.: > > invoke 1 2 > invoke 1 + 2 3 But that's the whole issue: This isn't how the scrubber works. It's not uniformly "different classes" (there's not really a proper concept of tokens either). Boundaries between what may be numbers or identifiers are different from boundaries between operators and alike. The original report that I started this effort from was (iirc) something like invoke (1) (2) where the (pretty obviously) two arguments become just one.
(In reply to Jan Beulich from comment #19) > (In reply to Michael Matz from comment #14) > > The scrubber removes whitespace between tokens of different classes, but > > retains > > whitespace between token of same class, which sometimes makes it so that > > whitespace in macro invocation does or does not separate arguments, the > > difference visible in e.g.: > > > > invoke 1 2 > > invoke 1 + 2 3 > > But that's the whole issue: This isn't how the scrubber works. I was talking "in principle" and hence imprecisely. > It's not uniformly "different classes" (there's not really a proper concept > of tokens either). Tokens _are_ classes of character-sequences. 'identifier', 'number', 'punctuator', and so on. (Sometimes the class has just one member: e.g. ')'). But you're right that it's not the difference of classes between RHS and LHS that enables white-space removal, but rather if pasting the two strings results in one new token or not. > Boundaries between what may be numbers or identifiers are > different from boundaries between operators and alike. The original > report that I started this effort from was (iirc) something like > > invoke (1) (2) > > where the (pretty obviously) two arguments become just one. Yes, when the removed whitespace doesn't change the parse (i.e. the character sequences now side-by-side don't form a new token), then it can be removed. Except of course, it cannot, because whitespace has semantic meaning as separator later on :-/ I don't have a good solution, I see only hacks (e.g. above: fix _this_ very instance of '(1) (2)' being pasted, to not be anymore, or alternatively to continue pasting them, but handle outermost ')' as additional macro argument separator). :-( Or: accept the fact that '(1) (2)' is a single macro argument, even if surprising.
(In reply to Michael Matz from comment #20) > Or: accept the fact that '(1) (2)' is a single macro argument, even if > surprising. I submitted a test for it: https://sourceware.org/pipermail/binutils/2024-August/136307.html
(In reply to Michael Matz from comment #20) > I don't have a good solution, I see only hacks (e.g. above: fix _this_ very > instance of '(1) (2)' being pasted, to not be anymore, or alternatively to > continue pasting them, but handle outermost ')' as additional macro argument > separator). :-( > > Or: accept the fact that '(1) (2)' is a single macro argument, even if > surprising. Fair enough. Would it help and work if we at least only permitted one kind of separator per invocation, i.e. if there's an unquoted comma in the argument string then it becomes the separator and whitespace is not?
(In reply to H.J. Lu from comment #0) > [...] > failed to build Linux kernel 6.10.4: > [...] > make[5]: *** [scripts/Makefile.build:362: arch/x86/crypto/sha256-avx2-asm.o] > Error 1 I have created a Linux kernel patch to fix arch/x86/crypto/sha256-avx2-asm.S https://lore.kernel.org/all/20240814044802.1743286-1-maskray@google.com/T/#u M a + b => M (a + b) Further investigation reveals that arch/arm64/kvm/hyp/nvhe/../hyp-entry has the following `(1 << 15) | 1` pattern, which was broken by the scrubber changes. (make -skj"$(nproc)" O=/tmp/linux/arm64 ARCH=arm64 LLVM=1 defconfig all) altinstruction_entry 661f, kvm_patch_vector_branch, (1 << 15) | 1, 662f-661f, 0 I have checked how the LLVM integrated assembler (which doesn't have a whitespace scrubber) handles x86 `a + b` and arm64 `(1 << 15) | 1`. After parsing `a`, it checks whether the next token is an operator, and if yes, parse the next token and merge them into `a`. https://github.com/llvm/llvm-project/commit/055006475e22014b28a070db1bff41ca15f322f0#diff-e6bafec46d3db78a4169cfe729e3852b72cf7eebe7d5cbab12b1d7e3cb00b6e8R1578 (In reply to Jan Beulich from comment #5) > (In reply to Andreas Schwab from comment #4) > > $ cat svml_d_acos2_core-sse2.S > > #define JUMPTARGET(name) *name##@GOTPCREL(%rip) > > > > .macro WRAPPER_IMPL_SSE2 callee > > call JUMPTARGET(\callee) > > .endm > > .text > > WRAPPER_IMPL_SSE2 acos > > Which cpp expands to > > .macro WRAPPER_IMPL_SSE2 callee > call *\ callee@GOTPCREL(%rip) > .endm > .text > WRAPPER_IMPL_SSE2 acos > > Wow. Where's the blank coming from? I didn't know the pre-processor may > insert random whitespace. And the pre-processed result is identical to that > of the (imo) bogus > > call JUMPTARGET(\ callee) > > How's the consumer supposed to be telling apart which one it was originally? > > Yet then documentation is unclear on whether there may be whitespace between > the \ and the parameter name. We could of course make macro expansion skip > whitespace when a valid parameter name follows. Yet I fear there could be > other anomalies as a result. In the clang -E expansion, there is no such space... .macro WRAPPER_IMPL_SSE2 callee call *\callee@GOTPCREL(%rip) .endm .text WRAPPER_IMPL_SSE2 acos But I am amused by this behavior (\ callee). .macro foo a // clang reports an error while gas currently accepts this call \ a .endm foo ab (In reply to Maciej W. Rozycki from comment #22) > (In reply to Michael Matz from comment #20) > > I don't have a good solution, I see only hacks (e.g. above: fix _this_ very > > instance of '(1) (2)' being pasted, to not be anymore, or alternatively to > > continue pasting them, but handle outermost ')' as additional macro argument > > separator). :-( > > > > Or: accept the fact that '(1) (2)' is a single macro argument, even if > > surprising. > Fair enough. Would it help and work if we at least only permitted one > kind of separator per invocation, i.e. if there's an unquoted comma in > the argument string then it becomes the separator and whitespace is not? I think this will be useful. We could explore additional opportunities to restrict space separators to as few scenarios as possible (as long as not used in the wild)...
(In reply to H.J. Lu from comment #6) > It also breaks GCC builds for ARM, AVR, PRU and others. Do you have links to the potentially brittle assembly? In the gcc repo, I checked a few `rg '\.macro' -g '*.s' -g '*.S'` occurrences, but I did not find anything.
https://inbox.sourceware.org/binutils/87a5hjmq7r.fsf@linux-m68k.org/
(In reply to Andreas Schwab from comment #25) > https://inbox.sourceware.org/binutils/87a5hjmq7r.fsf@linux-m68k.org/ Thanks. https://inbox.sourceware.org/binutils/73460160-a0bf-48d9-84b0-c531317d0ce3@suse.com/ has some discussion. 97: cfi_pop 97b - \unwind, 0xe, 0x0 6ae8a30d44f016cafb46a75843b5109316eb1996 ("gas: have scrubber retain more whitespace") caused the breakage. This case could be handled if we gas recognizes binary operators like `-`.
The master branch has been updated by H.J. Lu <hjl@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=4184396d5878a49c3e25d28d76a278940f5fbe6b commit 4184396d5878a49c3e25d28d76a278940f5fbe6b Author: H.J. Lu <hjl.tools@gmail.com> Date: Wed Aug 14 09:54:33 2024 -0700 Revert "MIPS: correct macro use in gas and ld testsuites" This reverts commit c0e9aca554e33e900efbd6425c1830f0a20012f5. commit 6ae8a30d44f016cafb46a75843b5109316eb1996 Author: Jan Beulich <jbeulich@suse.com> Date: Fri Aug 9 11:59:31 2024 +0200 gas: have scrubber retain more whitespace has been reverted to fix PR gas/32073.
The master branch has been updated by H.J. Lu <hjl@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=5835278179d574b36fe916654f9197f2c6a02f3c commit 5835278179d574b36fe916654f9197f2c6a02f3c Author: H.J. Lu <hjl.tools@gmail.com> Date: Wed Aug 14 09:54:34 2024 -0700 Revert "ia64: correct macro use in gas testsuite" This reverts commit 2231ac9b9e88191178001d0ae5845e292acb2a56. commit 6ae8a30d44f016cafb46a75843b5109316eb1996 Author: Jan Beulich <jbeulich@suse.com> Date: Fri Aug 9 11:59:31 2024 +0200 gas: have scrubber retain more whitespace has been reverted to fix PR gas/32073.
The master branch has been updated by H.J. Lu <hjl@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=a1299dc71ecd5cb89b85ab405c36b1ad76b8614c commit a1299dc71ecd5cb89b85ab405c36b1ad76b8614c Author: H.J. Lu <hjl.tools@gmail.com> Date: Wed Aug 14 09:54:35 2024 -0700 Revert "bfin: correct macro use in gas testsuite" This reverts commit a1b7023447d19d70bc36d71b7627f457dbfae5ce. commit 6ae8a30d44f016cafb46a75843b5109316eb1996 Author: Jan Beulich <jbeulich@suse.com> Date: Fri Aug 9 11:59:31 2024 +0200 gas: have scrubber retain more whitespace has been reverted to fix PR gas/32073.
The master branch has been updated by H.J. Lu <hjl@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=5e3c96bded40e0f3a953288675742317cc35535e commit 5e3c96bded40e0f3a953288675742317cc35535e Author: H.J. Lu <hjl.tools@gmail.com> Date: Wed Aug 14 09:54:36 2024 -0700 Revert "Arm: correct macro use in gas testsuite" This reverts commit cfa18744d435b55bbbbc5ef1ae1df67e84aa1777. commit 6ae8a30d44f016cafb46a75843b5109316eb1996 Author: Jan Beulich <jbeulich@suse.com> Date: Fri Aug 9 11:59:31 2024 +0200 gas: have scrubber retain more whitespace has been reverted to fix PR gas/32073.
Fixed.