demand_empty_rest_of_line and ignore_rest_of_line

demand_empty_rest_of_line and ignore_rest_of_line

[Nathan Sidwell]

Hi,
what is the intended difference between demand_empty_rest_of_line and
ignore_rest_of_line? From the source I see that demand_empty_ROL skips
whitespace but ignore_ROL does not. Then, demand_empty falls into
ignore_ROL to issue a warning and skip up to the EOL. All but one use
of ignore_ROL I saw (I've not checked the config dir yet) were of the form
	as_{bad,warn} ("something bad happened");
	ignore_rest_of_line ();
which implies to me that ignore_ROL should be silent.

Also, what do people think about demand_empty_ROL issuing an error?
IMHO, if the syntax requires no more stuff, it's an error if there is
more stuff.

Would a patch which made ignore_ROL silent and demand_empty_ROL issue
an error be acceptable?

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::     CodeSourcery LLC
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Hans-Peter Nilsson]

On Wed, 17 Mar 2004, Nathan Sidwell wrote:
> what is the intended difference between demand_empty_rest_of_line and
> ignore_rest_of_line?

From memory and trigged by the names (i.e. not cheating and
looking at the code ;-) one is supposed to issue an error
(demand_*) and the other (ignore_*) should just skip to the next
line (and update whatever counters and such).  If they don't,
I'd argue it's confusing enough that a change is warranted.

> which implies to me that ignore_ROL should be silent.

It's not?

> Also, what do people think about demand_empty_ROL issuing an error?

It doesn't?

:-)

> IMHO, if the syntax requires no more stuff, it's an error if there is
> more stuff.
>
> Would a patch which made ignore_ROL silent and demand_empty_ROL issue
> an error be acceptable?

It's used by config/tc-* so I suggest regression checking enough
targets to cover all target uses of demand_* and ignore_*.

brgds, H-P

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Ian Lance Taylor]

Nathan Sidwell <nathan@codesourcery.com> writes:

> what is the intended difference between demand_empty_rest_of_line and
> ignore_rest_of_line? From the source I see that demand_empty_ROL skips
> whitespace but ignore_ROL does not. Then, demand_empty falls into
> ignore_ROL to issue a warning and skip up to the EOL. All but one use
> of ignore_ROL I saw (I've not checked the config dir yet) were of the form
> 	as_{bad,warn} ("something bad happened");
> 	ignore_rest_of_line ();
> which implies to me that ignore_ROL should be silent.

demand_empty_rest_of_line is called when the rest of the line is
required to be empty, and we have no reason to think that it is not
empty.

ignore_rest_of_line is called after an error occurs, and is intended
to help the user see where the error happened, by indicating what
point the assembler reached on the line before it got the error.

So, I agree that demand_empty_rest_of_line should probably issue an
error rather than a warning.

I'm agnostic about whether ignore_rest_of_line should be silent.  The
current code is not a mistake.  But it's fine with me to change it if
making it shut up appears more useful in practice.

Ian

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Nathan Sidwell]

[-- Attachment #1: Type: text/plain, Size: 562 bytes --]

Hi,
This patch implements
*) demand_empty_rest_of_line issues an error and skips to EOL
*) ignore_rest_of_line silently skips to EOL.

I went through the config dir and inspected all uses of
ignore_rest_of_line.  Those that needed to be demand_empty_rest_of_line
are changed.  I tested with the following targets,

arc-unknown-elf
i686-pc-linux-gnu
ia64-unknown-linux-gnu
mips-unknown-elf

ok?

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::     CodeSourcery LLC
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk


[-- Attachment #2: EOL.patch --]
[-- Type: text/plain, Size: 8632 bytes --]

2004-03-18  Nathan Sidwell  <nathan@codesourcery.com>

	* read.c (read_a_source_file): Use demand_empty_rest_of_line.
	(demand_empty_rest_of_line): Issue an error here.
	(ignore_rest_of_line): Silently skip to end.
	(demand_copy_string): Issue an error, not warning.
	(equals): Likewise.
	* config/obj-elf.c (obj_elf_section_name): Likewise.
	(obj_elf_section): Likewise.
	* config/tc-arc.c (arc_extoper): Remove bogus NULL checks.
	(arc_extinst): Likewise.
	* config/tc-ia64.c (dot_saveb): Use demand_empty_rest_of_line.
	(dot_spill): Likewise.
	(dot_unwabi): Likewise.
	(dot_prologue): Likewise.

Index: read.c
===================================================================
RCS file: /cvs/src/src/gas/read.c,v
retrieving revision 1.76
diff -c -3 -p -r1.76 read.c
*** read.c	12 Mar 2004 17:48:12 -0000	1.76
--- read.c	18 Mar 2004 09:56:05 -0000
*************** read_a_source_file (char *name)
*** 1053,1059 ****
  #endif
  	  input_line_pointer--;
  	  /* Report unknown char as ignored.  */
! 	  ignore_rest_of_line ();
  	}
  
  #ifdef md_after_pass_hook
--- 1053,1059 ----
  #endif
  	  input_line_pointer--;
  	  /* Report unknown char as ignored.  */
! 	  demand_empty_rest_of_line ();
  	}
  
  #ifdef md_after_pass_hook
*************** s_text (int ignore ATTRIBUTE_UNUSED)
*** 3020,3025 ****
--- 3020,3029 ----
  #endif
  }
  \f
+ 
+ /* Verify that we are at the end of a line.  If not, issue an error and
+    skip to EOL.  */
+ 
  void
  demand_empty_rest_of_line (void)
  {
*************** demand_empty_rest_of_line (void)
*** 3027,3054 ****
    if (is_end_of_line[(unsigned char) *input_line_pointer])
      input_line_pointer++;
    else
-     ignore_rest_of_line ();
- 
-   /* Return having already swallowed end-of-line.  */
- }
- 
- void
- ignore_rest_of_line (void)
- {
-   /* For suspect lines: gives warning.  */
-   if (!is_end_of_line[(unsigned char) *input_line_pointer])
      {
        if (ISPRINT (*input_line_pointer))
! 	as_warn (_("rest of line ignored; first ignored character is `%c'"),
  		 *input_line_pointer);
        else
! 	as_warn (_("rest of line ignored; first ignored character valued 0x%x"),
  		 *input_line_pointer);
! 
!       while (input_line_pointer < buffer_limit
! 	     && !is_end_of_line[(unsigned char) *input_line_pointer])
! 	input_line_pointer++;
      }
  
    input_line_pointer++;
  
--- 3031,3059 ----
    if (is_end_of_line[(unsigned char) *input_line_pointer])
      input_line_pointer++;
    else
      {
        if (ISPRINT (*input_line_pointer))
! 	as_bad (_("junk at end of line, first unrecognized character is `%c'"),
  		 *input_line_pointer);
        else
! 	as_bad (_("junk at end of line, first unrecognized character valued 0x%x"),
  		 *input_line_pointer);
!       ignore_rest_of_line ();
      }
+   
+   /* Return pointing just after end-of-line.  */
+   know (is_end_of_line[(unsigned char) input_line_pointer[-1]]);
+ }
+ 
+ /* Silently advance to the end of line.  Use this after already having
+    issued an error about something bad.  */
+ 
+ void
+ ignore_rest_of_line (void)
+ {
+   while (input_line_pointer < buffer_limit
+ 	 && !is_end_of_line[(unsigned char) *input_line_pointer])
+     input_line_pointer++;
  
    input_line_pointer++;
  
*************** demand_copy_string (int *lenP)
*** 4738,4744 ****
      }
    else
      {
!       as_warn (_("missing string"));
        retval = NULL;
        ignore_rest_of_line ();
      }
--- 4743,4749 ----
      }
    else
      {
!       as_bad (_("missing string"));
        retval = NULL;
        ignore_rest_of_line ();
      }
*************** equals (char *sym_name, int reassign)
*** 4814,4820 ****
    if (flag_mri)
      {
        /* Check garbage after the expression.  */
!       ignore_rest_of_line ();
        mri_comment_end (stop, stopc);
      }
  }
--- 4819,4825 ----
    if (flag_mri)
      {
        /* Check garbage after the expression.  */
!       demand_empty_rest_of_line ();
        mri_comment_end (stop, stopc);
      }
  }
Index: config/obj-elf.c
===================================================================
RCS file: /cvs/src/src/gas/config/obj-elf.c,v
retrieving revision 1.79
diff -c -3 -p -r1.79 obj-elf.c
*** config/obj-elf.c	13 Dec 2003 12:57:40 -0000	1.79
--- config/obj-elf.c	18 Mar 2004 09:56:09 -0000
*************** obj_elf_section_name (void)
*** 787,793 ****
  	end++;
        if (end == input_line_pointer)
  	{
! 	  as_warn (_("missing name"));
  	  ignore_rest_of_line ();
  	  return NULL;
  	}
--- 787,793 ----
  	end++;
        if (end == input_line_pointer)
  	{
! 	  as_bad (_("missing name"));
  	  ignore_rest_of_line ();
  	  return NULL;
  	}
*************** obj_elf_section (int push)
*** 938,944 ****
  	      SKIP_WHITESPACE ();
  	      if (*input_line_pointer != '#')
  		{
! 		  as_warn (_("character following name is not '#'"));
  		  ignore_rest_of_line ();
  		  return;
  		}
--- 938,944 ----
  	      SKIP_WHITESPACE ();
  	      if (*input_line_pointer != '#')
  		{
! 		  as_bad (_("character following name is not '#'"));
  		  ignore_rest_of_line ();
  		  return;
  		}
Index: config/tc-arc.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-arc.c,v
retrieving revision 1.25
diff -c -3 -p -r1.25 tc-arc.c
*** config/tc-arc.c	10 Dec 2003 06:41:08 -0000	1.25
--- config/tc-arc.c	18 Mar 2004 09:56:13 -0000
*************** arc_extoper (opertype)
*** 905,915 ****
    name = input_line_pointer;
    c = get_symbol_end ();
    name = xstrdup (name);
-   if (NULL == name)
-     {
-       ignore_rest_of_line ();
-       return;
-     }
  
    p = name;
    while (*p)
--- 905,910 ----
*************** arc_extinst (ignore)
*** 1153,1163 ****
    name = input_line_pointer;
    c = get_symbol_end ();
    name = xstrdup (name);
-   if (NULL == name)
-     {
-       ignore_rest_of_line ();
-       return;
-     }
    strcpy (syntax, name);
    name_len = strlen (name);
  
--- 1148,1153 ----
*************** arc_extinst (ignore)
*** 1305,1322 ****
    strcat (syntax, "%S%L");
  
    ext_op = (struct arc_opcode *) xmalloc (sizeof (struct arc_opcode));
-   if (NULL == ext_op)
-     {
-       ignore_rest_of_line ();
-       return;
-     }
- 
    ext_op->syntax = xstrdup (syntax);
-   if (NULL == ext_op->syntax)
-     {
-       ignore_rest_of_line ();
-       return;
-     }
  
    ext_op->mask  = I (-1) | ((0x3 == opcode) ? C (-1) : 0);
    ext_op->value = I (opcode) | ((0x3 == opcode) ? C (subopcode) : 0);
--- 1295,1301 ----
Index: config/tc-ia64.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-ia64.c,v
retrieving revision 1.106
diff -c -3 -p -r1.106 tc-ia64.c
*** config/tc-ia64.c	5 Mar 2004 17:07:12 -0000	1.106
--- config/tc-ia64.c	18 Mar 2004 09:56:32 -0000
*************** dot_saveb (dummy)
*** 3617,3623 ****
      add_unwind_entry (output_br_mem (brmask));
  
    if (!is_end_of_line[sep] && !is_it_end_of_statement ())
!     ignore_rest_of_line ();
  }
  
  static void
--- 3617,3623 ----
      add_unwind_entry (output_br_mem (brmask));
  
    if (!is_end_of_line[sep] && !is_it_end_of_statement ())
!     demand_empty_rest_of_line ();
  }
  
  static void
*************** dot_spill (dummy)
*** 3649,3655 ****
  
    sep = parse_operand (&e);
    if (!is_end_of_line[sep] && !is_it_end_of_statement ())
!     ignore_rest_of_line ();
  
    if (e.X_op != O_constant)
      as_bad ("Operand to .spill must be a constant");
--- 3649,3655 ----
  
    sep = parse_operand (&e);
    if (!is_end_of_line[sep] && !is_it_end_of_statement ())
!     demand_empty_rest_of_line ();
  
    if (e.X_op != O_constant)
      as_bad ("Operand to .spill must be a constant");
*************** dot_unwabi (dummy)
*** 3925,3931 ****
      }
    sep = parse_operand (&e2);
    if (!is_end_of_line[sep] && !is_it_end_of_statement ())
!     ignore_rest_of_line ();
  
    if (e1.X_op != O_constant)
      {
--- 3925,3931 ----
      }
    sep = parse_operand (&e2);
    if (!is_end_of_line[sep] && !is_it_end_of_statement ())
!     demand_empty_rest_of_line ();
  
    if (e1.X_op != O_constant)
      {
*************** dot_prologue (dummy)
*** 4020,4026 ****
  	as_bad ("No second operand to .prologue");
        sep = parse_operand (&e2);
        if (!is_end_of_line[sep] && !is_it_end_of_statement ())
! 	ignore_rest_of_line ();
  
        if (e1.X_op == O_constant)
  	{
--- 4020,4026 ----
  	as_bad ("No second operand to .prologue");
        sep = parse_operand (&e2);
        if (!is_end_of_line[sep] && !is_it_end_of_statement ())
! 	demand_empty_rest_of_line ();
  
        if (e1.X_op == O_constant)
  	{

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Ian Lance Taylor]

Nathan Sidwell <nathan@codesourcery.com> writes:

> 2004-03-18  Nathan Sidwell  <nathan@codesourcery.com>
> 
> 	* read.c (read_a_source_file): Use demand_empty_rest_of_line.
> 	(demand_empty_rest_of_line): Issue an error here.
> 	(ignore_rest_of_line): Silently skip to end.
> 	(demand_copy_string): Issue an error, not warning.
> 	(equals): Likewise.
> 	* config/obj-elf.c (obj_elf_section_name): Likewise.
> 	(obj_elf_section): Likewise.
> 	* config/tc-arc.c (arc_extoper): Remove bogus NULL checks.
> 	(arc_extinst): Likewise.
> 	* config/tc-ia64.c (dot_saveb): Use demand_empty_rest_of_line.
> 	(dot_spill): Likewise.
> 	(dot_unwabi): Likewise.
> 	(dot_prologue): Likewise.

OK.

Ian

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Andreas Schwab]

Nathan Sidwell <nathan@codesourcery.com> writes:

> Index: read.c
> ===================================================================
> RCS file: /cvs/src/src/gas/read.c,v
> retrieving revision 1.76
> diff -c -3 -p -r1.76 read.c
> *** read.c	12 Mar 2004 17:48:12 -0000	1.76
> --- read.c	18 Mar 2004 09:56:05 -0000
> *************** read_a_source_file (char *name)
> *** 1053,1059 ****
>   #endif
>   	  input_line_pointer--;
>   	  /* Report unknown char as ignored.  */
> ! 	  ignore_rest_of_line ();
>   	}
>   
>   #ifdef md_after_pass_hook
> --- 1053,1059 ----
>   #endif
>   	  input_line_pointer--;
>   	  /* Report unknown char as ignored.  */
> ! 	  demand_empty_rest_of_line ();
>   	}
>   
>   #ifdef md_after_pass_hook

This means that the unknown character is no longer ignored, despite the
comment.  As a side effect a line starting with a line comment character
not followed by APP in NO_APP mode now triggers an error instead of just a
warning, breaking builds of glibc on m68k-linux.  Earlier in
read_a_source_file where #APP is handled there is another comment that
claims that unknown comments are ignored, when in fact they aren't (only
the initial line comment character is skipped).

Note that the presence of #APP will mess up the line counters, but
that appears to be difficult to fix.

Andreas.

2004-04-23  Andreas Schwab  <schwab@suse.de>

	* read.c (read_a_source_file): Ignore unknown text after line
	comment character.  Fix misleading comment.

--- gas/read.c.~1.78.~	2004-04-23 08:58:23.000000000 +0200
+++ gas/read.c	2004-04-23 21:49:01.000000000 +0200
@@ -1,6 +1,6 @@
 /* read.c - read a source file -
    Copyright 1986, 1987, 1990, 1991, 1992, 1993, 1994, 1995, 1996, 1997,
-   1998, 1999, 2000, 2001, 2002, 2003 Free Software Foundation, Inc.
+   1998, 1999, 2000, 2001, 2002, 2003, 2004 Free Software Foundation, Inc.
 
 This file is part of GAS, the GNU Assembler.
 
@@ -950,10 +950,14 @@ read_a_source_file (char *name)
 	      unsigned int new_length;
 	      char *tmp_buf = 0;
 
-	      bump_line_counters ();
 	      s = input_line_pointer;
 	      if (strncmp (s, "APP\n", 4))
-		continue;	/* We ignore it */
+		{
+		  /* We ignore it */
+		  ignore_rest_of_line ();
+		  continue;
+		}
+	      bump_line_counters ();
 	      s += 4;
 
 	      sb_new (&sbuf);
@@ -1052,7 +1056,7 @@ read_a_source_file (char *name)
 	    continue;
 #endif
 	  input_line_pointer--;
-	  /* Report unknown char as ignored.  */
+	  /* Report unknown char as error.  */
 	  demand_empty_rest_of_line ();
 	}
 

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Hans-Peter Nilsson]

On Fri, 23 Apr 2004, Andreas Schwab wrote:
> This means that the unknown character is no longer ignored, despite the
> comment.  As a side effect a line starting with a line comment character
> not followed by APP in NO_APP mode now triggers an error instead of just a
> warning, breaking builds of glibc on m68k-linux.

For the record, there aren't supposed to *be* any comments in
#NO_APP mode; just strict formatted assembly code and #APP of
course.  That's what it means.

If the glibc __sec_comment kludge is the problem, let me mention
that different patches have been sent to glibc in order to make
its kludge work properly(!) at least one that adds #NO_APP and
I'm sure would work, but it seems none have been applied.

Of course, we could do something that lets glibc emit its
.gnu.warning.* sections without assembler warnings, and avoid
using the __sec_comment kludge where the new thing is supported.

brgds, H-P

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Andreas Schwab]

Hans-Peter Nilsson <hp@bitrange.com> writes:

> If the glibc __sec_comment kludge is the problem, let me mention
> that different patches have been sent to glibc in order to make
> its kludge work properly(!) at least one that adds #NO_APP and
> I'm sure would work, but it seems none have been applied.

Unfortunately, m68k and arm are the only gcc targets supported by glibc
that defaults to #NO_APP, all other targets don't have this problem.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Hans-Peter Nilsson]

On Sat, 24 Apr 2004, Andreas Schwab wrote:

> Hans-Peter Nilsson <hp@bitrange.com> writes:
>
> > If the glibc __sec_comment kludge is the problem, let me mention
> > that different patches have been sent to glibc in order to make
> > its kludge work properly(!) at least one that adds #NO_APP and
> > I'm sure would work, but it seems none have been applied.
>
> Unfortunately, m68k and arm are the only gcc targets supported by glibc
> that defaults to #NO_APP, all other targets don't have this problem.

I think you forgot CRIS.  BTW, I'm surprised m68k and ARM has
been changed to start .s files with #NO_APP.

brgds, H-P

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Andreas Schwab]

Hans-Peter Nilsson <hp@bitrange.com> writes:

> I think you forgot CRIS.

CRIS doesn't as of current gcc mainline, and probably never did.  But VAX
does.  The only remaining target is NS32K, which has no glibc port.

> BTW, I'm surprised m68k and ARM has
> been changed to start .s files with #NO_APP.

m68k always did it.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Hans-Peter Nilsson]

On Sat, 24 Apr 2004, Andreas Schwab wrote:
> Hans-Peter Nilsson <hp@bitrange.com> writes:
>
> > I think you forgot CRIS.
>
> CRIS doesn't as of current gcc mainline, and probably never did.

I assume you mean s/gcc/glibc/?.  Oh well.  Mr. Drepper never
committed all the bits I sent for the CRIS port anyway and my
last patches and my suggestion of becoming the maintainer were
ignored.  Let's hope we can communicate better next time around.
Getting off-topic here...

>  But VAX
> does.  The only remaining target is NS32K, which has no glibc port.
>
> > BTW, I'm surprised m68k and ARM has
> > been changed to start .s files with #NO_APP.
>
> m68k always did it.

No, at least for a while it (m68k-linux) didn't.  See for
example the gcc-3.3 branch as of Mon Feb 23 18:58:32 GMT 2004.
It does "now" (last wednesday).  At a glance, it seems to have
been fixed by untangling the config #include mess.  I looked it
up last time we had a #NO_APP issue with binutils, and it didn't
at that time.  ARM (arm-linux) still didn't, main trunk as of
Wed Apr 21 08:31:07 GMT 2004.

Not that any of this is relevant wrt. the actual incorrectness
of emitting comments while in #NO_APP mode.

brgds, H-P

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Andreas Schwab]

Hans-Peter Nilsson <hp@bitrange.com> writes:

> On Sat, 24 Apr 2004, Andreas Schwab wrote:
>> Hans-Peter Nilsson <hp@bitrange.com> writes:
>>
>> > I think you forgot CRIS.
>>
>> CRIS doesn't as of current gcc mainline, and probably never did.
>
> I assume you mean s/gcc/glibc/?.

No.  gcc for cris does not (re-)define TARGET_ASM_FILE_START_APP_OFF, so
the assembler never goes into NO_APP mode.

>> m68k always did it.
>
> No, at least for a while it (m68k-linux) didn't.  See for
> example the gcc-3.3 branch as of Mon Feb 23 18:58:32 GMT 2004.

I can't find any relevant changes in this time frame under config/m68k on
the 3.3 branch.  IIRC m68k-linux never redefined ASM_FILE_START and still
inherits TARGET_ASM_FILE_START_APP_OFF from m68k.h.

> Not that any of this is relevant wrt. the actual incorrectness
> of emitting comments while in #NO_APP mode.

Maybe the #APP/#NO_APP switching should be removed and input scrubbing
enabled all the time, given how few targets actually disable it.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Hans-Peter Nilsson]

On Sat, 24 Apr 2004, Andreas Schwab wrote:
> Hans-Peter Nilsson <hp@bitrange.com> writes:
> > On Sat, 24 Apr 2004, Andreas Schwab wrote:
> >> CRIS doesn't as of current gcc mainline, and probably never did.
> > I assume you mean s/gcc/glibc/?.
>
> No.  gcc for cris does not (re-)define TARGET_ASM_FILE_START_APP_OFF, so
> the assembler never goes into NO_APP mode.

That conclusion is incorrect.  #NO_APP for cris-* is output
through default_file_start.  It seems there are multiple blessed
ways to get that #NO_APP out.  Hmm, I thought that kind of
multiplicity was something Zack W disliked but apparently not. ;-)

> > No, at least for a while it (m68k-linux) didn't.  See for
> > example the gcc-3.3 branch as of Mon Feb 23 18:58:32 GMT 2004.
>
> I can't find any relevant changes in this time frame under config/m68k on
> the 3.3 branch.

I would have expected some #include removals.  You want to look
at config.gcc too.  Maybe the it was all in the ASM_FILE_START
revamp anyway.  I compiled and checked but only guessed the
reason.

>  IIRC m68k-linux never redefined ASM_FILE_START and still
> inherits TARGET_ASM_FILE_START_APP_OFF from m68k.h.

Well, compile a file and see for yourself, given the source
dates I stated.  Note I mentioned the 3.3 branch, where there's
no TARGET_ASM_FILE_START_APP_OFF.

> Maybe the #APP/#NO_APP switching should be removed and input scrubbing
> enabled all the time, given how few targets actually disable it.

See binutils archives from last time this came up (search for
"no_app").  It saves as much as 1% off the compile time.  I
think *more* targets should use it, but most would need to tweak
their md:s to avoid redundant spaces.

brgds, H-P

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Andreas Schwab]

Hans-Peter Nilsson <hp@bitrange.com> writes:

> On Sat, 24 Apr 2004, Andreas Schwab wrote:
>> Hans-Peter Nilsson <hp@bitrange.com> writes:
>> > On Sat, 24 Apr 2004, Andreas Schwab wrote:
>> >> CRIS doesn't as of current gcc mainline, and probably never did.
>> > I assume you mean s/gcc/glibc/?.
>>
>> No.  gcc for cris does not (re-)define TARGET_ASM_FILE_START_APP_OFF, so
>> the assembler never goes into NO_APP mode.
>
> That conclusion is incorrect.  #NO_APP for cris-* is output
> through default_file_start.

You probably mean cris_file_start. Yes, I missed that.

>> > No, at least for a while it (m68k-linux) didn't.  See for
>> > example the gcc-3.3 branch as of Mon Feb 23 18:58:32 GMT 2004.
>>
>> I can't find any relevant changes in this time frame under config/m68k on
>> the 3.3 branch.
>
> I would have expected some #include removals.  You want to look
> at config.gcc too.  Maybe the it was all in the ASM_FILE_START
> revamp anyway.  I compiled and checked but only guessed the
> reason.

I stand corrected.  ASM_FILE_START was redefined in elfos.h, already since
gcc 3.1.

> Note I mentioned the 3.3 branch, where there's
> no TARGET_ASM_FILE_START_APP_OFF.

But it's in 3.4, where it's redefined for all m68k targets.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Zack Weinberg]

Hans-Peter Nilsson <hp@bitrange.com> writes:

> That conclusion is incorrect.  #NO_APP for cris-* is output
> through default_file_start.  It seems there are multiple blessed
> ways to get that #NO_APP out.  Hmm, I thought that kind of
> multiplicity was something Zack W disliked but apparently not. ;-)

I *tried*.  Y'all keep moving the goalposts.  ;-)

>> Maybe the #APP/#NO_APP switching should be removed and input scrubbing
>> enabled all the time, given how few targets actually disable it.
>
> See binutils archives from last time this came up (search for
> "no_app").  It saves as much as 1% off the compile time.  I
> think *more* targets should use it, but most would need to tweak
> their md:s to avoid redundant spaces.

Frankly, (as someone who has to stare at GCC's assembly output all the
damn time), I do not think 1% speedup is worth the *severe*
degradation in readability that this would impose.  I would much
rather put effort into making GAS's parser be faster in
input-scrubbing mode.

zw

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Hans-Peter Nilsson]

On Sat, 24 Apr 2004, Zack Weinberg wrote:
> Hans-Peter Nilsson <hp@bitrange.com> writes:
> > See binutils archives from last time this came up (search for
> > "no_app").  It saves as much as 1% off the compile time.  I
> > think *more* targets should use it, but most would need to tweak
> > their md:s to avoid redundant spaces.
>
> Frankly, (as someone who has to stare at GCC's assembly output all the
> damn time), I do not think 1% speedup is worth the *severe*
> degradation in readability that this would impose.  I would much
> rather put effort into making GAS's parser be faster in
> input-scrubbing mode.

I think you overrate the amount of change needed.  As another
person who has to stare at GCC's assembly output all the (...)
time, my take is that it would (if anything) improve readability
;-) but most problably it's neutral maintainerwise.

All it takes is to change some spacing like "move.d r0, r1" to
move.d r0,r1" and remove spurious (often just extraneous)
spacing in target-defined pseudoopcodes and rarely used
constructs like the trampoline template.

Consider checking cris.md and pointing out what #NO_APP-related
spacing makes it "severely" unreadable!

Not that speeding up GAS parsing would be a bad thing, of
course.

brgds, H-P

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Ian Lance Taylor]

Zack Weinberg <zack@codesourcery.com> writes:

> Frankly, (as someone who has to stare at GCC's assembly output all the
> damn time), I do not think 1% speedup is worth the *severe*
> degradation in readability that this would impose.  I would much
> rather put effort into making GAS's parser be faster in
> input-scrubbing mode.

I've certainly looked at plenty of assembler code, and I personally
don't think there is any degradation in readability whatsoever.  The
only change of any significance is that comments are not permitted.
It's not difficult to generate comments only when some option is
used--such as -fverbose-asm.

Can you give some examples of how pre-scrubbed code is harder to read?

I put a lot of time into speeding up input scrubbing a few years back,
and got some good results, which are now part of gas.  But no matter
what we do it's going to be faster to process code if we don't have to
skip spaces and comments.

Ian

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Zack Weinberg]

Hans-Peter Nilsson <hp@bitrange.com> writes:

> All it takes is to change some spacing like "move.d r0, r1" to
> move.d r0,r1" and remove spurious (often just extraneous)
> spacing in target-defined pseudoopcodes and rarely used
> constructs like the trampoline template.

Well, yeah, see, i386.md was changed from the #NO_APP-compatible
"mov %0,%1" to the current "mov\t%0, %1" in the egcs 1.1 timeframe, if
I recall right, and so we've had a chance to try the same assembly
language both ways, and I vastly prefer it with the tab and the space.

zw

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Zack Weinberg]

Ian Lance Taylor <ian@wasabisystems.com> writes:

> Can you give some examples of how pre-scrubbed code is harder to read?

For me, it really is all about the whitespace.  Here's a fragment of
x86 assembly as currently generated -

main:
        pushl   %ebp
        movl    $1, %eax
        movl    %esp, %ebp
        pushl   %ebx
        subl    $36, %esp
        andl    $-16, %esp
        movl    %eax, 4(%esp)
        movl    $13, (%esp)
        call    signal
        movl    $17, (%esp)
        movl    $1, %eax
        movl    %eax, 4(%esp)
        call    signal

and as it would be if prescrubbed:

main:
        pushl %ebp
        movl $1,%eax
        movl %esp,%ebp
        pushl %ebx
        subl $36,%esp
        andl $-16,%esp
        movl %eax,4(%esp)
        movl $13,(%esp)
        call signal
        movl $17,(%esp)
        movl $1,%eax
        movl %eax,4(%esp)
        call signal

The denser packing, and the lack of a nicely lined up arguments field,
mean substantially more mental effort just to see what it's doing.

zw

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Hans-Peter Nilsson]

On Sun, 25 Apr 2004, Zack Weinberg wrote:

> Hans-Peter Nilsson <hp@bitrange.com> writes:
>
> > All it takes is to change some spacing like "move.d r0, r1" to
> > move.d r0,r1" and remove spurious (often just extraneous)
> > spacing in target-defined pseudoopcodes and rarely used
> > constructs like the trampoline template.
>
> Well, yeah, see, i386.md was changed from the #NO_APP-compatible
> "mov %0,%1" to the current "mov\t%0, %1" in the egcs 1.1 timeframe, if

Right, IIRC I even mentioned i386.md last time. :-)
The i386 backend rewrite (going from CC0) had this
#NO_APP-incompatibility as an extra at no additional charge.

> I recall right, and so we've had a chance to try the same assembly
> language both ways, and I vastly prefer it with the tab and the space.

FWIW, I think you can have the TAB, but not the extra space in
the parameters.

The lack of this extra space (and/or the TAB) is a "*severe*
degradation in readability"?  Hmm.  I believe we've reduced this
issue to a matter of taste.  (Again, methinks -- same conclusion
as last time.)

brgds, H-P
PS  "Prescrubbed" is a misnomer, certainly about the #NO_APP strict format.

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Hans-Peter Nilsson]

On Sun, 25 Apr 2004, Hans-Peter Nilsson wrote:
> > I recall right, and so we've had a chance to try the same assembly
> > language both ways, and I vastly prefer it with the tab and the space.
>
> FWIW, I think you can have the TAB, but not the extra space in
> the parameters.

Nope, sorry for the misinformation.  For the old discussion, see
<URL:http://gcc.gnu.org/ml/gcc-patches/2003-06/msg02565.html>.

(And it's 2% not 1%.)

brgds, H-P

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Ian Lance Taylor]

Zack Weinberg <zack@codesourcery.com> writes:

> The denser packing, and the lack of a nicely lined up arguments field,
> mean substantially more mental effort just to see what it's doing.

I'd say you're making a mountain out of a molehill.  You're condemning
the entire world to have slower compiles because you don't want to
work a little bit harder to read assembler code, nor to write a sed
script to do beautification, nor to use the -fverbose-asm option.  If
there is really a 1% speedup with #NO_APP, then it might even save you
time overall due to faster compiles, even at the cost of spending more
time in the relatively rare cases that you have to look at i386
assembler output.  I'm sure it would save time for everybody else.

For that matter, this patch to gas would accept your example input in
#NO_APP mode.

Ian

Index: tc-i386.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-i386.c,v
retrieving revision 1.150
diff -u -r1.150 tc-i386.c
--- tc-i386.c	12 Mar 2004 10:14:29 -0000	1.150
+++ tc-i386.c	26 Apr 2004 02:05:49 -0000
@@ -237,6 +237,7 @@
 #define is_operand_char(x) (operand_chars[(unsigned char) x])
 #define is_register_char(x) (register_chars[(unsigned char) x])
 #define is_space_char(x) ((x) == ' ')
+#define is_space_or_tab_char(x) ((x) == ' ' || (x) == '\t')
 #define is_identifier_char(x) (identifier_chars[(unsigned char) x])
 #define is_digit_char(x) (digit_chars[(unsigned char) x])
 
@@ -1506,7 +1507,7 @@
 	    }
 	  l++;
 	}
-      if (!is_space_char (*l)
+      if (!is_space_or_tab_char (*l)
 	  && *l != END_OF_INSN
 	  && *l != PREFIX_SEPARATOR
 	  && *l != ',')
@@ -1528,7 +1529,7 @@
       current_templates = hash_find (op_hash, mnemonic);
 
       if (*l != END_OF_INSN
-	  && (!is_space_char (*l) || l[1] != END_OF_INSN)
+	  && (!is_space_or_tab_char (*l) || l[1] != END_OF_INSN)
 	  && current_templates
 	  && (current_templates->start->opcode_modifier & IsPrefix))
 	{
@@ -1673,7 +1674,7 @@
   while (*l != END_OF_INSN)
     {
       /* Skip optional white space before operand.  */
-      if (is_space_char (*l))
+      if (is_space_or_tab_char (*l))
 	++l;
       if (!is_operand_char (*l) && *l != END_OF_INSN)
 	{

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Zack Weinberg]

Ian Lance Taylor <ian@wasabisystems.com> writes:

> Zack Weinberg <zack@codesourcery.com> writes:
>
>> The denser packing, and the lack of a nicely lined up arguments field,
>> mean substantially more mental effort just to see what it's doing.
>
> I'd say you're making a mountain out of a molehill.  You're condemning
> the entire world to have slower compiles because you don't want to
> work a little bit harder to read assembler code, nor to write a sed
> script to do beautification, nor to use the -fverbose-asm option. 

The conversation so far was mainly about the appearance of the
generated assembly, which I agree is minor, but I have another
reason for not liking #NO_APP, which is that it adds a nontrivial
amount of complexity to GCC's assembly output logic.  I would like
if we could make it all just go away.

> For that matter, this patch to gas would accept your example input in
> #NO_APP mode.

This is the sort of thing I was thinking about when I said 'let's
spend time making GAS go faster instead' -- although, if I were in
charge of doing that, I'd try to collapse as much of the parsing logic
together as possible, rather than tweaking all the tc-* parsers.

zw

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Ian Lance Taylor]

Zack Weinberg <zack@codesourcery.com> writes:

> The conversation so far was mainly about the appearance of the
> generated assembly, which I agree is minor, but I have another
> reason for not liking #NO_APP, which is that it adds a nontrivial
> amount of complexity to GCC's assembly output logic.  I would like
> if we could make it all just go away.

It does add complexity where we want -fverbose-asm support.  Other
than that, as far as I can see, it does not.  Is that the nontrivial
complexity that you are talking about?

> > For that matter, this patch to gas would accept your example input in
> > #NO_APP mode.
> 
> This is the sort of thing I was thinking about when I said 'let's
> spend time making GAS go faster instead' -- although, if I were in
> charge of doing that, I'd try to collapse as much of the parsing logic
> together as possible, rather than tweaking all the tc-* parsers.

Well, the assembler parsing logic is already completely collapsed
together.  It is all done in gas/app.c, using a state machine that
operates on buffers.  (It turns out that there are a number of machine
dependencies for assembler parsing, but they are still all handled in
one place.)  The output of do_scrub_chars() in gas/app.c is what the
machine dependent parsers work with.

The idea behind #NO_APP is that the assembler input is precisely that
which is generated by do_scrub_chars().  Thus, in #NO_APP mode,
do_scrub_chars() is not called at all.  do_scrub_chars() will reliably
convert sequences of whitespace to a single space, which is why the
i386 assembler can reliably use is_space_char() to skip whitespace,
and that is why #NO_APP code is expected to use a single space
character.

My patch to config/tc-i386.c was to tweak what the i386 assembler will
accept so that it would take the tab characters you wanted.  I agree
that this approach is not particularly useful.


The general #NO_APP approach makes a great deal of sense to me.  99%
of the time the assembler code generated by the compiler is read only
by the assembler.  Given that, it is already kind of silly that the
compiler has to go to a lot of effort to generate a text file.  The
assembler has to spend time doing parsing and hash table lookups to
recreate information which the compiler already has.

Given that we are going to stick with a text file to communicate
between the compiler and the assembler, it makes sense to me to
eliminate parsing in the assembler as much as possible.  We have text
generated by one program and read by another program; parsing is
useless overhead.  That is the idea behind #NO_APP.

Naturally an even more efficient process would be to link the compiler
and the assembler together into one process, or to use a binary
communication format which does not require parsing.  (The latter is
what the MIPS compiler from MIPS used to do, and perhaps still does.)
However, both of those would require a great deal more engineering
effort than #NO_APP.

Ian

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Andreas Schwab]

Ian Lance Taylor <ian@wasabisystems.com> writes:

> Given that we are going to stick with a text file to communicate
> between the compiler and the assembler, it makes sense to me to
> eliminate parsing in the assembler as much as possible.  We have text
> generated by one program and read by another program; parsing is
> useless overhead.  That is the idea behind #NO_APP.

Currently (gcc 3.4.0) there are 5 targets that actually make use of
#NO_APP: arm, cris, m68k, ns32k and vax (and m68k-linux didn't in gcc
3.0.x upto 3.3.x due to a side effect of including "elfos.h", which
redefines ASM_FILE_START).

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Richard Earnshaw]

On Mon, 2004-04-26 at 15:19, Andreas Schwab wrote:
> Ian Lance Taylor <ian@wasabisystems.com> writes:
> 
> > Given that we are going to stick with a text file to communicate
> > between the compiler and the assembler, it makes sense to me to
> > eliminate parsing in the assembler as much as possible.  We have text
> > generated by one program and read by another program; parsing is
> > useless overhead.  That is the idea behind #NO_APP.
> 
> Currently (gcc 3.4.0) there are 5 targets that actually make use of
> #NO_APP: arm, cris, m68k, ns32k and vax (and m68k-linux didn't in gcc
> 3.0.x upto 3.3.x due to a side effect of including "elfos.h", which
> redefines ASM_FILE_START).

That's probably misleading.  The gcc might emit a directive for that,
but I'm almost certain the ARM assembler for one takes no notice of it.

R.

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Ian Lance Taylor]

Richard Earnshaw <Richard.Earnshaw@arm.com> writes:

> > Currently (gcc 3.4.0) there are 5 targets that actually make use of
> > #NO_APP: arm, cris, m68k, ns32k and vax (and m68k-linux didn't in gcc
> > 3.0.x upto 3.3.x due to a side effect of including "elfos.h", which
> > redefines ASM_FILE_START).
> 
> That's probably misleading.  The gcc might emit a directive for that,
> but I'm almost certain the ARM assembler for one takes no notice of it.

The code to handle #NO_APP in gas is independent of the CPU backend.
The initial #NO_APP comment is recognized in input_file_open(); it
must be the first characters in the file.  It then turns off
preprocessing (i.e., calls to do_scrub_chars) except for code which
follows #APP up to #NO_APP.

That said, I don't see #NO_APP in assembler code generated by an
xscale-elf compiler.  In fact, I see that
TARGET_ASM_FILE_START_APP_OFF is defined to be true, which should
generate #NO_APP, but that ASM_APP_OFF is defined to be "", so the
effect is nullified.  That is, the ARM compiler does not generally use
#NO_APP.

Ian

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Hans-Peter Nilsson]

On Mon, 26 Apr 2004, Ian Lance Taylor wrote:
> Richard Earnshaw <Richard.Earnshaw@arm.com> writes:
>
    Andreas Schwab wrote:
> > > Currently (gcc 3.4.0) there are 5 targets that actually make use of
> > > #NO_APP: arm, cris, m68k, ns32k and vax (and m68k-linux didn't in gcc
> > > 3.0.x upto 3.3.x due to a side effect of including "elfos.h", which
> > > redefines ASM_FILE_START).
> >
> > That's probably misleading.  The gcc might emit a directive for that,
> > but I'm almost certain the ARM assembler for one takes no notice of it.
>
> The code to handle #NO_APP in gas is independent of the CPU backend.
> The initial #NO_APP comment is recognized in input_file_open(); it
> must be the first characters in the file.  It then turns off
> preprocessing (i.e., calls to do_scrub_chars) except for code which
> follows #APP up to #NO_APP.
>
> That said, I don't see #NO_APP in assembler code generated by an
> xscale-elf compiler.

(I already mentioned, but some apparently missed it or
--hopefully not-- disbelieved it, so let me repeat anyway, that)
I checked arm-linux main trunk, confirming that *no* #NO_APP is
generated at the top of the generated assembly files.

I'm reluctant to accept the list above (except for CRIS and
m68k from previous checking) without checking myself. ;-)

brgds, H-P

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Kai Henningsen]

ian@wasabisystems.com (Ian Lance Taylor)  wrote on 26.04.04 in <m3wu42am0e.fsf@gossamer.airs.com>:

> Well, the assembler parsing logic is already completely collapsed
> together.  It is all done in gas/app.c, using a state machine that
> operates on buffers.  (It turns out that there are a number of machine
> dependencies for assembler parsing, but they are still all handled in
> one place.)  The output of do_scrub_chars() in gas/app.c is what the
> machine dependent parsers work with.

Has anyone tried including the effects of do_scrub_chars() into that state  
machine? It certainly sounds as if that ought to be entirely possible  
_and_ fast. Whereas the current two-pass logic sounds rather slow.

MfG Kai

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Ian Lance Taylor]

kaih@khms.westfalen.de (Kai Henningsen) writes:

> ian@wasabisystems.com (Ian Lance Taylor)  wrote on 26.04.04 in <m3wu42am0e.fsf@gossamer.airs.com>:
> 
> > Well, the assembler parsing logic is already completely collapsed
> > together.  It is all done in gas/app.c, using a state machine that
> > operates on buffers.  (It turns out that there are a number of machine
> > dependencies for assembler parsing, but they are still all handled in
> > one place.)  The output of do_scrub_chars() in gas/app.c is what the
> > machine dependent parsers work with.
> 
> Has anyone tried including the effects of do_scrub_chars() into that state  
> machine? It certainly sounds as if that ought to be entirely possible  
> _and_ fast. Whereas the current two-pass logic sounds rather slow.

I guess I was unclear.  do_scrub_chars() is the state machine.

The parsing is indeed two pass.  All the input (other than #NO_APP
code) is first run through the do_scrub_chars() state machine.  The
general parsing code then looks at the parsed input.

Basically the general parsing code in read_a_source_file() requests a
buffer from input_scrub_next_buffer().  That function calls
input_file_give_next_buffer() to read data.  That function calls
either fread() or do_scrub_chars(), depending upon whether #NO_APP is
in effect.  If do_scrub_chars() is called, it calls fread(), scrubs
it, and returns the resulting buffer.  input_scrub_next_buffer() only
returns complete lines.  The general parsing code parses all the
information in the buffer, then requests another buffer.  And so
forth.  This is all made more complex by handling macros and include
files.  The general parsing code then checks for a pseudo-op
(generally, a leading '.' indicates a pseudo-op) and handles
pseudo-ops in a more or less machine independent fashion.  Anything
else is passed to the machine dependent parser.

The code could certainly be cleaned up, as is true of most of gas.
But I don't see anything wrong with the underlying logic.

Ian

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Ian Lance Taylor]

Ian Lance Taylor <ian@wasabisystems.com> writes:

> The code could certainly be cleaned up, as is true of most of gas.
> But I don't see anything wrong with the underlying logic.

Actually, there is one obvious simplification we could apply: the
assembler could always read the entire input file into memory, and
process it that way, rather than reading it buffer by buffer.  That
would simplify and speed up the state machine logic.

However, that would in some cases be a significant change to the
memory requirements of gas.  For example, the i386 insn-attrtab.s file
is 2.7M on my system, which is a lot on some systems.  On the other
hand, the assembler builds other data structures which are
proportional to the size of the input file.  So memory requirements
will not increase by an order of magnitude, or anything like that.  So
it might be acceptable.

Ian

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Zack Weinberg]

Ian Lance Taylor <ian@wasabisystems.com> writes:

> Zack Weinberg <zack@codesourcery.com> writes:
>
>> The conversation so far was mainly about the appearance of the
>> generated assembly, which I agree is minor, but I have another
>> reason for not liking #NO_APP, which is that it adds a nontrivial
>> amount of complexity to GCC's assembly output logic.  I would like
>> if we could make it all just go away.
>
> It does add complexity where we want -fverbose-asm support.  Other
> than that, as far as I can see, it does not.  Is that the nontrivial
> complexity that you are talking about?

I'm talking about the 192 places in the GCC source code where "APP" or
"NO_APP" or other names related to emitting these strings in assembly
appear.  If it were one, I'd not be complaining.  Two or three even,
ok.  192, not cool.

(number got by running "find . -type f | grep -v CVS | grep -v ChangeLog |
xargs egrep '(\<APP\>|\<NO_APP\>|APP_ON|APP_OFF|app_on|app_off)' | wc -l" 
in a CVS checkout - yes, documentation deliberately included).

>> This is the sort of thing I was thinking about when I said 'let's
>> spend time making GAS go faster instead' -- although, if I were in
>> charge of doing that, I'd try to collapse as much of the parsing logic
>> together as possible, rather than tweaking all the tc-* parsers.
>
> Well, the assembler parsing logic is already completely collapsed
> together.  It is all done in gas/app.c, using a state machine that
> operates on buffers.  (It turns out that there are a number of machine
> dependencies for assembler parsing, but they are still all handled in
> one place.)  The output of do_scrub_chars() in gas/app.c is what the
> machine dependent parsers work with.

Uh, when I said "collapse the parsing logic together" I meant "think
of a way to get rid of the machine-dependent parsers to the maximum
extent possible."  While thinking very carefully about how to make the
parser go blazingly fast *without* special tricks.  I'm quite
confident this is possible.  Assembly syntax - with or without extra
added whitespace - is not complicated, nor does it vary that much
across architectures supported by GAS.

zw

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Hans-Peter Nilsson]

On Mon, 26 Apr 2004, Zack Weinberg wrote:
> I'm talking about the 192 places in the GCC source code where "APP" or
> "NO_APP" or other names related to emitting these strings in assembly
> appear.  If it were one, I'd not be complaining.  Two or three even,
> ok.  192, not cool.
>
> (number got by running "find . -type f | grep -v CVS | grep -v ChangeLog |
> xargs egrep '(\<APP\>|\<NO_APP\>|APP_ON|APP_OFF|app_on|app_off)' | wc -l"
> in a CVS checkout - yes, documentation deliberately included).

Seems you're counting occurrences in individual target files as
well.  Besides, IIRC there's redundancy in the machinery, some
of which you added yourself.

If you remove it all from core GCC, we can still have the
#NO_APP emitted in the target's file_start_whateveritsname
function.  I really don't see why there should *necessarily* be
a "nontrivial" overhead.  (Away from my checkout areas at
the moment so I can't produce counterclaims. ;-)

Point is, this functionality, emitting a string at the head of
the assembly file, and avoiding it if flag_verbose_asm, really
doesn't have to be complex...


brgds, H-P
PS.  Replies to Ian's messages be better sent to Ian than me.
Maybe the mailer is playing tricks on me but that'd be a first.

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Alan Modra]

On Mon, Apr 26, 2004 at 05:51:44PM -0700, Zack Weinberg wrote:
> Uh, when I said "collapse the parsing logic together" I meant "think
> of a way to get rid of the machine-dependent parsers to the maximum
> extent possible."  While thinking very carefully about how to make the
> parser go blazingly fast *without* special tricks.  I'm quite
> confident this is possible.  Assembly syntax - with or without extra
> added whitespace - is not complicated, nor does it vary that much
> across architectures supported by GAS.

One thing that complicates do_scrub_chars in gas, for very little gain,
is trying to remove all unneeded whitespace.  For example,

	foo:	mov	%eax, $2 + 3 +   4

is turned into

	foo: mov %eax,$2+3+4

The aim being to render whitespace removal unnecessary in target
dependent code.  That's a good idea until you encounter constructs like:

	foo: addr16 mov %eax,0

where the "opcode" part of the instruction itself has whitespace.
Without various hacks, do_scrub_chars would turn the above into

	foo: addr16 mov%eax,0

Other architectures have similar assembly constructs.  So I think you
underestimate the complexity in a general parser design.  Just
separating an assembly line into label, opcode, operands isn't so easy!

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Zack Weinberg]

Hans-Peter Nilsson <hp@bitrange.com> writes:

> On Mon, 26 Apr 2004, Zack Weinberg wrote:
>> I'm talking about the 192 places in the GCC source code where "APP" or
>> "NO_APP" or other names related to emitting these strings in assembly
>> appear.  If it were one, I'd not be complaining.  Two or three even,
>> ok.  192, not cool.
>>
>> (number got by running "find . -type f | grep -v CVS | grep -v ChangeLog |
>> xargs egrep '(\<APP\>|\<NO_APP\>|APP_ON|APP_OFF|app_on|app_off)' | wc -l"
>> in a CVS checkout - yes, documentation deliberately included).
>
> Seems you're counting occurrences in individual target files as
> well. 

Quite deliberately.  I said I'd not be complaining if this crap
appeared in one place, and I meant one place.  In the entire source
tree.  Which is hyperbole; but I do think the impact on the sources
could be narrowed down quite a bit, along the following lines:

 - completely decouple it from TARGET_ASM_FILE_START. GAS wants to see
   it as the very first thing in the file, and it's supposed to be a
   comment so no one else should care if it e.g. comes ahead of a
   .file directive.  So emit it from init_asm_output before
   targetm.asm_out.file_start.  Actually, fold all of
   default_file_start into init_asm_output.

 - eliminate ASM_APP_ON/OFF entirely; the words "NO_APP" and "APP"
   should be invariant, the comment character should be
   ASM_COMMENT_START (which ought to get target-hook-ized with a
   sensible default, "#" looks good).

 - think of a saner way to do what alpha.h is currently doing with
   ASM_APP_ON/OFF.

 - overhaul final_scan_insn so it's not such a horrible mess.

But I'd rather spend time thinking about how to make GAS faster so it
can all go away.

zw

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Ian Lance Taylor]

Zack Weinberg <zack@codesourcery.com> writes:

> Uh, when I said "collapse the parsing logic together" I meant "think
> of a way to get rid of the machine-dependent parsers to the maximum
> extent possible."  While thinking very carefully about how to make the
> parser go blazingly fast *without* special tricks.  I'm quite
> confident this is possible.  Assembly syntax - with or without extra
> added whitespace - is not complicated, nor does it vary that much
> across architectures supported by GAS.

That turns out to be an oversimplification.  Consider i386 prefixes,
as Alan mentioned.  Consider the d10v, for which the assembler
programmer (or the compiler) may specify packing orderings between two
instructions written on the same line.  Consider m68k operand syntax,
which is sufficiently complex that we wrote a bison parser to handle
it.  Let's not even discuss MRI compatibility mode.  Remember that
while the assembler normally handles compiler output, it must also
handle hand-written assembler code which is much more free in how
operands and expressions are used.  Look at the current complexity of
do_scrub_chars() today, when it doesn't have to worry about many of
the details handled by the machine dependent parsers.

Can these issues be solved?  Yes, of course they can.  Would the
resulting parser be maintainable and blazingly fast?  I very much
doubt that both could be achieved simultaneously.  Even if were
achieved, would it still be faster to use some equivalent of #NO_APP
mode?  Almost certainly.

Ian

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Zack Weinberg]

Alan Modra <amodra@bigpond.net.au> writes:
...
> 	foo: addr16 mov %eax,0
>
> where the "opcode" part of the instruction itself has whitespace.
> Without various hacks, do_scrub_chars would turn the above into
>
> 	foo: addr16 mov%eax,0
>
> Other architectures have similar assembly constructs.  So I think you
> underestimate the complexity in a general parser design.  Just
> separating an assembly line into label, opcode, operands isn't so easy!

Eh, that doesn't look so bad to me, if the set of opcodes and the set
of prefixes is made available to the generic parser.  The basic
grammar is something like

line: label
    | label? WS (prefix WS)* opcode WS operands
    | WS? directive operands

operands: operand
        | operands WS? ',' WS? operand

where the sets of prefixes, opcodes, and directives are known well in
advance - this might be a good application for perfect hashing.
Operand parsing might have to stay machine-specific, but I hope not,
the commonalities are huge.  And then there are additional little bits
of goo that some architectures let in, like IA64 and its punctuation -
I would handle this by treating those as directives, which is why the
above grammar doesn't separate the '.' from the directive.

Macro handling (I mean .macro, not the built-in pseudo-opcodes that
some architectures have) is awkward because you can override opcodes
with macros.  Not sure what to do about that.  

zw

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Ian Lance Taylor]

Zack Weinberg <zack@codesourcery.com> writes:

I already mentioned some difficulties, here are just a few more.

> Eh, that doesn't look so bad to me, if the set of opcodes and the set
> of prefixes is made available to the generic parser.  The basic
> grammar is something like
> 
> line: label
>     | label? WS (prefix WS)* opcode WS operands
>     | WS? directive operands
> 
> operands: operand
>         | operands WS? ',' WS? operand

You left out lines like "foo = 4".  You left out TI label pseudo-op
syntax: "foo: .struct ..." in which the label must be handled
differently.  You left out MIPS .loc which does not use commas between
operands.  You left out operand syntax like ', (single quote followed
by comma) which is supported by some targets but not others.  You left
out the truly dreadful ARM .symver, in which '@' starts a comment at
any time--except when it appears in the operand of .symver.  And,
repeating myself, MRI mode.

> where the sets of prefixes, opcodes, and directives are known well in
> advance - this might be a good application for perfect hashing.

The set of opcodes varies at run time, sometimes significantly as for
PPC or MIPS 32-bit mode vs. 64-bit mode.  However, it would probably
be acceptable to build a perfect hash and handle the results after
lookup.

> I would handle this by treating those as directives, which is why the
> above grammar doesn't separate the '.' from the directive.

A good thing, too, since on m88k, and MRI mode, and some m68k
variants, the '.' is optional.

> Macro handling (I mean .macro, not the built-in pseudo-opcodes that
> some architectures have) is awkward because you can override opcodes
> with macros.  Not sure what to do about that.  

Actually, that is relatively easy.  If any macros have been defined,
you have to look up the directive name in the macro hash table before
you look it up in the other hash table.  Macros don't normally appear
in compiler output, so speed is not as important here.

Again, can these issues be solved?  Yes.  Is this obviously going to
make everything better?  No.

Ian

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Zack Weinberg]

Ian Lance Taylor <ian@wasabisystems.com> writes:

> Zack Weinberg <zack@codesourcery.com> writes:
>
> I already mentioned some difficulties, here are just a few more.
[...]

I'm going to take this as "put up or shut up", and indeed I would not
have blathered on about this for so long if it weren't that I expect
to get a chance to do some performance work on GAS fairly soon (by
which I mean "sometime this year").  I'm going to drop the thread now,
and come back when I have actual code to show people.

zw

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Ian Lance Taylor]

Zack Weinberg <zack@codesourcery.com> writes:

> I'm going to take this as "put up or shut up", and indeed I would not
> have blathered on about this for so long if it weren't that I expect
> to get a chance to do some performance work on GAS fairly soon (by
> which I mean "sometime this year").  I'm going to drop the thread now,
> and come back when I have actual code to show people.

