diff mbox series

[07/11] gas: consistently drop trailing whitespace when scrubbing

Message ID 1e5129fb-d94a-4b0a-97d6-c5ec1771bb7b@suse.com
State Superseded
Headers
Series gas: scrubber adjustments |

Commit Message

Jan Beulich June 28, 2024, 1:22 p.m. UTC 
  From especially the checks for the two separator forms it appears to
follow that the construct being touched is about trailing whitespace. In
such a case, considering that for many targets ordinary and line comment
chars overlap, take into account that line comment chars override
ordinary ones in lex[] (logic elsewhere in do_scrub_chars() actually
depends on that ordering, and also accounts for this overriding).

Plus of course IS_NEWLINE() would better also be consulted. Note also
that the DOUBLESLASH_LINE_COMMENTS change should generally have no
effect just yet; it's a prereq for a later change but better fits here.

Leave respective comments as well, and update documentation to correct
which comment form is actually replaced by a single blank (i.e. neither
the ones starting with what {,tc_}comment_chars[] has nor the ones
starting with what line_comment_chars[] has).
---
In the MIPS testsuite adjustments I wasn't sure how exactly to make the
necessary changes: I could also have zapped the two blanks each, but
then I wondered why they were part of the expectations in the first
place.

Why are there both comment_chars[] and tc_comment_chars[]? The former is
per-arch anyway, so why would there be a need for an arch override? Is
this perhaps merely a historical leftover, which could be eliminated?

Comments

Maciej W. Rozycki June 28, 2024, 1:53 p.m. UTC | #1 
On Fri, 28 Jun 2024, Jan Beulich wrote:

> >From especially the checks for the two separator forms it appears to

 Accidental `>' here?

> In the MIPS testsuite adjustments I wasn't sure how exactly to make the
> necessary changes: I could also have zapped the two blanks each, but
> then I wondered why they were part of the expectations in the first
> place.

 Please do.

 The only reason I can think of why the patterns are such as they are is 
that they need to match the output as it has been produced.  It made no 
sense to make whitespace matching optional when said whitespace is always 
there.

 Of course I could have investigated why this extraneous whitespace is 
present there in the first place, but either I chose it wasn't worth it or 
I missed it in the eyeball review of the output produced; there are only 
two among 133 lines and not key to the purpose of the test case.  It's 
been a while, I genuinely do not remember.

 Thank you for making this clean-up, and taking care of the MIPS port in 
particular.

  Maciej
Jan Beulich July 1, 2024, 5:58 a.m. UTC | #2 
On 28.06.2024 15:53, Maciej W. Rozycki wrote:
> On Fri, 28 Jun 2024, Jan Beulich wrote:
> 
>> >From especially the checks for the two separator forms it appears to
> 
>  Accidental `>' here?

Indeed. Don't have that locally, so not really sure where it came from.

>> In the MIPS testsuite adjustments I wasn't sure how exactly to make the
>> necessary changes: I could also have zapped the two blanks each, but
>> then I wondered why they were part of the expectations in the first
>> place.
> 
>  Please do.

Done locally; I won't post a v2 just for this, though.

>  The only reason I can think of why the patterns are such as they are is 
> that they need to match the output as it has been produced.  It made no 
> sense to make whitespace matching optional when said whitespace is always 
> there.

But the purpose of those tests isn't the _exact_ output that gas would
produce, is it? The presence / absence of whitespace in most tests
really is entirely unrelated to what is being tested. Just to mention
it ...

Jan
Maciej W. Rozycki July 1, 2024, 11:05 a.m. UTC | #3 
On Mon, 1 Jul 2024, Jan Beulich wrote:

> >> In the MIPS testsuite adjustments I wasn't sure how exactly to make the
> >> necessary changes: I could also have zapped the two blanks each, but
> >> then I wondered why they were part of the expectations in the first
> >> place.
> > 
> >  Please do.
> 
> Done locally; I won't post a v2 just for this, though.

 Thank you, and agreed.

> >  The only reason I can think of why the patterns are such as they are is 
> > that they need to match the output as it has been produced.  It made no 
> > sense to make whitespace matching optional when said whitespace is always 
> > there.
> 
> But the purpose of those tests isn't the _exact_ output that gas would
> produce, is it? The presence / absence of whitespace in most tests
> really is entirely unrelated to what is being tested. Just to mention
> it ...

 Of course, but something has to be placed there as the pattern to match 
against and taking the message produced literally, whitespace or not, is 
the simplest and most obvious choice.

 Also think of the MIPS tests as coverage for your good work; is there any 
other?

  Maciej
Michael Matz July 3, 2024, 1:42 p.m. UTC | #4 
On Mon, 1 Jul 2024, Jan Beulich wrote:

> On 28.06.2024 15:53, Maciej W. Rozycki wrote:
> > On Fri, 28 Jun 2024, Jan Beulich wrote:
> > 
> >> >From especially the checks for the two separator forms it appears to
> > 
> >  Accidental `>' here?
> 
> Indeed. Don't have that locally, so not really sure where it came from.

That happens if anything in the mailpipe doesn't properly deal 
with from-escaping in mbox files:

  https://doc.dovecot.org/admin_manual/mailbox_formats/mbox/#from-escaping

(either too triger-happy escaping on the sender side, or not enough 
de-escaping on the store/forward/retrieve side)


Ciao,
Michael.
diff mbox series

Patch

--- a/gas/app.c
+++ b/gas/app.c
@@ -87,6 +87,7 @@  static char last_char;
 #define IS_PARALLEL_SEPARATOR(c)	(lex[c] == LEX_IS_PARALLEL_SEPARATOR)
 #define IS_COMMENT(c)			(lex[c] == LEX_IS_COMMENT_START)
 #define IS_LINE_COMMENT(c)		(lex[c] == LEX_IS_LINE_COMMENT_START)
+#define IS_TWOCHAR_COMMENT_1ST(c)	(lex[c] == LEX_IS_TWOCHAR_COMMENT_1ST)
 #define	IS_NEWLINE(c)			(lex[c] == LEX_IS_NEWLINE)
 
 static char lex[256] = {
@@ -149,6 +150,9 @@  do_scrub_begin (int m68k_mri ATTRIBUTE_U
   for (p = tc_comment_chars; *p; p++)
     lex[(unsigned char) *p] = LEX_IS_COMMENT_START;
 
+  /* While counter intuitive to have more special purpose line comment chars
+     override more general purpose ordinary ones, logic in do_scrub_chars()
+     depends on this ordering.   */
   for (p = line_comment_chars; *p; p++)
     lex[(unsigned char) *p] = LEX_IS_LINE_COMMENT_START;
 
@@ -888,7 +892,12 @@  do_scrub_chars (size_t (*get) (char *, s
 		}
 	    }
 #endif
+
+	  /* Prune trailing whitespace.  */
 	  if (IS_COMMENT (ch)
+	      || (IS_LINE_COMMENT (ch)
+	          && (state < 1 || strchr (tc_comment_chars, ch)))
+	      || IS_NEWLINE (ch)
 	      || IS_LINE_SEPARATOR (ch)
 	      || IS_PARALLEL_SEPARATOR (ch))
 	    {
@@ -901,6 +910,16 @@  do_scrub_chars (size_t (*get) (char *, s
 		}
 	      goto recycle;
 	    }
+#ifdef DOUBLESLASH_LINE_COMMENTS
+	  if (IS_TWOCHAR_COMMENT_1ST (ch))
+	    {
+	      ch2 = GET ();
+	      if (ch2 != EOF)
+	        UNGET (ch2);
+	      if (ch2 == '/')
+		goto recycle;
+	    }
+#endif
 
 	  /* If we're in state 2 or 11, we've seen a non-white
 	     character followed by whitespace.  If the next character
--- a/gas/doc/as.texi
+++ b/gas/doc/as.texi
@@ -2987,11 +2987,11 @@  as exactly one space.
 @section Comments
 
 @cindex comments
-There are two ways of rendering comments to @command{@value{AS}}.  In both
-cases the comment is equivalent to one space.
+There are two ways of rendering comments to @command{@value{AS}}.
 
 Anything from @samp{/*} through the next @samp{*/} is a comment.
-This means you may not nest these comments.
+This means you may not nest these comments.  Such a comment is equivalent to
+one space, plus bumping the line counter accordingly.
 
 @smallexample
 /*
--- a/gas/testsuite/gas/mips/mips16-32@mips16-insn-e.l
+++ b/gas/testsuite/gas/mips/mips16-32@mips16-insn-e.l
@@ -25,7 +25,7 @@ 
 .*:50: Warning: extended operand requested but not required
 .*:51: Error: opcode not supported on this processor: mips1 \(mips1\) `restore\.e 128'
 .*:52: Error: opcode not supported on this processor: mips1 \(mips1\) `save\.e 128'
-.*:53: Error: unrecognized extended version of MIPS16 opcode `nop\.e '
+.*:53: Error: unrecognized extended version of MIPS16 opcode `nop\.e ?'
 .*:54: Error: unrecognized extended version of MIPS16 opcode `move\.e \$0,\$16'
 .*:55: Error: unrecognized extended version of MIPS16 opcode `move\.e \$16,\$0'
 .*:57: Warning: extended operand requested but not required
@@ -71,7 +71,7 @@ 
 .*:123: Error: unrecognized extended version of MIPS16 opcode `srav\.e \$16,\$16'
 .*:124: Error: operand 2 must be an immediate expression `sra\.e \$16,\$16'
 .*:125: Error: opcode not supported on this processor: mips1 \(mips1\) `dsrl\.e \$16,8'
-.*:126: Error: unrecognized extended version of MIPS16 opcode `entry\.e '
+.*:126: Error: unrecognized extended version of MIPS16 opcode `entry\.e ?'
 .*:127: Error: unrecognized extended version of MIPS16 opcode `entry\.e \$31'
 .*:128: Error: unrecognized extended version of MIPS16 opcode `exit\.e \$f0'
 .*:129: Error: unrecognized extended version of MIPS16 opcode `exit\.e'
--- a/gas/testsuite/gas/mips/mips16-64@mips16-insn-e.l
+++ b/gas/testsuite/gas/mips/mips16-64@mips16-insn-e.l
@@ -25,7 +25,7 @@ 
 .*:50: Warning: extended operand requested but not required
 .*:51: Error: opcode not supported on this processor: mips3 \(mips3\) `restore\.e 128'
 .*:52: Error: opcode not supported on this processor: mips3 \(mips3\) `save\.e 128'
-.*:53: Error: unrecognized extended version of MIPS16 opcode `nop\.e '
+.*:53: Error: unrecognized extended version of MIPS16 opcode `nop\.e ?'
 .*:54: Error: unrecognized extended version of MIPS16 opcode `move\.e \$0,\$16'
 .*:55: Error: unrecognized extended version of MIPS16 opcode `move\.e \$16,\$0'
 .*:57: Warning: extended operand requested but not required
@@ -71,7 +71,7 @@ 
 .*:123: Error: unrecognized extended version of MIPS16 opcode `srav\.e \$16,\$16'
 .*:124: Error: operand 2 must be an immediate expression `sra\.e \$16,\$16'
 .*:125: Warning: extended operand requested but not required
-.*:126: Error: unrecognized extended version of MIPS16 opcode `entry\.e '
+.*:126: Error: unrecognized extended version of MIPS16 opcode `entry\.e ?'
 .*:127: Error: unrecognized extended version of MIPS16 opcode `entry\.e \$31'
 .*:128: Error: unrecognized extended version of MIPS16 opcode `exit\.e \$f0'
 .*:129: Error: unrecognized extended version of MIPS16 opcode `exit\.e'
--- a/gas/testsuite/gas/mips/mips16-insn-e.l
+++ b/gas/testsuite/gas/mips/mips16-insn-e.l
@@ -23,7 +23,7 @@ 
 .*:48: Warning: extended operand requested but not required
 .*:49: Warning: extended operand requested but not required
 .*:50: Warning: extended operand requested but not required
-.*:53: Error: unrecognized extended version of MIPS16 opcode `nop\.e '
+.*:53: Error: unrecognized extended version of MIPS16 opcode `nop\.e ?'
 .*:54: Error: unrecognized extended version of MIPS16 opcode `move\.e \$0,\$16'
 .*:55: Error: unrecognized extended version of MIPS16 opcode `move\.e \$16,\$0'
 .*:57: Warning: extended operand requested but not required
@@ -69,7 +69,7 @@ 
 .*:123: Error: unrecognized extended version of MIPS16 opcode `srav\.e \$16,\$16'
 .*:124: Error: operand 2 must be an immediate expression `sra\.e \$16,\$16'
 .*:125: Warning: extended operand requested but not required
-.*:126: Error: unrecognized extended version of MIPS16 opcode `entry\.e '
+.*:126: Error: unrecognized extended version of MIPS16 opcode `entry\.e ?'
 .*:127: Error: unrecognized extended version of MIPS16 opcode `entry\.e \$31'
 .*:128: Error: unrecognized extended version of MIPS16 opcode `exit\.e \$f0'
 .*:129: Error: unrecognized extended version of MIPS16 opcode `exit\.e'
--- a/gas/testsuite/gas/mips/mips16e-32@mips16-insn-e.l
+++ b/gas/testsuite/gas/mips/mips16e-32@mips16-insn-e.l
@@ -23,7 +23,7 @@ 
 .*:48: Warning: extended operand requested but not required
 .*:49: Warning: extended operand requested but not required
 .*:50: Warning: extended operand requested but not required
-.*:53: Error: unrecognized extended version of MIPS16 opcode `nop\.e '
+.*:53: Error: unrecognized extended version of MIPS16 opcode `nop\.e ?'
 .*:54: Error: unrecognized extended version of MIPS16 opcode `move\.e \$0,\$16'
 .*:55: Error: unrecognized extended version of MIPS16 opcode `move\.e \$16,\$0'
 .*:57: Warning: extended operand requested but not required
@@ -69,7 +69,7 @@ 
 .*:123: Error: unrecognized extended version of MIPS16 opcode `srav\.e \$16,\$16'
 .*:124: Error: operand 2 must be an immediate expression `sra\.e \$16,\$16'
 .*:125: Error: opcode not supported on this processor: mips32 \(mips32\) `dsrl\.e \$16,8'
