Re: The new assembler with unwind table bug fix

[Jim Wilson <wilson at specifixinc dot com>]

On Fri, 2004-01-16 at 14:56, H. J. Lu wrote:
> On Thu, Jan 15, 2004 at 05:09:17PM -0800, David Mosberger wrote:
> >         .proc foo
> >         .prologue
> > foo:    .save rp, r2
> >         nop 0
> >         .align 64
> >         .endp foo
> > The region length should be 15, but instead it comes out as 1.  It
> > seems that .align is handled correctly only inside .body, not inside
> > .prologue, which must work, too.

We can fix this by emitting a dummy unwind record at the end of the
function, for use in calculating region lengths.

However, some of the behavior here is intentional.  There is a comment
                /* In the absence of an explicit .body directive,
                   the prologue ends after the last instruction
                   covered by an unwind directive.  */
So gas is deliberately making prologue regions as small as possible when
there is no explicit body region.  This seems wise from an efficiency
standpoint, but it also seems wrong to do this unless we create an
implicit body region to cover the rest of the function, and we aren't
doing that.

This code comes from a patch that you wrote, see the 2000-05-24
gas/config/tc-ia64.c change.  Unfortunately I don't see it in the
binutils mailing list archives, so it must have been handled on another
mailing list, one of the trillian lists perhaps.

I think I could make this work correctly by putting code in
optimize_unw_records to detect this case and insert the implicit body
directive.  This would re-use some of the code that I am deleting from
fixup_unw_records.  I haven't tried to do this yet.  It isn't clear how
important it is.  This case seems rare.  Advice welcome.

I have attached the patch I have so far.  This gives the results you
want for your testcase, and also modifications of it with .body added,
and/or a nop added after the .align.  This was tested with the binutils
testsuite, there are no regressions.  This was tested with a linux
kernel build, there were no unwind info changes according to readelf -u.

I haven't checked this in yet pending a decision on whether we need to
automatically shorten prologue regions which are not followed by body
regions.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

2004-02-03  James E Wilson  <wilson@specifixinc.com>

	* config/tc-ia64.c (output_endp): New.
	(count_bits): Delete.
	(ia64_flush_insns, process_one_record, optimize_unw_records): Handle
	endp unwind records.
	(fixup_unw_records): Handle endp unwind records.  Delete code for
	shortening prologue regions not followed by a body record.
	(dot_endp): Call add_unwind_entry to emit endp unwind record.
	* config/tc-ia64.h (unw_record_type): Add endp.

Index: tc-ia64.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-ia64.c,v
retrieving revision 1.98
diff -p -r1.98 tc-ia64.c
*** tc-ia64.c	4 Feb 2004 04:40:24 -0000	1.98
--- tc-ia64.c	4 Feb 2004 06:28:54 -0000
*************** static void output_X2_format PARAMS ((vb
*** 822,827 ****
--- 822,828 ----
  static void output_X3_format PARAMS ((vbyte_func, unw_record_type, int, int, int, unsigned long,
  				      unsigned long));
  static void output_X4_format PARAMS ((vbyte_func, int, int, int, int, int, int, unsigned long));
+ static unw_rec_list *output_endp PARAMS ((void));
  static unw_rec_list *output_prologue PARAMS ((void));
  static unw_rec_list *output_prologue_gr PARAMS ((unsigned int, unsigned int));
  static unw_rec_list *output_body PARAMS ((void));
*************** static void process_one_record PARAMS ((
*** 896,902 ****
  static void process_unw_records PARAMS ((unw_rec_list *, vbyte_func));
  static int calc_record_size PARAMS ((unw_rec_list *));
  static void set_imask PARAMS ((unw_rec_list *, unsigned long, unsigned long, unsigned int));
- static int count_bits PARAMS ((unsigned long));
  static unsigned long slot_index PARAMS ((unsigned long, fragS *,
  					 unsigned long, fragS *));
  static unw_rec_list *optimize_unw_records PARAMS ((unw_rec_list *));
--- 897,902 ----
*************** ia64_flush_insns ()
*** 1088,1099 ****
    CURR_SLOT.tag_fixups = 0;
  
    /* In case there are unwind directives following the last instruction,
!      resolve those now.  We only handle body and prologue directives here.
!      Give an error for others.  */
    for (ptr = unwind.current_entry; ptr; ptr = ptr->next)
      {
        if (ptr->r.type == prologue || ptr->r.type == prologue_gr
! 	  || ptr->r.type == body)
  	{
  	  ptr->slot_number = (unsigned long) frag_more (0);
  	  ptr->slot_frag = frag_now;
--- 1088,1099 ----
    CURR_SLOT.tag_fixups = 0;
  
    /* In case there are unwind directives following the last instruction,
!      resolve those now.  We only handle prologue, body, and endp directives
!      here.  Give an error for others.  */
    for (ptr = unwind.current_entry; ptr; ptr = ptr->next)
      {
        if (ptr->r.type == prologue || ptr->r.type == prologue_gr
! 	  || ptr->r.type == body || ptr->r.type == endp)
  	{
  	  ptr->slot_number = (unsigned long) frag_more (0);
  	  ptr->slot_frag = frag_now;
*************** alloc_record (unw_record_type t)
*** 1712,1717 ****
--- 1712,1727 ----
    return ptr;
  }
  
+ /* Dummy unwind record used for calculating the length of the last prologue or
+    body region.  */
+ 
+ static unw_rec_list *
+ output_endp ()
+ {
+   unw_rec_list *ptr = alloc_record (endp);
+   return ptr;
+ }
+ 
  static unw_rec_list *
  output_prologue ()
  {
*************** process_one_record (ptr, f)
*** 2332,2337 ****
--- 2342,2351 ----
  
    switch (ptr->r.type)
      {
+       /* This is a dummy record that takes up no space in the output.  */
+     case endp:
+       break;
+ 
      case gr_mem:
      case fr_mem:
      case br_mem:
*************** set_imask (region, regmask, t, type)
*** 2574,2592 ****
      }
  }
  
- static int
- count_bits (unsigned long mask)
- {
-   int n = 0;
- 
-   while (mask)
-     {
-       mask &= mask - 1;
-       ++n;
-     }
-   return n;
- }
- 
  /* Return the number of instruction slots from FIRST_ADDR to SLOT_ADDR.
     SLOT_FRAG is the frag containing SLOT_ADDR, and FIRST_FRAG is the frag
     containing FIRST_ADDR.  */
--- 2588,2593 ----
*************** optimize_unw_records (list)
*** 2680,2687 ****
    /* If the only unwind record is ".prologue" or ".prologue" followed
       by ".body", then we can optimize the unwind directives away.  */
    if (list->r.type == prologue
!       && (list->next == NULL
! 	  || (list->next->r.type == body && list->next->next == NULL)))
      return NULL;
  
    return list;
--- 2681,2688 ----
    /* If the only unwind record is ".prologue" or ".prologue" followed
       by ".body", then we can optimize the unwind directives away.  */
    if (list->r.type == prologue
!       && (list->next->r.type == endp
! 	  || (list->next->r.type == body && list->next->next->r.type == endp)))
      return NULL;
  
    return list;
*************** fixup_unw_records (list)
*** 2713,2771 ****
  	case body:
  	  {
  	    unw_rec_list *last;
! 	    int size, dir_len = 0;
! 	    unsigned long last_addr;
! 	    fragS *last_frag;
  
  	    first_addr = ptr->slot_number;
  	    first_frag = ptr->slot_frag;
  	    /* Find either the next body/prologue start, or the end of
! 	       the list, and determine the size of the region.  */
! 	    last_addr = list->next_slot_number;
! 	    last_frag = list->next_slot_frag;
  	    for (last = ptr->next; last != NULL; last = last->next)
  	      if (last->r.type == prologue || last->r.type == prologue_gr
! 		  || last->r.type == body)
  		{
  		  last_addr = last->slot_number;
  		  last_frag = last->slot_frag;
  		  break;
  		}
! 	      else if (!last->next)
! 		{
! 		  /* In the absence of an explicit .body directive,
! 		     the prologue ends after the last instruction
! 		     covered by an unwind directive.  */
! 		  if (ptr->r.type != body)
! 		    {
! 		      last_addr = last->slot_number;
! 		      last_frag = last->slot_frag;
! 		      switch (last->r.type)
! 			{
! 			case frgr_mem:
! 			  dir_len = (count_bits (last->r.record.p.frmask)
! 				     + count_bits (last->r.record.p.grmask));
! 			  break;
! 			case fr_mem:
! 			case gr_mem:
! 			  dir_len += count_bits (last->r.record.p.rmask);
! 			  break;
! 			case br_mem:
! 			case br_gr:
! 			  dir_len += count_bits (last->r.record.p.brmask);
! 			  break;
! 			case gr_gr:
! 			  dir_len += count_bits (last->r.record.p.grmask);
! 			  break;
! 			default:
! 			  dir_len = 1;
! 			  break;
! 			}
! 		    }
! 		  break;
! 		}
! 	    size = (slot_index (last_addr, last_frag, first_addr, first_frag)
! 		    + dir_len);
  	    rlen = ptr->r.record.r.rlen = size;
  	    if (ptr->r.type == body)
  	      /* End of region.  */
--- 2714,2736 ----
  	case body:
  	  {
  	    unw_rec_list *last;
! 	    int size;
! 	    unsigned long last_addr = 0;
! 	    fragS *last_frag = NULL;
  
  	    first_addr = ptr->slot_number;
  	    first_frag = ptr->slot_frag;
  	    /* Find either the next body/prologue start, or the end of
! 	       the function, and determine the size of the region.  */
  	    for (last = ptr->next; last != NULL; last = last->next)
  	      if (last->r.type == prologue || last->r.type == prologue_gr
! 		  || last->r.type == body || last->r.type == endp)
  		{
  		  last_addr = last->slot_number;
  		  last_frag = last->slot_frag;
  		  break;
  		}
! 	    size = slot_index (last_addr, last_frag, first_addr, first_frag);
  	    rlen = ptr->r.record.r.rlen = size;
  	    if (ptr->r.type == body)
  	      /* End of region.  */
*************** dot_endp (dummy)
*** 4080,4085 ****
--- 4045,4052 ----
      text_name = "";
  
    insn_group_break (1, 0, 0);
+ 
+   add_unwind_entry (output_endp ());
  
    /* If there wasn't a .handlerdata, we haven't generated an image yet.  */
    if (!unwind.info)
Index: tc-ia64.h
===================================================================
RCS file: /cvs/src/src/gas/config/tc-ia64.h,v
retrieving revision 1.29
diff -p -r1.29 tc-ia64.h
*** tc-ia64.h	7 Jan 2004 19:19:35 -0000	1.29
--- tc-ia64.h	4 Feb 2004 06:28:54 -0000
*************** typedef enum
*** 202,208 ****
    bspstore_gr, bspstore_psprel, bspstore_sprel, rnat_when, rnat_gr,
    rnat_psprel, rnat_sprel, epilogue, label_state, copy_state,
    spill_psprel, spill_sprel, spill_reg, spill_psprel_p, spill_sprel_p,
!   spill_reg_p, unwabi
  } unw_record_type;
  
  /* These structures declare the fields that can be used in each of the
--- 202,208 ----
    bspstore_gr, bspstore_psprel, bspstore_sprel, rnat_when, rnat_gr,
    rnat_psprel, rnat_sprel, epilogue, label_state, copy_state,
    spill_psprel, spill_sprel, spill_reg, spill_psprel_p, spill_sprel_p,
!   spill_reg_p, unwabi, endp
  } unw_record_type;
  
  /* These structures declare the fields that can be used in each of the