I'm glad that you will have some time to work on this.

For what it's worth, I think the thing which most needs fixing in gas
is the relocation handling, which is completely and horribly broken.
However, that is not a performance issue.

For performance issues, I'll note that assembler relaxation currently
walks the entire frag chain at least three times.  I'm sure a more
clever algorithm is possible.  I wouldn't be surprised if some more
code is quietly walking long lists somewhere.

It might be beneficial to change the hash table to use
libiberty/hashtab.c; I don't know.

The last time I did serious performance hacking on gas, I did the
following.  I uncovered this mostly by using profiling and examing
memory usage.

Ian

http://sources.redhat.com/ml/binutils/1999-q2/msg00108.html

1999-06-03  Ian Lance Taylor  <ian@zembu.com>

	Add support for storing local symbols in a small structure to save
	memory when assembling large files.
	* as.h: Don't include struc-symbol.h.
	(symbolS): Add typedef.
	* symbols.c: Include struc-symbol.h.
	(local_hash): New static variable.
	(save_symbol_name): New static function, from symbol_create.
	(symbol_create): Call save_symbol_name.
	(local_symbol_count): New static variable.
	(local_symbol_conversion_count): Likewise.
	(LOCAL_SYMBOL_CHECK): Define.
	(local_symbol_make): New static function.
	(local_symbol_convert): New static function.
	(colon): Handle local symbols.  Create local symbol for local
	label name.
	(symbol_table_insert): Handle local symbols.
	(symbol_find_or_make): Create local symbol for local label name.
	(symbol_find_base): Check for local symbol.
	(symbol_append, symbol_insert): Check for local symbols.
	(symbol_clear_list_pointers, symbol_remove): Likewise.
	(verify_symbol_chain): Likewise.
	(copy_symbol_attributes): Likewise.
	(resolve_symbol_value): Handle local symbols.
	(resolve_local_symbol): New static function.
	(resolve_local_symbol_values): New function.
	(S_GET_VALUE, S_SET_VALUE): Handle local symbols.
	(S_IS_FUNCTION, S_IS_EXTERNAL, S_IS_WEAK, S_IS_COMMON): Likewise.
	(S_IS_DEFINED, S_IS_DEBUG, S_IS_LOCAL, S_GET_NAME): Likewise.
	(S_GET_SEGMENT, S_SET_SEGMENT, S_SET_EXTERNAL): Likewise.
	(S_CLEAR_EXTERNAL, S_SET_WEAK, S_SET_NAME): Likewise.
	(symbol_previous, symbol_next): New functions.
	(symbol_get_value_expression): Likewise.
	(symbol_set_value_expression): Likewise.
	(symbol_set_frag, symbol_get_frag): Likewise.
	(symbol_mark_used, symbol_clear_used, symbol_used_p): Likewise.
	(symbol_mark_used_in_reloc): Likewise.
	(symbol_clear_used_in_reloc, symbol_used_in_reloc_p): Likewise.
	(symbol_mark_mri_common, symbol_clear_mri_common): Likewise.
	(symbol_mri_common_p): Likewise.
	(symbol_mark_written, symbol_clear_written): Likewise.
	(symbol_written_p): Likewise.
	(symbol_mark_resolved, symbol_resolved_p): Likewise.
	(symbol_section_p, symbol_equated_p): Likewise.
	(symbol_constant_p): Likewise.
	(symbol_get_bfdsym, symbol_set_bfdsym): Likewise.
	(symbol_get_obj, symbol_set_obj): Likewise.
	(symbol_get_tc, symbol_set_tc): Likewise.
	(symbol_begin): Initialize local_hash.
	(print_symbol_value_1): Handle local symbols.
	(symbol_print_statistics): Print local symbol statistics.
	* symbols.h: Include "struc-symbol.h" if not BFD_ASSEMBLER.
	Declare new symbols.c functions.  Move many declarations here from
	struc-symbol.h.
	(SYMBOLS_NEED_BACKPOINTERS): Define if needed.
	* struc-symbol.h (SYMBOLS_NEED_BACKPOINTERS): Don't set.
	(struct symbol): Move bsym to make it clearly the first field.
	Remove TARGET_SYMBOL_FIELDS.
	(symbolS): Don't typedef.
	(struct broken_word): Remove.
	(N_TYPE_seg, seg_N_TYPE): Move to symbol.h.
	(SEGMENT_TO_SYMBOL_TYPE, N_REGISTER): Likewise.
	(symbol_clear_list_pointers): Likewise.
	(symbol_insert, symbol_remove): Likewise.
	(symbol_previous, symbol_append): Likewise.
	(verify_symbol_chain, verify_symbol_chain_2): Likewise.
	(struct local_symbol): Define.
	(local_symbol_converted_p, local_symbol_mark_converted): Define.
	(local_symbol_resolved_p, local_symbol_mark_resolved): Define.
	(local_symbol_get_frag, local_symbol_set_frag): Define.
	(local_symbol_get_real_symbol): Define.
	(local_symbol_set_real_symbol): Define.
	Define.
	* write.c (write_object_file): Call resolve_local_symbol_values.
	* config/obj-ecoff.h (OBJ_SYMFIELD_TYPE): Define.
	(TARGET_SYMBOL_FIELDS): Don't define.
	* config/obj-elf.h (OBJ_SYMFIELD_TYPE): Add local field.  If
	ECOFF_DEBUGGING, add ECOFF fields.
	(ELF_TARGET_SYMBOL_FIELDS, TARGET_SYMBOL_FIELDS): Don't define.
	* config/obj-multi.h (struct elf_obj_sy): Add local field.  If
	ECOFF_DEBUGGING, add ECOFF fields.
	(ELF_TARGET_SYMBOL_FIELDS, TARGET_SYMBOL_FIELDS): Don't define.
	(ECOFF_DEBUG_TARGET_SYMBOL_FIELDS): Don't define.
	* config/tc-mcore.h: Don't include struc-symbol.h.
	(TARGET_SYMBOL_FIELDS): Don't define.
	(struct mcore_tc_sy): Define.
	(TC_SYMFIELD_TYPE): Define.
	* Many files: Use symbolS instead of struct symbol.  Use new
	accessor functions rather than referring to symbolS fields
	directly.

	* read.c (s_mri_common): Don't add in value of line_label.

	* config/tc-mips.c (md_apply_fix): Correct parenthesization when
	checking for SEC_LINK_ONCE.

	* config/tc-sh.h (sh_fix_adjustable): Declare.

	* app.c (input_buffer): New static variable.
	(app_push): Save saved_input in allocated buffer.
	(app_pop): Restored saved_input.
	(do_scrub_chars): Change get parameter to take char * and int as
	arguments.  Change GET macro to pass input_buffer to get
	function.  Don't save input into allocated buffer.
	* as.h (do_scrub_chars): Update declaration.
	* input-file.c (input_file_get): Change to take char * and int.
	Read data into passed in buffer.  Remove static buffer.
	* read.c (scrub_from_string): Change to take char * and int.  Copy
	data into passed in buffer.

	* hash.h: Neaten.  Declare hash_traverse.
	* hash.c: Complete rewrite based on BFD hashing code.
	* gasp.c (chunksize): New variable.
	* macro.c (macro_expand_body): Call hash_jam with NULL rather than
	hash_delete.

Re: demand_empty_rest_of_line and ignore_rest_of_line

[Richard Earnshaw]

On Tue, 2004-04-27 at 04:39, Ian Lance Taylor wrote:
> You left
> out the truly dreadful ARM .symver, in which '@' starts a comment at
> any time--except when it appears in the operand of .symver.  And,
> repeating myself, MRI mode.

I'd dearly like to deprecate use of '@' as a comment char, but it's not
trivial.  There simply is no single char in ASCII (with the possible
exception of '`') that can be used as a substitute and that doesn't have
other uses in the syntax.

One possibility would be to adopt a 2-character sequence (such as '//'),
and to that extent I notice that there is already code in the parser to
handle this.

However, there's still the legacy issue.  Old versions of gcc generate @
in abundance, and there is substantial hand-written assembly code out
there that does similarly.  One way of addressing this might be to
reject '@' in the EABI conforming modes -- it provides a relatively
clean cut-off point.  Can the selection of comment characters be made
command-line option dependent?

R.