as bug (was: Re: smp/up alternatives crash when CONFIG_HOTPLUG_CPU)

as bug (was: Re: smp/up alternatives crash when CONFIG_HOTPLUG_CPU)

[Denis Vlasenko]

[binutils list CC'ed]

On Friday 21 April 2006 10:48, Ingo Molnar wrote:
> you can also try the mutex.bad.s file i attached:
> 
>  $ as mutex.s.bad
>  mutex.s.bad: Assembler messages:
>  mutex.s.bad:267: Warning: .space or .fill with negative value, ignored
>  mutex.s.bad:355: Warning: .space or .fill with negative value, ignored
>  mutex.s.bad:412: Warning: .space or .fill with negative value, ignored
>  mutex.s.bad:574: Warning: .space or .fill with negative value, ignored
>  mutex.s.bad:627: Warning: .space or .fill with negative value, ignored

Reduced testcase, which still exhibits the bug.

# as mutex.bad_minimal.s
mutex.bad_minimal.s: Assembler messages:
mutex.bad_minimal.s:21: Warning: .space or .fill with negative value, ignored
# as --version | head -1
GNU assembler 2.15.91.0.1 20040527
# cat mutex.bad_minimal.s

661:
662:
.section .smp_altinstructions,"a"
  .align 4
  .long 661b
  .byte 0x68
  .byte 662b-661b
.section .smp_altinstr_replacement,"awx"
        .fill 662b-661b,1,0x42
        .section        .sched.text,"ax",@progbits
        call    _spin_unlock    #
661:
2:      jle 2b  #
662:
.section .smp_altinstructions,"a"
  .align 4
  .long 661b
  .byte 0x68
  .byte 662b-661b
.section .smp_altinstr_replacement,"awx"
        .fill 662b-661b,1,0x42

--
vda

Re: as bug (was: Re: smp/up alternatives crash when CONFIG_HOTPLUG_CPU)

[Denis Vlasenko]

On Friday 05 May 2006 11:40, Denis Vlasenko wrote:
> Reduced testcase, which still exhibits the bug.

Smaller one:

.section .smp_altinstr_replacement,"awx"
.section        .sched.text,"ax",@progbits
        call    _spin_unlock    #
661:
2:      jle 2b  #
662:
.section .smp_altinstr_replacement,"awx"
        .fill 662b-661b,1,0x42

--
vda

Re: as bug (was: Re: smp/up alternatives crash when CONFIG_HOTPLUG_CPU)

[Denis Vlasenko]

On Friday 05 May 2006 11:40, Denis Vlasenko wrote:
> Reduced testcase, which still exhibits the bug.

Smaller one:

.section .smp_altinstr_replacement,"awx"
.section        .sched.text,"ax",@progbits
        call    _spin_unlock    #
661:
2:      jle 2b  #
662:
.section .smp_altinstr_replacement,"awx"
        .fill 662b-661b,1,0x42

--
vda

Re: as bug (was: Re: smp/up alternatives crash when CONFIG_HOTPLUG_CPU)

[Alan Modra]

On Fri, May 05, 2006 at 11:45:54AM +0300, Denis Vlasenko wrote:
> 
> .section .smp_altinstr_replacement,"awx"
> .section        .sched.text,"ax",@progbits
>         call    _spin_unlock    #
> 661:
> 2:      jle 2b  #
> 662:
> .section .smp_altinstr_replacement,"awx"
>         .fill 662b-661b,1,0x42

gas should give a better error message here, but really, gas shouldn't
be expected to assemble this.  In essence, you have forward references
in that expression for the .fill length..

Some background:  Gas is a single pass assembler.  It emits code and
data into "frags", buffers containing some fixed number of bytes and
possibly a variable length tail.  The variable length part allows
various features, notably that of variable length instructions.  Symbols
are defined relative to their frags.  Until the frag addresses are
finalized, an expresion involving subtraction of two symbols in
different frags cannot be evaluated correctly.  With the testcase above
you have exactly that situation.  The x86 "jle" instruction can be two
sizes, either 6 bytes or 2 bytes depending on the offset needed, and gas
doesn't have the smarts to recognize that the "jle" above is just 2
bytes.  Instead, it assumes a variable size, putting the "jle" in its
own frag.  This means that label "661" and "662" are in separate frags
with "661" at offset 5 in its frag, and "662" at offset 0.

Since you define the ".smp_altinstr_replacement" section before the
".sched.text section", gas tries to finalize ".smp_altinstr_replacement"
first.  When it tries to calculate the fill size using
   (<base addr "662" frag>+<offset "662">)
    - (<base addr "661" frag>+<offset "661">)
the frag base addresses have not yet been set, and zero is used.  ie.
gas tries to assemble ".fill -5,1,0x42".

A workaround is to ensure that the ".sched.text" section is defined
before ".smp_altinst_replacement".

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: as bug (was: Re: smp/up alternatives crash when CONFIG_HOTPLUG_CPU)

[Alan Modra]

On Fri, May 05, 2006 at 11:45:54AM +0300, Denis Vlasenko wrote:
> 
> .section .smp_altinstr_replacement,"awx"
> .section        .sched.text,"ax",@progbits
>         call    _spin_unlock    #
> 661:
> 2:      jle 2b  #
> 662:
> .section .smp_altinstr_replacement,"awx"
>         .fill 662b-661b,1,0x42

gas should give a better error message here, but really, gas shouldn't
be expected to assemble this.  In essence, you have forward references
in that expression for the .fill length..

Some background:  Gas is a single pass assembler.  It emits code and
data into "frags", buffers containing some fixed number of bytes and
possibly a variable length tail.  The variable length part allows
various features, notably that of variable length instructions.  Symbols
are defined relative to their frags.  Until the frag addresses are
finalized, an expresion involving subtraction of two symbols in
different frags cannot be evaluated correctly.  With the testcase above
you have exactly that situation.  The x86 "jle" instruction can be two
sizes, either 6 bytes or 2 bytes depending on the offset needed, and gas
doesn't have the smarts to recognize that the "jle" above is just 2
bytes.  Instead, it assumes a variable size, putting the "jle" in its
own frag.  This means that label "661" and "662" are in separate frags
with "661" at offset 5 in its frag, and "662" at offset 0.

Since you define the ".smp_altinstr_replacement" section before the
".sched.text section", gas tries to finalize ".smp_altinstr_replacement"
first.  When it tries to calculate the fill size using
   (<base addr "662" frag>+<offset "662">)
    - (<base addr "661" frag>+<offset "661">)
the frag base addresses have not yet been set, and zero is used.  ie.
gas tries to assemble ".fill -5,1,0x42".

A workaround is to ensure that the ".sched.text" section is defined
before ".smp_altinst_replacement".

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: as bug (was: Re: smp/up alternatives crash when CONFIG_HOTPLUG_CPU)

[Denis Vlasenko]

On Friday 05 May 2006 15:20, Alan Modra wrote:
> On Fri, May 05, 2006 at 11:45:54AM +0300, Denis Vlasenko wrote:
> > 
> > .section .smp_altinstr_replacement,"awx"
> > .section        .sched.text,"ax",@progbits
> >         call    _spin_unlock    #
> > 661:
> > 2:      jle 2b  #
> > 662:
> > .section .smp_altinstr_replacement,"awx"
> >         .fill 662b-661b,1,0x42
> 
> gas should give a better error message here, but really, gas shouldn't
> be expected to assemble this.  In essence, you have forward references
> in that expression for the .fill length..

Yes, this should be an error, not warning. It produces miscompiled
object modules.
 
> Some background:  Gas is a single pass assembler.  It emits code and
> data into "frags", buffers containing some fixed number of bytes and
> possibly a variable length tail.  The variable length part allows
> various features, notably that of variable length instructions.  Symbols
> are defined relative to their frags.  Until the frag addresses are
> finalized, an expresion involving subtraction of two symbols in
> different frags cannot be evaluated correctly.  With the testcase above
> you have exactly that situation.  The x86 "jle" instruction can be two
> sizes, either 6 bytes or 2 bytes depending on the offset needed, and gas
> doesn't have the smarts to recognize that the "jle" above is just 2
> bytes.  Instead, it assumes a variable size, putting the "jle" in its
> own frag.  This means that label "661" and "662" are in separate frags
> with "661" at offset 5 in its frag, and "662" at offset 0.
> 
> Since you define the ".smp_altinstr_replacement" section before the
> ".sched.text section", gas tries to finalize ".smp_altinstr_replacement"
> first.  When it tries to calculate the fill size using
>    (<base addr "662" frag>+<offset "662">)
>     - (<base addr "661" frag>+<offset "661">)
> the frag base addresses have not yet been set, and zero is used.  ie.
> gas tries to assemble ".fill -5,1,0x42".

Thanks for the explanation.
--
vda

Re: as bug (was: Re: smp/up alternatives crash when CONFIG_HOTPLUG_CPU)

[Denis Vlasenko]

On Friday 05 May 2006 15:20, Alan Modra wrote:
> On Fri, May 05, 2006 at 11:45:54AM +0300, Denis Vlasenko wrote:
> > 
> > .section .smp_altinstr_replacement,"awx"
> > .section        .sched.text,"ax",@progbits
> >         call    _spin_unlock    #
> > 661:
> > 2:      jle 2b  #
> > 662:
> > .section .smp_altinstr_replacement,"awx"
> >         .fill 662b-661b,1,0x42
> 
> gas should give a better error message here, but really, gas shouldn't
> be expected to assemble this.  In essence, you have forward references
> in that expression for the .fill length..

Yes, this should be an error, not warning. It produces miscompiled
object modules.
 
> Some background:  Gas is a single pass assembler.  It emits code and
> data into "frags", buffers containing some fixed number of bytes and
> possibly a variable length tail.  The variable length part allows
> various features, notably that of variable length instructions.  Symbols
> are defined relative to their frags.  Until the frag addresses are
> finalized, an expresion involving subtraction of two symbols in
> different frags cannot be evaluated correctly.  With the testcase above
> you have exactly that situation.  The x86 "jle" instruction can be two
> sizes, either 6 bytes or 2 bytes depending on the offset needed, and gas
> doesn't have the smarts to recognize that the "jle" above is just 2
> bytes.  Instead, it assumes a variable size, putting the "jle" in its
> own frag.  This means that label "661" and "662" are in separate frags
> with "661" at offset 5 in its frag, and "662" at offset 0.
> 
> Since you define the ".smp_altinstr_replacement" section before the
> ".sched.text section", gas tries to finalize ".smp_altinstr_replacement"
> first.  When it tries to calculate the fill size using
>    (<base addr "662" frag>+<offset "662">)
>     - (<base addr "661" frag>+<offset "661">)
> the frag base addresses have not yet been set, and zero is used.  ie.
> gas tries to assemble ".fill -5,1,0x42".

Thanks for the explanation.
--
vda

Re: as bug (was: Re: smp/up alternatives crash when CONFIG_HOTPLUG_CPU)

[Alan Modra]

On Fri, May 05, 2006 at 04:13:24PM +0300, Denis Vlasenko wrote:
> On Friday 05 May 2006 15:20, Alan Modra wrote:
> > the frag base addresses have not yet been set, and zero is used.  ie.
> > gas tries to assemble ".fill -5,1,0x42".

The fact that enabling gas listings fixes this has been nagging at me
since writing the sketchy description of gas frags and relaxation.
I'd forgotten that relaxation keeps iterating over all sections until no
frag changes address.  ie. even though the first .fill is using invalid
addresses, there will be a subsequent pass that uses the correct value.

The reason why gas -al helps with this case is that gas creates a new
frag for each line as somewhere to hang the file/line number info.  So
both "661" and "662" start off at offset zero in their frags and the
initial pass .fill has a zero length rather than a negative one.

So perhaps gas ought to be able to handle this after all..  I'll see if
I can come up with a fix.

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: as bug (was: Re: smp/up alternatives crash when CONFIG_HOTPLUG_CPU)

[Alan Modra]

On Sat, May 06, 2006 at 12:41:41PM +0930, Alan Modra wrote:
> The reason why gas -al helps with this case is that gas creates a new
> frag for each line as somewhere to hang the file/line number info.  So
> both "661" and "662" start off at offset zero in their frags and the
> initial pass .fill has a zero length rather than a negative one.
> 
> So perhaps gas ought to be able to handle this after all..

Like this.

	* write.c (relax_segment): Add pass count arg.  Don't error on
	negative org/space on first two passes.
	(relax_seg_info): New struct.
	(relax_seg, write_object_file): Adjust.
	* write.h (relax_segment): Update prototype.

Index: gas/write.c
===================================================================
RCS file: /cvs/src/src/gas/write.c,v
retrieving revision 1.100
diff -u -p -r1.100 write.c
--- gas/write.c	3 May 2006 23:52:15 -0000	1.100
+++ gas/write.c	6 May 2006 06:44:19 -0000
@@ -512,19 +512,21 @@ cvt_frag_to_fill (segT sec ATTRIBUTE_UNU
 #endif
 }
 
-static void relax_seg (bfd *, asection *, PTR);
+struct relax_seg_info
+{
+  int pass;
+  int changed;
+};
 
 static void
-relax_seg (bfd *abfd ATTRIBUTE_UNUSED, asection *sec, PTR xxx)
+relax_seg (bfd *abfd ATTRIBUTE_UNUSED, asection *sec, void *xxx)
 {
   segment_info_type *seginfo = seg_info (sec);
+  struct relax_seg_info *info = (struct relax_seg_info *) xxx;
 
   if (seginfo && seginfo->frchainP
-      && relax_segment (seginfo->frchainP->frch_root, sec))
-    {
-      int *result = (int *) xxx;
-      *result = 1;
-    }
+      && relax_segment (seginfo->frchainP->frch_root, sec, info->pass))
+    info->changed = 1;
 }
 
 static void size_seg (bfd *, asection *, PTR);
