[PATCH] gas: retain whitespace between strings

[PATCH] gas: retain whitespace between strings

[Jan Beulich]

Macro arguments may be separated by commas or just blanks. Macro
arguments may also be quoted (where one level of quotes is removed in
the course of determining the values for the respective formal
parameters). Furthermore this quote removal knows _two_ somewhat odd
escaping mechanisms: One, apparently in existence forever, is that a
pair of quotes counts as the escaping of a quote, with the pair being
transformed to a single quote in the course of quote removal. The other
(introduced by c06ae4f232e6) looks more usual on the surface in that it
deals with \" sequences, but it _retains_ the escaping \. Hence only the
former mechanism is suitable when the value to be used by the macro body
is to contain a quote. Yet this results in ambiguity of what "a""b" is
intended to mean; elsewhere (e.g. for .ascii) it represents two
successive string literals. However, in any event is the above different
from "a" "b": I don't think this can be viewed the same as "a""b" when
processing macro arguments.

Change the scrubber to retain such whitespace, by making the processing
of strings more similar to that of symbols. And indeed this appears to
make sense when taking into account that for quite a while gas has been
supporting quoted symbol names.

Taking a more general view, however, the change doesn't go quite far
enough. There are further cases where significant whitespace is removed
by the scrubber. The new testcase enumerates a few in its ".if 0"
section. I'm afraid the only way that I see to deal with this would be
to significantly simplify the scrubber, such that it wouldn't do much
more than collapse sequences of unquoted whitespace into a single blank.
To be honest problems in this area aren't really surprising when seeing
that there's hardly any checking of .macro use throughout the testsuite
(and in particular in the [relatively] generic tests under all/).
---
Partly RFC: While I did test this for x86, I didn't get around yet to
run a much wider set of tests.

--- a/gas/app.c
+++ b/gas/app.c
@@ -1088,10 +1088,10 @@ do_scrub_chars (size_t (*get) (char *, s
 	      /* PUT didn't jump out.  We could just break, but we
 		 know what will happen, so optimize a bit.  */
 	      ch = GET ();
-	      old_state = 3;
+	      old_state = 9;
 	    }
-	  else if (state == 9)
-	    old_state = 3;
+	  else if (state == 3)
+	    old_state = 9;
 	  else
 	    old_state = state;
 	  state = 5;
--- a/gas/testsuite/gas/all/gas.exp
+++ b/gas/testsuite/gas/all/gas.exp
@@ -461,6 +461,8 @@ if [is_elf_format] {
 
 run_dump_test quoted-sym-names
 
+run_list_test macro "-alm"
+
 run_list_test pr20312
 
 load_lib gas-dg.exp
--- /dev/null
+++ b/gas/testsuite/gas/all/macro.l
@@ -0,0 +1,25 @@
+# This should match the output of gas -alm macro.s.
+
+.*: Assembler messages:
+.*:10: Error: too many positional arguments
+.*macro.s.*
+
+
+[ 	]*[1-9][0-9]*[ 	]+\.macro[ 	]+m[ 	]+arg1,[ 	]*arg2[ 	]*
+#...
+[ 	]*[1-9][0-9]*[ 	]+\.endm[ 	]*
+[ 	]*[1-9][0-9]*[ 	]+
+[ 	]*[1-9][0-9]*[ 	]+m[ 	]+1,[ 	]*2
+[ 	]*[1-9][0-9]*[ 	]+.... 01[ 	]+>  .byte 1
+[ 	]*[1-9][0-9]*[ 	]+.... 02[ 	]+>  .byte 2
+[ 	]*[1-9][0-9]*[ 	]+m[ 	]+3[ 	]+4
+[ 	]*[1-9][0-9]*[ 	]+.... 03[ 	]+>  .byte 3
+[ 	]*[1-9][0-9]*[ 	]+.... 04[ 	]+>  .byte 4
+[ 	]*[1-9][0-9]*[ 	]+m[ 	]+"5",[ 	]*"6"
+[ 	]*[1-9][0-9]*[ 	]+.... 05[ 	]+>  .byte 5
+[ 	]*[1-9][0-9]*[ 	]+.... 06[ 	]+>  .byte 6
+[ 	]*[1-9][0-9]*[ 	]+m[ 	]+"7"[ 	]+"8"
+[ 	]*[1-9][0-9]*[ 	]+.... 07[ 	]+>  .byte 7
+[ 	]*[1-9][0-9]*[ 	]+.... 08[ 	]+>  .byte 8
+[ 	]*[1-9][0-9]*[ 	]+m[ 	]+""[ 	]+""[ 	]+""
+#pass
--- /dev/null
+++ b/gas/testsuite/gas/all/macro.s
@@ -0,0 +1,16 @@
+	.macro m arg1, arg2
+	.byte \arg1
+	.byte \arg2
+	.endm
+
+	m 1, 2
+	m 3 4
+	m "5", "6"
+	m "7" "8"
+	m "" "" ""
+
+	.if 0
+	m 1 +2
+	m (3) +4
+	m (5) (6)
+	.endif

Re: [PATCH] gas: retain whitespace between strings

[Nick Clifton]

Hi Jan,

> Change the scrubber to retain such whitespace, by making the processing
> of strings more similar to that of symbols. And indeed this appears to
> make sense when taking into account that for quite a while gas has been
> supporting quoted symbol names.

OK, that makes sense.

> Taking a more general view, however, the change doesn't go quite far
> enough. There are further cases where significant whitespace is removed
> by the scrubber. The new testcase enumerates a few in its ".if 0"
> section. I'm afraid the only way that I see to deal with this would be
> to significantly simplify the scrubber, such that it wouldn't do much
> more than collapse sequences of unquoted whitespace into a single blank.
> To be honest problems in this area aren't really surprising when seeing
> that there's hardly any checking of .macro use throughout the testsuite
> (and in particular in the [relatively] generic tests under all/).

Plus assembler macro syntax has never been that well specified.


> Partly RFC: While I did test this for x86, I didn't get around yet to
> run a much wider set of tests.

I ran my regression tester with the patch applied and nothing turned up,
so I am happy to approve the patch.  One thing though - I think that it
would be a good idea to extend the documentation to cover this behaviour.
I do not mind if you do that as a separate patch, or just an extension
to this one, but I think that it should be done.

Cheers
   Nick

Re: [PATCH] gas: retain whitespace between strings

[Jan Beulich]

Nick,

On 16.03.2022 17:55, Nick Clifton wrote:
>> Change the scrubber to retain such whitespace, by making the processing
>> of strings more similar to that of symbols. And indeed this appears to
>> make sense when taking into account that for quite a while gas has been
>> supporting quoted symbol names.
> 
> OK, that makes sense.
> 
>> Taking a more general view, however, the change doesn't go quite far
>> enough. There are further cases where significant whitespace is removed
>> by the scrubber. The new testcase enumerates a few in its ".if 0"
>> section. I'm afraid the only way that I see to deal with this would be
>> to significantly simplify the scrubber, such that it wouldn't do much
>> more than collapse sequences of unquoted whitespace into a single blank.
>> To be honest problems in this area aren't really surprising when seeing
>> that there's hardly any checking of .macro use throughout the testsuite
>> (and in particular in the [relatively] generic tests under all/).
> 
> Plus assembler macro syntax has never been that well specified.
> 
> 
>> Partly RFC: While I did test this for x86, I didn't get around yet to
>> run a much wider set of tests.
> 
> I ran my regression tester with the patch applied and nothing turned up,
> so I am happy to approve the patch.

Thanks. I did meanwhile get around to do a full run as well, but for
now I can only say "likely nothing turned up" because I did this with
"ELF32: don't silently truncate relocation addends" also in place.
The regressions found are likely all from this 2nd patch (matching
what Alan did point out already). But I didn't get around yet to
inspect the test failures individually.

>  One thing though - I think that it
> would be a good idea to extend the documentation to cover this behaviour.
> I do not mind if you do that as a separate patch, or just an extension
> to this one, but I think that it should be done.

Hmm, I would probably want to fold a related doc change into the patch
right away, but I'm having trouble seeing where this behavior would
sensibly be described. It's an implementation detail, after all, which
simply ended up broken internally. The behavior can't be described for
any directive in particular (or for insns in general), and whether
adjacent strings (irrespective of the intervening whitespace) are
concatenated is directive-dependent.

Or did you mean me to merely describe the specific behavior for .macro,
leaving aside potential effects elsewhere?

Jan

Re: [PATCH] gas: retain whitespace between strings

[Nick Clifton]

Hi Jan,

>>   One thing though - I think that it
>> would be a good idea to extend the documentation to cover this behaviour.

> Or did you mean me to merely describe the specific behavior for .macro,
> leaving aside potential effects elsewhere?

I think that this should be sufficient. :-)

Cheers
   Nick

Re: [PATCH] gas: retain whitespace between strings

[Jan Beulich]

On 18.03.2022 16:44, Nick Clifton wrote:
>>>   One thing though - I think that it
>>> would be a good idea to extend the documentation to cover this behaviour.
> 
>> Or did you mean me to merely describe the specific behavior for .macro,
>> leaving aside potential effects elsewhere?
> 
> I think that this should be sufficient. :-)

Okay, I'll try to invent some wording. But before that I guess I'll
actually need to deal a strange regression with the change in tic4x
and tic54x - the output of the .byte directives in all/macro.s has
extra zeros appended in the listing (didn't check yet whether that's
also the case in the generated object files). Of the changes I have
under test, I can only guess it's the one here, but of course I'll
need to isolate that as the first step to figuring what's going on
there. I'm mentioning it here merely in case it rings a bell ...

Jan

Re: [PATCH] gas: retain whitespace between strings

[Jan Beulich]

On 21.03.2022 08:16, Jan Beulich via Binutils wrote:
> On 18.03.2022 16:44, Nick Clifton wrote:
>>>>   One thing though - I think that it
>>>> would be a good idea to extend the documentation to cover this behaviour.
>>
>>> Or did you mean me to merely describe the specific behavior for .macro,
>>> leaving aside potential effects elsewhere?
>>
>> I think that this should be sufficient. :-)
> 
> Okay, I'll try to invent some wording. But before that I guess I'll
> actually need to deal a strange regression with the change in tic4x
> and tic54x - the output of the .byte directives in all/macro.s has
> extra zeros appended in the listing (didn't check yet whether that's
> also the case in the generated object files). Of the changes I have
> under test, I can only guess it's the one here, but of course I'll
> need to isolate that as the first step to figuring what's going on
> there. I'm mentioning it here merely in case it rings a bell ...

Ah, yes - I've spotted respective comments and alike for other test
cases.

Jan