-.*:126: Error: unrecognized extended version of MIPS16 opcode `entry\.e '
+.*:126: Error: unrecognized extended version of MIPS16 opcode `entry\.e ?'
 .*:127: Error: unrecognized extended version of MIPS16 opcode `entry\.e \$31'
 .*:128: Error: unrecognized extended version of MIPS16 opcode `exit\.e \$f0'
 .*:129: Error: unrecognized extended version of MIPS16 opcode `exit\.e'
--- a/gas/testsuite/gas/mips/mips16e2-32@mips16-insn-e.l
+++ b/gas/testsuite/gas/mips/mips16e2-32@mips16-insn-e.l
@@ -23,7 +23,7 @@ 
 .*:48: Warning: extended operand requested but not required
 .*:49: Warning: extended operand requested but not required
 .*:50: Warning: extended operand requested but not required
-.*:53: Error: unrecognized extended version of MIPS16 opcode `nop\.e '
+.*:53: Error: unrecognized extended version of MIPS16 opcode `nop\.e ?'
 .*:54: Error: unrecognized extended version of MIPS16 opcode `move\.e \$0,\$16'
 .*:55: Error: unrecognized extended version of MIPS16 opcode `move\.e \$16,\$0'
 .*:57: Warning: extended operand requested but not required
@@ -69,7 +69,7 @@ 
 .*:123: Error: unrecognized extended version of MIPS16 opcode `srav\.e \$16,\$16'
 .*:124: Error: operand 2 must be an immediate expression `sra\.e \$16,\$16'
 .*:125: Error: opcode not supported on this processor: mips32r2 \(mips32r2\) `dsrl\.e \$16,8'
-.*:126: Error: unrecognized extended version of MIPS16 opcode `entry\.e '
+.*:126: Error: unrecognized extended version of MIPS16 opcode `entry\.e ?'
 .*:127: Error: unrecognized extended version of MIPS16 opcode `entry\.e \$31'
 .*:128: Error: unrecognized extended version of MIPS16 opcode `exit\.e \$f0'
 .*:129: Error: unrecognized extended version of MIPS16 opcode `exit\.e'
--- a/gas/testsuite/gas/mips/mips16e2-interaptiv-mr2@mips16-insn-e.l
+++ b/gas/testsuite/gas/mips/mips16e2-interaptiv-mr2@mips16-insn-e.l
@@ -23,7 +23,7 @@ 
 .*:48: Warning: extended operand requested but not required
 .*:49: Warning: extended operand requested but not required
 .*:50: Warning: extended operand requested but not required
-.*:53: Error: unrecognized extended version of MIPS16 opcode `nop\.e '
+.*:53: Error: unrecognized extended version of MIPS16 opcode `nop\.e ?'
 .*:54: Error: unrecognized extended version of MIPS16 opcode `move\.e \$0,\$16'
 .*:55: Error: unrecognized extended version of MIPS16 opcode `move\.e \$16,\$0'
 .*:57: Warning: extended operand requested but not required
@@ -69,7 +69,7 @@ 
 .*:123: Error: unrecognized extended version of MIPS16 opcode `srav\.e \$16,\$16'
 .*:124: Error: operand 2 must be an immediate expression `sra\.e \$16,\$16'
 .*:125: Error: opcode not supported on this processor: interaptiv-mr2 \(mips32r3\) `dsrl\.e \$16,8'
-.*:126: Error: unrecognized extended version of MIPS16 opcode `entry\.e '
+.*:126: Error: unrecognized extended version of MIPS16 opcode `entry\.e ?'
 .*:127: Error: unrecognized extended version of MIPS16 opcode `entry\.e \$31'
 .*:128: Error: unrecognized extended version of MIPS16 opcode `exit\.e \$f0'
 .*:129: Error: unrecognized extended version of MIPS16 opcode `exit\.e'