@@ -1206,6 +1208,7 @@ subsegs_finish (void)
 void
 write_object_file (void)
 {
+  struct relax_seg_info rsi;
 #ifndef WORKING_DOT_WORD
   fragS *fragP;			/* Track along all frags.  */
 #endif
@@ -1264,10 +1267,9 @@ write_object_file (void)
       merge_data_into_text ();
     }
 
+  rsi.pass = 0;
   while (1)
     {
-      int changed;
-
 #ifndef WORKING_DOT_WORD
       /* We need to reset the markers in the broken word list and
 	 associated frags between calls to relax_segment (via
@@ -1288,9 +1290,10 @@ write_object_file (void)
 	}
 #endif
 
-      changed = 0;
-      bfd_map_over_sections (stdoutput, relax_seg, &changed);
-      if (!changed)
+      rsi.changed = 0;
+      bfd_map_over_sections (stdoutput, relax_seg, &rsi);
+      rsi.pass++;
+      if (!rsi.changed)
 	break;
     }
 
@@ -1721,7 +1724,7 @@ relax_align (register relax_addressT add
    addresses.  */
 
 int
-relax_segment (struct frag *segment_frag_root, segT segment)
+relax_segment (struct frag *segment_frag_root, segT segment, int pass)
 {
   unsigned long frag_count;
   struct frag *fragP;
@@ -1835,6 +1838,7 @@ relax_segment (struct frag *segment_frag
     if (max_iterations < frag_count)
       max_iterations = frag_count;
 
+    ret = 0;
     do
       {
 	stretch = 0;
@@ -1964,6 +1968,16 @@ relax_segment (struct frag *segment_frag
 		  growth = target - after;
 		  if (growth < 0)
 		    {
+		      growth = 0;
+
+		      /* Don't error on first few frag relax passes.  */
+		      if (pass < 2)
+			{
+			  /* Force another pass.  */
+			  ret = 1;
+			  break;
+			}
+
 		      /* Growth may be negative, but variable part of frag
 			 cannot have fewer than 0 chars.  That is, we can't
 			 .org backwards.  */
@@ -1976,7 +1990,7 @@ relax_segment (struct frag *segment_frag
 		      fragP->fr_subtype = 0;
 		      fragP->fr_offset = 0;
 		      fragP->fr_fix = after - was_address;
-		      growth = stretch;
+		      break;
 		    }
 
 		  /* This is an absolute growth factor  */
@@ -2002,6 +2016,14 @@ relax_segment (struct frag *segment_frag
 		      }
 		    else if (amount < 0)
 		      {
+			/* Don't error on first few frag relax passes.  */
+			if (pass < 2)
+			  {
+			    /* Force another pass.  */
+			    ret = 1;
+			    break;
+			  }
+
 			as_warn_where (fragP->fr_file, fragP->fr_line,
 				       _(".space or .fill with negative value, ignored"));
 			fragP->fr_symbol = 0;
@@ -2063,7 +2085,6 @@ relax_segment (struct frag *segment_frag
 		segment_name (segment));
   }
 
-  ret = 0;
   for (fragP = segment_frag_root; fragP; fragP = fragP->fr_next)
     if (fragP->last_fr_address != fragP->fr_address)
       {
Index: gas/write.h
===================================================================
RCS file: /cvs/src/src/gas/write.h,v
retrieving revision 1.14
diff -u -p -r1.14 write.h
--- gas/write.h	11 Aug 2005 01:25:20 -0000	1.14
+++ gas/write.h	6 May 2006 06:44:19 -0000
@@ -1,6 +1,6 @@
 /* write.h
    Copyright 1987, 1992, 1993, 1994, 1995, 1996, 1997, 1999, 2000, 2001,
-   2002, 2003, 2005 Free Software Foundation, Inc.
+   2002, 2003, 2005, 2006 Free Software Foundation, Inc.
 
    This file is part of GAS, the GNU Assembler.
 
@@ -156,7 +156,7 @@ extern int get_recorded_alignment (segT 
 extern void subsegs_finish (void);
 extern void write_object_file (void);
 extern long relax_frag (segT, fragS *, long);
-extern int relax_segment (struct frag * seg_frag_root, segT seg_type);
+extern int relax_segment (struct frag *, segT, int);
 extern void number_to_chars_littleendian (char *, valueT, int);
 extern void number_to_chars_bigendian (char *, valueT, int);
 extern fixS *fix_new


-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: as bug

[Nick Clifton]

Hi Alan,

> +		      /* Don't error on first few frag relax passes.  */
> +		      if (pass < 2)
> +			{
> +			  /* Force another pass.  */
> +			  ret = 1;
> +			  break;
> +			}

I think that it might help if you include a comment saying *why* the 
first two passes should ignore a negative growth value.

Cheers
   Nick

Re: as bug

[Alan Modra]

On Sun, May 07, 2006 at 03:48:25PM +0100, Nick Clifton wrote:
> Hi Alan,
> 
> >+		      /* Don't error on first few frag relax passes.  */
> >+		      if (pass < 2)
> >+			{
> >+			  /* Force another pass.  */
> >+			  ret = 1;
> >+			  break;
> >+			}
> 
> I think that it might help if you include a comment saying *why* the 
> first two passes should ignore a negative growth value.

As it happened, my commit failed due to gas/ChangeLog not being
up-to-date.  Committing with this comment.

+		      /* Don't error on first few frag relax passes.
+			 The symbol might be an expression involving
+			 symbol values from other sections.  If those
+			 sections have not yet been processed their
+			 frags will all have zero addresses, so we
+			 will calculate incorrect values for them.  The
+			 number of passes we allow before giving an
+			 error is somewhat arbitrary.  It should be at
+			 least one, with larger values requiring
+			 increasingly contrived dependencies between
+			 frags to trigger a false error.  */

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre