[RFA] dwarf2cfi.c improvements

[RFA] dwarf2cfi.c improvements

[Michal Ludvig]

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

Hi all,
this (quite long) patch addresses following issues in dwarf2cfi.c:
1) Read both .debug_frame and .eh_frame if they are present. Not all 
frames must be covered in both frames. Example: imagine an exec linked 
from file1.o that was compiled with -g and file2.o that was compiled 
without it. Than old gdb would parse only a .debug_frame that came from 
file1.o and wouldn't have any frame info for code from file2.o that 
provided only the .eh_frame. That's the reason why to parse both of them.
2) Of course we shouldn't have duplicate FDEs (one from .debug_frame and 
one from .eh_frame). If the file contains only an .eh_frame it's easy, 
there can't be duplicates, but if we parse both sections, we must check 
fde_chunks before adding new FDE from eh_frame. (see comments inside the 
patch).
3) The original dwarf2cfi.c didn't implement CIE Augmentation correctly. 
The main problem was a non-standard addressing (aug. 'R') that I had to 
rewrite completely. This fixes many problems with backtraces through 
relocatable (-fPIC compiled) objects without .debug_info.
4) parse_frame_info now also recognises aug. 'L' (= LSDA pointer present 
in FDE) but I was told I don't need LSDA in GDB, so I ignore its content.

2002-05-31  Michal Ludvig  <mludvig@suse.cz>

         * dwarf2cfi.c (dwarf_frame_buffer): Delete.
         (read_encoded_pointer): Add new argument,
         warn on indirect pointers.
         (execute_cfa_program): Warn on relative addressing,
         change messages.
         (frame_state_for, cfi_get_saved_register)
         (cfi_virtual_frame_pointer): Change message.
         (dwarf2_build_frame_info): Only call parse_frame_info
         for .debug_frame and .eh_frame.
         (parse_frame_info): New, derived from dwarf2_build_frame_info,
         fixed augmentation handling, added relative addressing,
         ignore duplicate FDEs. Added comments.

OK to commit?

Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* +420 2 9654 5373   * http://www.suse.cz

[-- Attachment #2: cfi-huge3.diff --]
[-- Type: text/plain, Size: 18679 bytes --]

Index: dwarf2cfi.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2cfi.c,v
retrieving revision 1.7
diff -c -3 -p -r1.7 dwarf2cfi.c
*** dwarf2cfi.c	22 May 2002 13:21:19 -0000	1.7
--- dwarf2cfi.c	31 May 2002 15:15:18 -0000
*************** extern file_ptr dwarf_frame_offset;
*** 188,195 ****
  extern unsigned int dwarf_frame_size;
  extern file_ptr dwarf_eh_frame_offset;
  extern unsigned int dwarf_eh_frame_size;
- 
- static char *dwarf_frame_buffer;
  \f
  
  extern char *dwarf2_read_section (struct objfile *objfile, file_ptr offset,
--- 188,193 ----
*************** static ULONGEST read_uleb128 (bfd *abfd,
*** 217,223 ****
  static LONGEST read_sleb128 (bfd *abfd, char **p);
  static CORE_ADDR read_pointer (bfd *abfd, char **p);
  static CORE_ADDR read_encoded_pointer (bfd *abfd, char **p,
! 				       unsigned char encoding);
  
  static LONGEST read_initial_length (bfd *abfd, char *buf, int *bytes_read);
  static ULONGEST read_length (bfd *abfd, char *buf, int *bytes_read,
--- 215,221 ----
  static LONGEST read_sleb128 (bfd *abfd, char **p);
  static CORE_ADDR read_pointer (bfd *abfd, char **p);
  static CORE_ADDR read_encoded_pointer (bfd *abfd, char **p,
! 			unsigned char encoding, int *flag_pcrel);
  
  static LONGEST read_initial_length (bfd *abfd, char *buf, int *bytes_read);
  static ULONGEST read_length (bfd *abfd, char *buf, int *bytes_read,
*************** read_pointer (bfd *abfd, char **p)
*** 487,496 ****
  }
  
  static CORE_ADDR
! read_encoded_pointer (bfd *abfd, char **p, unsigned char encoding)
  {
    CORE_ADDR ret;
! 
    switch (encoding & 0x0f)
      {
      case DW_EH_PE_absptr:
--- 485,497 ----
  }
  
  static CORE_ADDR
! read_encoded_pointer (bfd *abfd, char **p, unsigned char encoding, 
! 	int *flag_pcrel)
  {
    CORE_ADDR ret;
!   
!   if (flag_pcrel) *flag_pcrel = 0;
!   
    switch (encoding & 0x0f)
      {
      case DW_EH_PE_absptr:
*************** read_encoded_pointer (bfd *abfd, char **
*** 526,549 ****
  
      default:
        internal_error (__FILE__, __LINE__,
! 		      "read_encoded_pointer: unknown pointer encoding");
      }
  
    if (ret != 0)
!     switch (encoding & 0xf0)
        {
        case DW_EH_PE_absptr:
  	break;
        case DW_EH_PE_pcrel:
! 	ret += (CORE_ADDR) *p;
  	break;
        case DW_EH_PE_textrel:
        case DW_EH_PE_datarel:
        case DW_EH_PE_funcrel:
        default:
  	internal_error (__FILE__, __LINE__,
! 			"read_encoded_pointer: unknown pointer encoding");
        }
  
    return ret;
  }
--- 527,560 ----
  
      default:
        internal_error (__FILE__, __LINE__,
! 		      "%s: unknown pointer encoding: 0x%x",
! 		      __func__, encoding);
      }
  
    if (ret != 0)
!   {
!     switch (encoding & 0x70)
        {
        case DW_EH_PE_absptr:
  	break;
        case DW_EH_PE_pcrel:
! 	if (flag_pcrel) *flag_pcrel = 1;
  	break;
        case DW_EH_PE_textrel:
        case DW_EH_PE_datarel:
        case DW_EH_PE_funcrel:
        default:
  	internal_error (__FILE__, __LINE__,
! 			"%s: unknown pointer encoding: 0x%x",
! 			__func__, encoding);
        }
+     
+     if (encoding & DW_EH_PE_indirect)
+     {
+       warning ("%s: Unsupported pointer encoding: DW_EH_PE_indirect", 
+       	__func__);
+     }
+   }
  
    return ret;
  }
*************** execute_cfa_program ( struct objfile *ob
*** 597,602 ****
--- 608,614 ----
        unsigned char insn = *insn_ptr++;
        ULONGEST reg, uoffset;
        LONGEST offset;
+       int flag_pcrel;
  
        if (insn & DW_CFA_advance_loc)
  	fs->pc += (insn & 0x3f) * fs->code_align;
*************** execute_cfa_program ( struct objfile *ob
*** 618,624 ****
  	  {
  	  case DW_CFA_set_loc:
  	    fs->pc = read_encoded_pointer (objfile->obfd, &insn_ptr,
! 					   fs->addr_encoding);
  	    break;
  
  	  case DW_CFA_advance_loc1:
--- 630,642 ----
  	  {
  	  case DW_CFA_set_loc:
  	    fs->pc = read_encoded_pointer (objfile->obfd, &insn_ptr,
! 					   fs->addr_encoding, &flag_pcrel);
! 	    if (flag_pcrel)
! 	    {
! 	      warning ("%s: DW_CFA_set_loc uses relative addressing at pc=%p", 
! 	      	__func__, (void*)fs->pc);
! 	      fs->pc += (CORE_ADDR)insn_ptr;
! 	    }
  	    break;
  
  	  case DW_CFA_advance_loc1:
*************** execute_cfa_program ( struct objfile *ob
*** 766,772 ****
  	    break;
  
  	  default:
! 	    error ("dwarf cfi error: unknown cfa instruction %d.", insn);
  	  }
      }
  }
--- 784,795 ----
  	    break;
  
  	  default:
! 	    if(insn >= DW_CFA_lo_user && insn <= DW_CFA_hi_user)
! 		    warning("CFI: Unknown user CFA instruction 0x%x "
! 		    	"for frame %p", insn, (void*)fs->pc);
! 	    else
! 		    warning("CFI: Unknown CFA instruction 0x%x "
! 		    	"for frame %p", insn, (void*)fs->pc);
  	  }
      }
  }
*************** frame_state_for (struct context *context
*** 826,832 ****
    }
    else
  	internal_error (__FILE__, __LINE__,
! 		"%s(): Internal error: fde->cie_ptr==NULL !", 
  		__func__);
  }
  
--- 849,855 ----
    }
    else
  	internal_error (__FILE__, __LINE__,
! 		"%s: Internal error: fde->cie_ptr==NULL !", 
  		__func__);
  }
  
*************** compare_fde_unit (const void *a, const v
*** 1372,1409 ****
  
  /*  Build the cie_chunks and fde_chunks tables from informations
      in .debug_frame section.  */
! void
! dwarf2_build_frame_info (struct objfile *objfile)
  {
    bfd *abfd = objfile->obfd;
!   char *start = NULL;
!   char *end = NULL;
!   int from_eh = 0;
  
    obstack_init (&unwind_tmp_obstack);
  
!   dwarf_frame_buffer = 0;
  
!   if (dwarf_frame_offset)
!     {
!       dwarf_frame_buffer = dwarf2_read_section (objfile,
! 						dwarf_frame_offset,
! 						dwarf_frame_size);
! 
!       start = dwarf_frame_buffer;
!       end = dwarf_frame_buffer + dwarf_frame_size;
!     }
!   else if (dwarf_eh_frame_offset)
!     {
!       dwarf_frame_buffer = dwarf2_read_section (objfile,
! 						dwarf_eh_frame_offset,
! 						dwarf_eh_frame_size);
! 
!       start = dwarf_frame_buffer;
!       end = dwarf_frame_buffer + dwarf_eh_frame_size;
! 
!       from_eh = 1;
!     }
  
    if (start)
      {
--- 1395,1423 ----
  
  /*  Build the cie_chunks and fde_chunks tables from informations
      in .debug_frame section.  */
! static void
! parse_frame_info (struct objfile *objfile, file_ptr frame_offset, 
! 	unsigned int frame_size, int eh_frame)
  {
    bfd *abfd = objfile->obfd;
!   asection *curr_section_ptr;
!   char *start = NULL, *end = NULL, *frame_buffer = NULL;
!   char *curr_section_name, *aug_data;
!   struct cie_unit *last_cie=NULL;
!   int last_dup_fde = 0, aug_len, i;
!   CORE_ADDR curr_section_vma = 0;
  
    obstack_init (&unwind_tmp_obstack);
  
!   frame_buffer = dwarf2_read_section (objfile, frame_offset, frame_size);
  
!   start = frame_buffer;
!   end = frame_buffer + frame_size;
!   
!   curr_section_name = eh_frame?".eh_frame":".debug_frame";
!   curr_section_ptr = bfd_get_section_by_name (abfd, curr_section_name);
!   if (curr_section_ptr)
!     curr_section_vma = curr_section_ptr->vma;
  
    if (start)
      {
*************** dwarf2_build_frame_info (struct objfile 
*** 1411,1419 ****
  	{
  	  unsigned long length;
  	  ULONGEST cie_id;
! 	  ULONGEST unit_offset = start - dwarf_frame_buffer;
! 	  int bytes_read;
! 	  int dwarf64;
  	  char *block_end;
  
  	  length = read_initial_length (abfd, start, &bytes_read);
--- 1425,1432 ----
  	{
  	  unsigned long length;
  	  ULONGEST cie_id;
! 	  ULONGEST unit_offset = start - frame_buffer;
! 	  int bytes_read, dwarf64, flag_pcrel;
  	  char *block_end;
  
  	  length = read_initial_length (abfd, start, &bytes_read);
*************** dwarf2_build_frame_info (struct objfile 
*** 1424,1430 ****
  	  cie_id = read_length (abfd, start, &bytes_read, dwarf64);
  	  start += bytes_read;
  
! 	  if ((from_eh && cie_id == 0) || is_cie (cie_id, dwarf64))
  	    {
  	      struct cie_unit *cie = cie_unit_alloc ();
  	      char *aug;
--- 1437,1443 ----
  	  cie_id = read_length (abfd, start, &bytes_read, dwarf64);
  	  start += bytes_read;
  
! 	  if ((eh_frame && cie_id == 0) || is_cie (cie_id, dwarf64))
  	    {
  	      struct cie_unit *cie = cie_unit_alloc ();
  	      char *aug;
*************** dwarf2_build_frame_info (struct objfile 
*** 1440,1523 ****
  	      start++;		/* version */
  
  	      cie->augmentation = aug = start;
! 	      while (*start)
! 		start++;
! 	      start++;		/* skip past NUL */
  
  	      cie->code_align = read_uleb128 (abfd, &start);
  	      cie->data_align = read_sleb128 (abfd, &start);
  	      cie->ra = read_1u (abfd, &start);
  
  	      if (*aug == 'z')
  		{
! 		  int xtra = read_uleb128 (abfd, &start);
! 		  start += xtra;
  		  ++aug;
  		}
  
  	      while (*aug != '\0')
  		{
  		  if (aug[0] == 'e' && aug[1] == 'h')
  		    {
! 		      start += sizeof (void *);
! 		      aug += 2;
  		    }
  		  else if (aug[0] == 'R')
  		    {
! 		      cie->addr_encoding = *start++;
! 		      aug += 1;
  		    }
! 		  else if (aug[0] == 'P')
  		    {
! 		      CORE_ADDR ptr;
! 		      ptr = read_encoded_pointer (abfd, &start,
! 						  cie->addr_encoding);
! 		      aug += 1;
  		    }
  		  else
! 		    warning ("%s(): unknown augmentation", __func__);
  		}
  
! 	      cie->data = start;
! 	      cie->data_length = block_end - start;
  	    }
  	  else
  	    {
  	      struct fde_unit *fde;
  	      struct cie_unit *cie;
  
! 	      fde_chunks_need_space ();
! 	      fde = fde_unit_alloc ();
! 
! 	      fde_chunks.array[fde_chunks.elems++] = fde;
! 	      
! 	      fde->initial_location = read_pointer (abfd, &start)
! 	        + ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
! 	      fde->address_range = read_pointer (abfd, &start);
! 
! 	      cie = cie_chunks;
! 	      while(cie)
  	      {
! 	        if (cie->objfile == objfile)
  		{
! 		  if (from_eh && (cie->offset == (unit_offset + bytes_read - cie_id)))
! 		      break;
! 		  if (!from_eh && (cie->offset == cie_id))
! 		    break;
  		}
  
! 		cie = cie->next;
  	      }
- 	    
- 	      if (!cie)
- 		error ("%s(): can't find CIE pointer", __func__);
- 	      fde->cie_ptr = cie;
  
! 	      if (cie->augmentation[0] == 'z')
! 		  read_uleb128 (abfd, &start);
  
! 	      fde->data = start;
! 	      fde->data_length = block_end - start;
  	    }
  	  start = block_end;
  	}
--- 1453,1630 ----
  	      start++;		/* version */
  
  	      cie->augmentation = aug = start;
! 	      while (*start++);	/* Skips last NULL as well */
  
  	      cie->code_align = read_uleb128 (abfd, &start);
  	      cie->data_align = read_sleb128 (abfd, &start);
  	      cie->ra = read_1u (abfd, &start);
  
+ 	      /* Augmentation:
+ 		 z	Indicates that a uleb128 is present to size the
+ 		 	augmentation section.
+ 		 L	Indicates the encoding (and thus presence) of
+ 			an LSDA pointer in the FDE augmentation.
+ 		 R	Indicates a non-default pointer encoding for
+ 			FDE code pointers.
+ 		 P	Indicates the presence of an encoding + language
+ 			personality routine in the CIE augmentation.
+ 	      
+ 	         [This info comes from GCC's dwarf2out.c]
+ 		*/
  	      if (*aug == 'z')
  		{
! 		  aug_len = read_uleb128 (abfd, &start);
! 		  aug_data = start;
! 		  start += aug_len;
  		  ++aug;
  		}
  
+ 	      cie->data = start;
+ 	      cie->data_length = block_end - cie->data;
+ 	      
  	      while (*aug != '\0')
  		{
  		  if (aug[0] == 'e' && aug[1] == 'h')
  		    {
! 		      aug_data += sizeof (void *);
! 		      aug++;
  		    }
  		  else if (aug[0] == 'R')
+ 		      cie->addr_encoding = *aug_data++;
+ 		  else if (aug[0] == 'P')
  		    {
! 		      CORE_ADDR pers_addr;
! 		      int pers_addr_enc;
! 		      
! 		      pers_addr_enc = *aug_data++;
! 		      /* We use (pers_addr_enc & 0x7f) to avoid 
! 		         warning about indirect relative addressing.
! 			 We don't need pers_addr value anyway and so we 
! 			 don't care if it is correct :-)  */
! 		      pers_addr = read_encoded_pointer (abfd, &aug_data,
! 					pers_addr_enc & 0x7f, NULL);
  		    }
! 		  else if (aug[0] == 'L' && eh_frame)
  		    {
! 		      int lsda_addr_enc;
! 
! 		      /* Perhaps we should save this to CIE for later use?
! 		         Do we need it for something in GDB?  */
! 		      lsda_addr_enc = *aug_data++;
  		    }
  		  else
! 		    warning ("CFI warning: unknown augmentation \"%c\""
! 		    	" in \"%s\" of\n"
! 		    	"\t%s", aug[0], curr_section_name, objfile->name);
! 		  aug++;
  		}
  
! 	      last_cie = cie;
  	    }
  	  else
  	    {
  	      struct fde_unit *fde;
  	      struct cie_unit *cie;
+ 	      int dup = 0;
+ 	      CORE_ADDR	init_loc, base_offset;
  
! 	      /* We assume that debug_frame is in order 
! 	         CIE,FDE,CIE,FDE,FDE,...  and thus the CIE for this FDE
! 	         should be stored in last_cie pointer. If not, we'll 
! 	         try to find it by the older way.  */
! 	      if(last_cie)
! 		cie = last_cie;
! 	      else
  	      {
! 		warning ("%s: last_cie == NULL. "
! 			"Perhaps a malformed %s section in '%s'...?\n", 
! 			__func__, curr_section_name, objfile->name);
! 
! 		cie = cie_chunks;
! 		while(cie)
  		{
! 		  if (cie->objfile == objfile)
! 		  {
! 		    if (eh_frame && 
! 		        (cie->offset == (unit_offset + bytes_read - cie_id)))
! 			   break;
! 		    if (!eh_frame && (cie->offset == cie_id))
! 		       break;
! 		  }
! 
! 		  cie = cie->next;
  		}
+ 		if (!cie)
+ 		   error ("%s: can't find CIE pointer", __func__); 
+ 	      }
  
! 	      /* start-frame_buffer gives offset from 
! 	         the beginning of actual section.  */
! 	      base_offset = curr_section_vma + start - frame_buffer;
! 		      
! 	      init_loc = read_encoded_pointer (abfd, &start, 
! 	      			cie->addr_encoding, &flag_pcrel);
! 	      
! 	      if (flag_pcrel)
! 		init_loc += base_offset;
! 	      
! 	      /* For relocatable objects we must add an offset telling
! 	         where the section is actually mapped in the memory.  */
! 	      init_loc += ANOFFSET (objfile->section_offsets, 
! 				SECT_OFF_TEXT (objfile));
! 	
! 	      /* If we have both .debug_frame and .eh_frame present in 
! 	         a file, we must eliminate duplicate FDEs. For now we'll 
! 		 run through all entries in fde_chunks and check it one 
! 		 by one. Perhaps in the future we can implement a faster 
! 		 searching algorithm.  */
! 	      /* eh_frame==2 indicates, that this file has an already 
! 		 parsed .debug_frame too. When eh_frame==1 it means, that no
! 		 .debug_frame is present and thus we don't need to check for
! 		 duplicities. eh_frame==0 means, that we parse .debug_frame
! 		 and don't need to care about duplicate FDEs, because
! 		 .debug_frame is parsed first.  */
! 	      for(i = 0; eh_frame == 2 && i < fde_chunks.elems; i++)
! 	      {
! 		      /* We assume that FDEs in .debug_frame and .eh_frame 
! 		         have the same order (if they are present, of course).
! 			 If we find a duplicate entry for one FDE and save
! 			 it's index to last_dup_fde it's very likely, that 
! 			 we'll find an entry for the following FDE right after 
! 			 the previous one. Thus in many cases we'll run this 
! 			 loop only once.  */
! 		      last_dup_fde = (last_dup_fde + i) % fde_chunks.elems;
! 		      if(fde_chunks.array[last_dup_fde]->initial_location 
! 			      == init_loc)
! 		      {
! 			      dup=1;
! 			      break;
! 		      }
  	      }
  
! 	      /* Allocate a new entry only if this FDE isn't a duplicate of
! 	         something we have already seen.   */
! 	      if(!dup)
! 	      {
! 		      fde_chunks_need_space ();
! 		      fde = fde_unit_alloc ();
! 
! 		      fde_chunks.array[fde_chunks.elems++] = fde;
! 		      
! 		      fde->initial_location = init_loc;
! 		      fde->address_range = read_encoded_pointer (abfd, &start,
! 		      	cie->addr_encoding, NULL);
! 
! 		      fde->cie_ptr = cie;
! 
! 		      /* Here we intentionally ignore augmentation data
! 		         from FDE, because we don't need them.  */
! 		      if (cie->augmentation[0] == 'z')
! 			  start += read_uleb128 (abfd, &start);
  
! 		      fde->data = start;
! 		      fde->data_length = block_end - start;
! 	      }
  	    }
  	  start = block_end;
  	}
*************** dwarf2_build_frame_info (struct objfile 
*** 1525,1531 ****
  	     sizeof (struct fde_unit *), compare_fde_unit);
      }
  }
! \f
  
  /* Return the frame address.  */
  CORE_ADDR
--- 1632,1659 ----
  	     sizeof (struct fde_unit *), compare_fde_unit);
      }
  }
! 
! /* We must parse both .debug_frame section and .eh_frame because 
!    not all frames must be present in both of these sections.  */
! void
! dwarf2_build_frame_info (struct objfile *objfile)
! {
! 	int eh_frame;
! 
! 	/* If we have .debug_frame then the parser is called with 
! 	   eh_frame==0 for .debug_frame and eh_frame==2 for .eh_frame,
! 	   otherwise it's only called once for .eh_frame with argument
! 	   eh_frame==1  */
! 	eh_frame = 0;
! 	
! 	if (dwarf_frame_offset)
! 		parse_frame_info (objfile, dwarf_frame_offset, 
! 			dwarf_frame_size, eh_frame++);
! 	
! 	if (dwarf_eh_frame_offset)
! 		parse_frame_info (objfile, dwarf_eh_frame_offset, 
! 			dwarf_eh_frame_size, eh_frame+1);
! }
  
  /* Return the frame address.  */
  CORE_ADDR
*************** cfi_get_saved_register (char *raw_buffer
*** 1750,1756 ****
  	  break;
  	default:
  	  internal_error (__FILE__, __LINE__,
! 			  "cfi_get_saved_register: unknown register rule");
  	}
      }
  }
--- 1878,1884 ----
  	  break;
  	default:
  	  internal_error (__FILE__, __LINE__,
! 			  "%s: unknown register rule", __func__);
  	}
      }
  }
*************** cfi_virtual_frame_pointer (CORE_ADDR pc,
*** 1778,1784 ****
        *frame_offset = fs->cfa_offset;
      }
    else
!     error ("dwarf cfi error: CFA is not defined as CFA_REG_OFFSET");
  
    unwind_tmp_obstack_free ();
  }
--- 1906,1912 ----
        *frame_offset = fs->cfa_offset;
      }
    else
!     error ("%s: CFA is not defined as CFA_REG_OFFSET", __func__);
  
    unwind_tmp_obstack_free ();
  }

Re: [RFA] dwarf2cfi.c improvements

[Andrew Cagney]

>   	internal_error (__FILE__, __LINE__,
> ! 			"%s: unknown pointer encoding: 0x%x",
> ! 			__func__, encoding);
>         }

Michael,

Just FYI, __func__ can't be used.  It isn't considered part of the ISO C 
that GDB builds to.

> !     error ("%s: CFA is not defined as CFA_REG_OFFSET", __func__);

Hmm, I don't think user error messages need to include function names.

Can I suggest pulling the error() and internal_error() message changes 
out, cleaning them up, and then committing them separatly.   Better 
messages (eg printing the reason for a bad switch) are always a good idea.

That also has the benefit of trimming your patch back.

enjoy,
Andrew

Re: [RFA] dwarf2cfi.c improvements

[Michal Ludvig]

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

Andrew Cagney wrote:
> Can I suggest pulling the error() and internal_error() message changes 
> out, cleaning them up, and then committing them separatly.   Better 
> messages (eg printing the reason for a bad switch) are always a good idea.
> 
> That also has the benefit of trimming your patch back.

Well, this is the patch without unrelated message changes. Can I commit 
it? It seems to be quite stable in our everyday use...

2002-05-31  Michal Ludvig  <mludvig@suse.cz>

         * dwarf2cfi.c (dwarf_frame_buffer): Delete.
         (read_encoded_pointer): Add new argument,
         warn on indirect pointers.
         (execute_cfa_program): Warn on relative addressing.
         (dwarf2_build_frame_info): Only call parse_frame_info
         for .debug_frame and .eh_frame.
         (parse_frame_info): New, derived from dwarf2_build_frame_info,
         fixed augmentation handling, added relative addressing,
         ignore duplicate FDEs. Added comments.

Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* +420 2 9654 5373   * http://www.suse.cz

[-- Attachment #2: cfi-huge5.diff --]
[-- Type: text/plain, Size: 16675 bytes --]

Index: dwarf2cfi.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2cfi.c,v
retrieving revision 1.7
diff -c -3 -p -r1.7 dwarf2cfi.c
*** dwarf2cfi.c	22 May 2002 13:21:19 -0000	1.7
--- dwarf2cfi.c	4 Jun 2002 11:28:06 -0000
*************** extern file_ptr dwarf_frame_offset;
*** 188,195 ****
  extern unsigned int dwarf_frame_size;
  extern file_ptr dwarf_eh_frame_offset;
  extern unsigned int dwarf_eh_frame_size;
- 
- static char *dwarf_frame_buffer;
  \f
  
  extern char *dwarf2_read_section (struct objfile *objfile, file_ptr offset,
--- 188,193 ----
*************** static ULONGEST read_uleb128 (bfd *abfd,
*** 217,223 ****
  static LONGEST read_sleb128 (bfd *abfd, char **p);
  static CORE_ADDR read_pointer (bfd *abfd, char **p);
  static CORE_ADDR read_encoded_pointer (bfd *abfd, char **p,
! 				       unsigned char encoding);
  
  static LONGEST read_initial_length (bfd *abfd, char *buf, int *bytes_read);
  static ULONGEST read_length (bfd *abfd, char *buf, int *bytes_read,
--- 215,221 ----
  static LONGEST read_sleb128 (bfd *abfd, char **p);
  static CORE_ADDR read_pointer (bfd *abfd, char **p);
  static CORE_ADDR read_encoded_pointer (bfd *abfd, char **p,
! 			unsigned char encoding, int *flag_pcrel);
  
  static LONGEST read_initial_length (bfd *abfd, char *buf, int *bytes_read);
  static ULONGEST read_length (bfd *abfd, char *buf, int *bytes_read,
*************** read_pointer (bfd *abfd, char **p)
*** 487,496 ****
  }
  
  static CORE_ADDR
! read_encoded_pointer (bfd *abfd, char **p, unsigned char encoding)
  {
    CORE_ADDR ret;
! 
    switch (encoding & 0x0f)
      {
      case DW_EH_PE_absptr:
--- 485,497 ----
  }
  
  static CORE_ADDR
! read_encoded_pointer (bfd *abfd, char **p, unsigned char encoding, 
! 	int *flag_pcrel)
  {
    CORE_ADDR ret;
!   
!   if (flag_pcrel) *flag_pcrel = 0;
!   
    switch (encoding & 0x0f)
      {
      case DW_EH_PE_absptr:
*************** read_encoded_pointer (bfd *abfd, char **
*** 530,541 ****
      }
  
    if (ret != 0)
!     switch (encoding & 0xf0)
        {
        case DW_EH_PE_absptr:
  	break;
        case DW_EH_PE_pcrel:
! 	ret += (CORE_ADDR) *p;
  	break;
        case DW_EH_PE_textrel:
        case DW_EH_PE_datarel:
--- 531,543 ----
      }
  
    if (ret != 0)
!   {
!     switch (encoding & 0x70)
        {
        case DW_EH_PE_absptr:
  	break;
        case DW_EH_PE_pcrel:
! 	if (flag_pcrel) *flag_pcrel = 1;
  	break;
        case DW_EH_PE_textrel:
        case DW_EH_PE_datarel:
*************** read_encoded_pointer (bfd *abfd, char **
*** 544,549 ****
--- 546,555 ----
  	internal_error (__FILE__, __LINE__,
  			"read_encoded_pointer: unknown pointer encoding");
        }
+     
+     if (encoding & DW_EH_PE_indirect)
+       warning ("CFI: Unsupported pointer encoding: DW_EH_PE_indirect");
+   }
  
    return ret;
  }
*************** execute_cfa_program ( struct objfile *ob
*** 597,602 ****
--- 603,609 ----
        unsigned char insn = *insn_ptr++;
        ULONGEST reg, uoffset;
        LONGEST offset;
+       int flag_pcrel;
  
        if (insn & DW_CFA_advance_loc)
  	fs->pc += (insn & 0x3f) * fs->code_align;
*************** execute_cfa_program ( struct objfile *ob
*** 618,624 ****
  	  {
  	  case DW_CFA_set_loc:
  	    fs->pc = read_encoded_pointer (objfile->obfd, &insn_ptr,
! 					   fs->addr_encoding);
  	    break;
  
  	  case DW_CFA_advance_loc1:
--- 625,637 ----
  	  {
  	  case DW_CFA_set_loc:
  	    fs->pc = read_encoded_pointer (objfile->obfd, &insn_ptr,
! 					   fs->addr_encoding, &flag_pcrel);
! 	    if (flag_pcrel)
! 	    {
! 	      warning ("CFI: DW_CFA_set_loc uses relative addressing at pc=%p", 
! 	      	(void*)fs->pc);
! 	      fs->pc += (CORE_ADDR)insn_ptr;
! 	    }
  	    break;
  
  	  case DW_CFA_advance_loc1:
*************** compare_fde_unit (const void *a, const v
*** 1372,1409 ****
  
  /*  Build the cie_chunks and fde_chunks tables from informations
      in .debug_frame section.  */
! void
! dwarf2_build_frame_info (struct objfile *objfile)
  {
    bfd *abfd = objfile->obfd;
!   char *start = NULL;
!   char *end = NULL;
!   int from_eh = 0;
  
    obstack_init (&unwind_tmp_obstack);
  
!   dwarf_frame_buffer = 0;
! 
!   if (dwarf_frame_offset)
!     {
!       dwarf_frame_buffer = dwarf2_read_section (objfile,
! 						dwarf_frame_offset,
! 						dwarf_frame_size);
! 
!       start = dwarf_frame_buffer;
!       end = dwarf_frame_buffer + dwarf_frame_size;
!     }
!   else if (dwarf_eh_frame_offset)
!     {
!       dwarf_frame_buffer = dwarf2_read_section (objfile,
! 						dwarf_eh_frame_offset,
! 						dwarf_eh_frame_size);
! 
!       start = dwarf_frame_buffer;
!       end = dwarf_frame_buffer + dwarf_eh_frame_size;
  
!       from_eh = 1;
!     }
  
    if (start)
      {
--- 1385,1413 ----
  
  /*  Build the cie_chunks and fde_chunks tables from informations
      in .debug_frame section.  */
! static void
! parse_frame_info (struct objfile *objfile, file_ptr frame_offset, 
! 	unsigned int frame_size, int eh_frame)
  {
    bfd *abfd = objfile->obfd;
!   asection *curr_section_ptr;
!   char *start = NULL, *end = NULL, *frame_buffer = NULL;
!   char *curr_section_name, *aug_data;
!   struct cie_unit *last_cie=NULL;
!   int last_dup_fde = 0, aug_len, i;
!   CORE_ADDR curr_section_vma = 0;
  
    obstack_init (&unwind_tmp_obstack);
  
!   frame_buffer = dwarf2_read_section (objfile, frame_offset, frame_size);
  
!   start = frame_buffer;
!   end = frame_buffer + frame_size;
!   
!   curr_section_name = eh_frame?".eh_frame":".debug_frame";
!   curr_section_ptr = bfd_get_section_by_name (abfd, curr_section_name);
!   if (curr_section_ptr)
!     curr_section_vma = curr_section_ptr->vma;
  
    if (start)
      {
*************** dwarf2_build_frame_info (struct objfile 
*** 1411,1419 ****
  	{
  	  unsigned long length;
  	  ULONGEST cie_id;
! 	  ULONGEST unit_offset = start - dwarf_frame_buffer;
! 	  int bytes_read;
! 	  int dwarf64;
  	  char *block_end;
  
  	  length = read_initial_length (abfd, start, &bytes_read);
--- 1415,1422 ----
  	{
  	  unsigned long length;
  	  ULONGEST cie_id;
! 	  ULONGEST unit_offset = start - frame_buffer;
! 	  int bytes_read, dwarf64, flag_pcrel;
  	  char *block_end;
  
  	  length = read_initial_length (abfd, start, &bytes_read);
*************** dwarf2_build_frame_info (struct objfile 
*** 1421,1430 ****
  	  dwarf64 = (bytes_read == 12);
  	  block_end = start + length;
  
  	  cie_id = read_length (abfd, start, &bytes_read, dwarf64);
  	  start += bytes_read;
  
! 	  if ((from_eh && cie_id == 0) || is_cie (cie_id, dwarf64))
  	    {
  	      struct cie_unit *cie = cie_unit_alloc ();
  	      char *aug;
--- 1424,1439 ----
  	  dwarf64 = (bytes_read == 12);
  	  block_end = start + length;
  
+ 	  if(length == 0)
+ 	  {
+ 		  start = block_end;
+ 		  continue;
+ 	  }
+ 
  	  cie_id = read_length (abfd, start, &bytes_read, dwarf64);
  	  start += bytes_read;
  
! 	  if ((eh_frame && cie_id == 0) || is_cie (cie_id, dwarf64))
  	    {
  	      struct cie_unit *cie = cie_unit_alloc ();
  	      char *aug;
*************** dwarf2_build_frame_info (struct objfile 
*** 1440,1523 ****
  	      start++;		/* version */
  
  	      cie->augmentation = aug = start;
! 	      while (*start)
! 		start++;
! 	      start++;		/* skip past NUL */
  
  	      cie->code_align = read_uleb128 (abfd, &start);
  	      cie->data_align = read_sleb128 (abfd, &start);
  	      cie->ra = read_1u (abfd, &start);
  
  	      if (*aug == 'z')
  		{
! 		  int xtra = read_uleb128 (abfd, &start);
! 		  start += xtra;
  		  ++aug;
  		}
  
  	      while (*aug != '\0')
  		{
  		  if (aug[0] == 'e' && aug[1] == 'h')
  		    {
! 		      start += sizeof (void *);
! 		      aug += 2;
  		    }
  		  else if (aug[0] == 'R')
  		    {
! 		      cie->addr_encoding = *start++;
! 		      aug += 1;
  		    }
! 		  else if (aug[0] == 'P')
  		    {
! 		      CORE_ADDR ptr;
! 		      ptr = read_encoded_pointer (abfd, &start,
! 						  cie->addr_encoding);
! 		      aug += 1;
  		    }
  		  else
! 		    warning ("%s(): unknown augmentation", __func__);
  		}
  
! 	      cie->data = start;
! 	      cie->data_length = block_end - start;
  	    }
  	  else
  	    {
  	      struct fde_unit *fde;
  	      struct cie_unit *cie;
  
! 	      fde_chunks_need_space ();
! 	      fde = fde_unit_alloc ();
! 
! 	      fde_chunks.array[fde_chunks.elems++] = fde;
! 	      
! 	      fde->initial_location = read_pointer (abfd, &start)
! 	        + ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
! 	      fde->address_range = read_pointer (abfd, &start);
! 
! 	      cie = cie_chunks;
! 	      while(cie)
  	      {
! 	        if (cie->objfile == objfile)
  		{
! 		  if (from_eh && (cie->offset == (unit_offset + bytes_read - cie_id)))
! 		      break;
! 		  if (!from_eh && (cie->offset == cie_id))
! 		    break;
  		}
  
! 		cie = cie->next;
  	      }
- 	    
- 	      if (!cie)
- 		error ("%s(): can't find CIE pointer", __func__);
- 	      fde->cie_ptr = cie;
  
! 	      if (cie->augmentation[0] == 'z')
! 		  read_uleb128 (abfd, &start);
  
! 	      fde->data = start;
! 	      fde->data_length = block_end - start;
  	    }
  	  start = block_end;
  	}
--- 1449,1626 ----
  	      start++;		/* version */
  
  	      cie->augmentation = aug = start;
! 	      while (*start++);	/* Skips last NULL as well */
  
  	      cie->code_align = read_uleb128 (abfd, &start);
  	      cie->data_align = read_sleb128 (abfd, &start);
  	      cie->ra = read_1u (abfd, &start);
  
+ 	      /* Augmentation:
+ 		 z	Indicates that a uleb128 is present to size the
+ 		 	augmentation section.
+ 		 L	Indicates the encoding (and thus presence) of
+ 			an LSDA pointer in the FDE augmentation.
+ 		 R	Indicates a non-default pointer encoding for
+ 			FDE code pointers.
+ 		 P	Indicates the presence of an encoding + language
+ 			personality routine in the CIE augmentation.
+ 	      
+ 	         [This info comes from GCC's dwarf2out.c]
+ 		*/
  	      if (*aug == 'z')
  		{
! 		  aug_len = read_uleb128 (abfd, &start);
! 		  aug_data = start;
! 		  start += aug_len;
  		  ++aug;
  		}
  
+ 	      cie->data = start;
+ 	      cie->data_length = block_end - cie->data;
+ 	      
  	      while (*aug != '\0')
  		{
  		  if (aug[0] == 'e' && aug[1] == 'h')
  		    {
! 		      aug_data += sizeof (void *);
! 		      aug++;
  		    }
  		  else if (aug[0] == 'R')
+ 		      cie->addr_encoding = *aug_data++;
+ 		  else if (aug[0] == 'P')
  		    {
! 		      CORE_ADDR pers_addr;
! 		      int pers_addr_enc;
! 		      
! 		      pers_addr_enc = *aug_data++;
! 		      /* We use (pers_addr_enc & 0x7f) to avoid 
! 		         warning about indirect relative addressing.
! 			 We don't need pers_addr value anyway and so we 
! 			 don't care if it is correct :-)  */
! 		      pers_addr = read_encoded_pointer (abfd, &aug_data,
! 					pers_addr_enc & 0x7f, NULL);
  		    }
! 		  else if (aug[0] == 'L' && eh_frame)
  		    {
! 		      int lsda_addr_enc;
! 
! 		      /* Perhaps we should save this to CIE for later use?
! 		         Do we need it for something in GDB?  */
! 		      lsda_addr_enc = *aug_data++;
  		    }
  		  else
! 		    warning ("CFI warning: unknown augmentation \"%c\""
! 		    	" in \"%s\" of\n"
! 		    	"\t%s", aug[0], curr_section_name, objfile->name);
! 		  aug++;
  		}
  
! 	      last_cie = cie;
  	    }
  	  else
  	    {
  	      struct fde_unit *fde;
  	      struct cie_unit *cie;
+ 	      int dup = 0;
+ 	      CORE_ADDR	init_loc, base_offset;
  
! 	      /* We assume that debug_frame is in order 
! 	         CIE,FDE,CIE,FDE,FDE,...  and thus the CIE for this FDE
! 	         should be stored in last_cie pointer. If not, we'll 
! 	         try to find it by the older way.  */
! 	      if(last_cie)
! 		cie = last_cie;
! 	      else
  	      {
! 		warning ("CFI: last_cie == NULL. "
! 			"Perhaps a malformed %s section in '%s'...?\n", 
! 			curr_section_name, objfile->name);
! 
! 		cie = cie_chunks;
! 		while(cie)
  		{
! 		  if (cie->objfile == objfile)
! 		  {
! 		    if (eh_frame && 
! 		        (cie->offset == (unit_offset + bytes_read - cie_id)))
! 			   break;
! 		    if (!eh_frame && (cie->offset == cie_id))
! 		       break;
! 		  }
! 
! 		  cie = cie->next;
  		}
+ 		if (!cie)
+ 		   error ("CFI: can't find CIE pointer"); 
+ 	      }
  
! 	      /* start-frame_buffer gives offset from 
! 	         the beginning of actual section.  */
! 	      base_offset = curr_section_vma + start - frame_buffer;
! 		      
! 	      init_loc = read_encoded_pointer (abfd, &start, 
! 	      			cie->addr_encoding, &flag_pcrel);
! 	      
! 	      if (flag_pcrel)
! 		init_loc += base_offset;
! 	      
! 	      /* For relocatable objects we must add an offset telling
! 	         where the section is actually mapped in the memory.  */
! 	      init_loc += ANOFFSET (objfile->section_offsets, 
! 				SECT_OFF_TEXT (objfile));
! 	
! 	      /* If we have both .debug_frame and .eh_frame present in 
! 	         a file, we must eliminate duplicate FDEs. For now we'll 
! 		 run through all entries in fde_chunks and check it one 
! 		 by one. Perhaps in the future we can implement a faster 
! 		 searching algorithm.  */
! 	      /* eh_frame==2 indicates, that this file has an already 
! 		 parsed .debug_frame too. When eh_frame==1 it means, that no
! 		 .debug_frame is present and thus we don't need to check for
! 		 duplicities. eh_frame==0 means, that we parse .debug_frame
! 		 and don't need to care about duplicate FDEs, because
! 		 .debug_frame is parsed first.  */
! 	      for(i = 0; eh_frame == 2 && i < fde_chunks.elems; i++)
! 	      {
! 		      /* We assume that FDEs in .debug_frame and .eh_frame 
! 		         have the same order (if they are present, of course).
! 			 If we find a duplicate entry for one FDE and save
! 			 it's index to last_dup_fde it's very likely, that 
! 			 we'll find an entry for the following FDE right after 
! 			 the previous one. Thus in many cases we'll run this 
! 			 loop only once.  */
! 		      last_dup_fde = (last_dup_fde + i) % fde_chunks.elems;
! 		      if(fde_chunks.array[last_dup_fde]->initial_location 
! 			      == init_loc)
! 		      {
! 			      dup=1;
! 			      break;
! 		      }
  	      }
  
! 	      /* Allocate a new entry only if this FDE isn't a duplicate of
! 	         something we have already seen.   */
! 	      if(!dup)
! 	      {
! 		      fde_chunks_need_space ();
! 		      fde = fde_unit_alloc ();
  
! 		      fde_chunks.array[fde_chunks.elems++] = fde;
! 		      
! 		      fde->initial_location = init_loc;
! 		      fde->address_range = read_encoded_pointer (abfd, &start,
! 		      	cie->addr_encoding, NULL);
! 
! 		      fde->cie_ptr = cie;
! 
! 		      /* Here we intentionally ignore augmentation data
! 		         from FDE, because we don't need them.  */
! 		      if (cie->augmentation[0] == 'z')
! 			  start += read_uleb128 (abfd, &start);
! 
! 		      fde->data = start;
! 		      fde->data_length = block_end - start;
! 	      }
  	    }
  	  start = block_end;
  	}
*************** dwarf2_build_frame_info (struct objfile 
*** 1525,1531 ****
  	     sizeof (struct fde_unit *), compare_fde_unit);
      }
  }
! \f
  
  /* Return the frame address.  */
  CORE_ADDR
--- 1628,1655 ----
  	     sizeof (struct fde_unit *), compare_fde_unit);
      }
  }
! 
! /* We must parse both .debug_frame section and .eh_frame because 
!    not all frames must be present in both of these sections.  */
! void
! dwarf2_build_frame_info (struct objfile *objfile)
! {
! 	int eh_frame;
! 
! 	/* If we have .debug_frame then the parser is called with 
! 	   eh_frame==0 for .debug_frame and eh_frame==2 for .eh_frame,
! 	   otherwise it's only called once for .eh_frame with argument
! 	   eh_frame==1  */
! 	eh_frame = 0;
! 
! 	if (dwarf_frame_offset)
! 		parse_frame_info (objfile, dwarf_frame_offset, 
! 			dwarf_frame_size, eh_frame++);
! 
! 	if (dwarf_eh_frame_offset)
! 		parse_frame_info (objfile, dwarf_eh_frame_offset, 
! 			dwarf_eh_frame_size, eh_frame+1);
! }
  
  /* Return the frame address.  */
  CORE_ADDR

Re: [RFA] dwarf2cfi.c improvements

[Elena Zannoni]

Michal Ludvig writes:
 > Andrew Cagney wrote:
 > > Can I suggest pulling the error() and internal_error() message changes 
 > > out, cleaning them up, and then committing them separatly.   Better 
 > > messages (eg printing the reason for a bad switch) are always a good idea.
 > > 
 > > That also has the benefit of trimming your patch back.
 > 
 > Well, this is the patch without unrelated message changes. Can I commit 
 > it? It seems to be quite stable in our everyday use...
 > 

Michal, sorry, not as is.  

There are coding standard violations having to do with spaces before
'(' and after ')' in casts. Also, it is better to not have multiple
initializations in a single line, like this:
 > !   char *start = NULL, *end = NULL, *frame_buffer = NULL;
Conditional expressions are also flagged by the ARI.

Getting rid of the dwarf_frame_buffer would also require an update to
the comment in the structure cie_unit.

Can I suggest that you split the pcrel changes into a separate patch?
About pcrel, I don't like the use of the flag like you have below.
Could you find some other way to do it w/o passing the address of the
flag around? Could you maybe change the function to return 0/1 and have the
current return value as an out parameter instead?
Or alternatively, could you have the function read_encoded_pointer
emit the warning itself? This would not require you to pull out of the
function some of its current logic (the increment). After all, none of
the other error/warning messages are that detailed, they don't print
the PC value. I noticed that in a couple of the calls to
read_encoded_pointer the code doesn't care about emitting the warning,
why? Or add a pc parameter for the purpose of the warning.


About the eh_frame part of the patch.
I don't really like the use of eh_frame as you have in dwarf2_build_frame_info.
Could you try to figure out a simpler way to deal with the two cases?
You could use an enumeration for eh_frame.
Could you do 2 passes, one for each section?

I am not able to apply your patch, which I usually do, to try a test
run. Is there anyway you can regenerate the patch? Maybe with a unified
diff instead? (it's just that I find it easier to read)

I would like to give you more intelligent comments about the eh_frame
part, but I need to apply the patch first.

Elena


 > 2002-05-31  Michal Ludvig  <mludvig@suse.cz>
 > 
 >          * dwarf2cfi.c (dwarf_frame_buffer): Delete.
 >          (read_encoded_pointer): Add new argument,
 >          warn on indirect pointers.
 >          (execute_cfa_program): Warn on relative addressing.
 >          (dwarf2_build_frame_info): Only call parse_frame_info
 >          for .debug_frame and .eh_frame.
 >          (parse_frame_info): New, derived from dwarf2_build_frame_info,
 >          fixed augmentation handling, added relative addressing,
 >          ignore duplicate FDEs. Added comments.
 > 
 > Michal Ludvig
 > -- 
 > * SuSE CR, s.r.o     * mludvig@suse.cz
 > * +420 2 9654 5373   * http://www.suse.cz
 > Index: dwarf2cfi.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/dwarf2cfi.c,v
 > retrieving revision 1.7
 > diff -c -3 -p -r1.7 dwarf2cfi.c
 > *** dwarf2cfi.c	22 May 2002 13:21:19 -0000	1.7
 > --- dwarf2cfi.c	4 Jun 2002 11:28:06 -0000
 > *************** extern file_ptr dwarf_frame_offset;
 > *** 188,195 ****
 >   extern unsigned int dwarf_frame_size;
 >   extern file_ptr dwarf_eh_frame_offset;
 >   extern unsigned int dwarf_eh_frame_size;
 > - 
 > - static char *dwarf_frame_buffer;
 >   \f
 >   
 >   extern char *dwarf2_read_section (struct objfile *objfile, file_ptr offset,
 > --- 188,193 ----
 > *************** static ULONGEST read_uleb128 (bfd *abfd,
 > *** 217,223 ****
 >   static LONGEST read_sleb128 (bfd *abfd, char **p);
 >   static CORE_ADDR read_pointer (bfd *abfd, char **p);
 >   static CORE_ADDR read_encoded_pointer (bfd *abfd, char **p,
 > ! 				       unsigned char encoding);
 >   
 >   static LONGEST read_initial_length (bfd *abfd, char *buf, int *bytes_read);
 >   static ULONGEST read_length (bfd *abfd, char *buf, int *bytes_read,
 > --- 215,221 ----
 >   static LONGEST read_sleb128 (bfd *abfd, char **p);
 >   static CORE_ADDR read_pointer (bfd *abfd, char **p);
 >   static CORE_ADDR read_encoded_pointer (bfd *abfd, char **p,
 > ! 			unsigned char encoding, int *flag_pcrel);
 >   
 >   static LONGEST read_initial_length (bfd *abfd, char *buf, int *bytes_read);
 >   static ULONGEST read_length (bfd *abfd, char *buf, int *bytes_read,
 > *************** read_pointer (bfd *abfd, char **p)
 > *** 487,496 ****
 >   }
 >   
 >   static CORE_ADDR
 > ! read_encoded_pointer (bfd *abfd, char **p, unsigned char encoding)
 >   {
 >     CORE_ADDR ret;
 > ! 
 >     switch (encoding & 0x0f)
 >       {
 >       case DW_EH_PE_absptr:
 > --- 485,497 ----
 >   }
 >   
 >   static CORE_ADDR
 > ! read_encoded_pointer (bfd *abfd, char **p, unsigned char encoding, 
 > ! 	int *flag_pcrel)
 >   {
 >     CORE_ADDR ret;
 > !   
 > !   if (flag_pcrel) *flag_pcrel = 0;
 > !   
 >     switch (encoding & 0x0f)
 >       {
 >       case DW_EH_PE_absptr:
 > *************** read_encoded_pointer (bfd *abfd, char **
 > *** 530,541 ****
 >       }
 >   
 >     if (ret != 0)
 > !     switch (encoding & 0xf0)
 >         {
 >         case DW_EH_PE_absptr:
 >   	break;
 >         case DW_EH_PE_pcrel:
 > ! 	ret += (CORE_ADDR) *p;
 >   	break;
 >         case DW_EH_PE_textrel:
 >         case DW_EH_PE_datarel:
 > --- 531,543 ----
 >       }
 >   
 >     if (ret != 0)
 > !   {
 > !     switch (encoding & 0x70)
 >         {
 >         case DW_EH_PE_absptr:
 >   	break;
 >         case DW_EH_PE_pcrel:
 > ! 	if (flag_pcrel) *flag_pcrel = 1;
 >   	break;
 >         case DW_EH_PE_textrel:
 >         case DW_EH_PE_datarel:
 > *************** read_encoded_pointer (bfd *abfd, char **
 > *** 544,549 ****
 > --- 546,555 ----
 >   	internal_error (__FILE__, __LINE__,
 >   			"read_encoded_pointer: unknown pointer encoding");
 >         }
 > +     
 > +     if (encoding & DW_EH_PE_indirect)
 > +       warning ("CFI: Unsupported pointer encoding: DW_EH_PE_indirect");
 > +   }
 >   
 >     return ret;
 >   }
 > *************** execute_cfa_program ( struct objfile *ob
 > *** 597,602 ****
 > --- 603,609 ----
 >         unsigned char insn = *insn_ptr++;
 >         ULONGEST reg, uoffset;
 >         LONGEST offset;
 > +       int flag_pcrel;
 >   
 >         if (insn & DW_CFA_advance_loc)
 >   	fs->pc += (insn & 0x3f) * fs->code_align;
 > *************** execute_cfa_program ( struct objfile *ob
 > *** 618,624 ****
 >   	  {
 >   	  case DW_CFA_set_loc:
 >   	    fs->pc = read_encoded_pointer (objfile->obfd, &insn_ptr,
 > ! 					   fs->addr_encoding);
 >   	    break;
 >   
 >   	  case DW_CFA_advance_loc1:
 > --- 625,637 ----
 >   	  {
 >   	  case DW_CFA_set_loc:
 >   	    fs->pc = read_encoded_pointer (objfile->obfd, &insn_ptr,
 > ! 					   fs->addr_encoding, &flag_pcrel);
 > ! 	    if (flag_pcrel)
 > ! 	    {
 > ! 	      warning ("CFI: DW_CFA_set_loc uses relative addressing at pc=%p", 
 > ! 	      	(void*)fs->pc);
 > ! 	      fs->pc += (CORE_ADDR)insn_ptr;
 > ! 	    }
 >   	    break;
 >   
 >   	  case DW_CFA_advance_loc1:
 > *************** compare_fde_unit (const void *a, const v
 > *** 1372,1409 ****
 >   
 >   /*  Build the cie_chunks and fde_chunks tables from informations
 >       in .debug_frame section.  */
 > ! void
 > ! dwarf2_build_frame_info (struct objfile *objfile)
 >   {
 >     bfd *abfd = objfile->obfd;
 > !   char *start = NULL;
 > !   char *end = NULL;
 > !   int from_eh = 0;
 >   
 >     obstack_init (&unwind_tmp_obstack);
 >   
 > !   dwarf_frame_buffer = 0;
 > ! 
 > !   if (dwarf_frame_offset)
 > !     {
 > !       dwarf_frame_buffer = dwarf2_read_section (objfile,
 > ! 						dwarf_frame_offset,
 > ! 						dwarf_frame_size);
 > ! 
 > !       start = dwarf_frame_buffer;
 > !       end = dwarf_frame_buffer + dwarf_frame_size;
 > !     }
 > !   else if (dwarf_eh_frame_offset)
 > !     {
 > !       dwarf_frame_buffer = dwarf2_read_section (objfile,
 > ! 						dwarf_eh_frame_offset,
 > ! 						dwarf_eh_frame_size);
 > ! 
 > !       start = dwarf_frame_buffer;
 > !       end = dwarf_frame_buffer + dwarf_eh_frame_size;
 >   
 > !       from_eh = 1;
 > !     }
 >   
 >     if (start)
 >       {
 > --- 1385,1413 ----
 >   
 >   /*  Build the cie_chunks and fde_chunks tables from informations
 >       in .debug_frame section.  */
 > ! static void
 > ! parse_frame_info (struct objfile *objfile, file_ptr frame_offset, 
 > ! 	unsigned int frame_size, int eh_frame)
 >   {
 >     bfd *abfd = objfile->obfd;
 > !   asection *curr_section_ptr;
 > !   char *start = NULL, *end = NULL, *frame_buffer = NULL;
 > !   char *curr_section_name, *aug_data;
 > !   struct cie_unit *last_cie=NULL;
 > !   int last_dup_fde = 0, aug_len, i;
 > !   CORE_ADDR curr_section_vma = 0;
 >   
 >     obstack_init (&unwind_tmp_obstack);
 >   
 > !   frame_buffer = dwarf2_read_section (objfile, frame_offset, frame_size);
 >   
 > !   start = frame_buffer;
 > !   end = frame_buffer + frame_size;
 > !   
 > !   curr_section_name = eh_frame?".eh_frame":".debug_frame";
 > !   curr_section_ptr = bfd_get_section_by_name (abfd, curr_section_name);
 > !   if (curr_section_ptr)
 > !     curr_section_vma = curr_section_ptr->vma;
 >   
 >     if (start)
 >       {
 > *************** dwarf2_build_frame_info (struct objfile 
 > *** 1411,1419 ****
 >   	{
 >   	  unsigned long length;
 >   	  ULONGEST cie_id;
 > ! 	  ULONGEST unit_offset = start - dwarf_frame_buffer;
 > ! 	  int bytes_read;
 > ! 	  int dwarf64;
 >   	  char *block_end;
 >   
 >   	  length = read_initial_length (abfd, start, &bytes_read);
 > --- 1415,1422 ----
 >   	{
 >   	  unsigned long length;
 >   	  ULONGEST cie_id;
 > ! 	  ULONGEST unit_offset = start - frame_buffer;
 > ! 	  int bytes_read, dwarf64, flag_pcrel;
 >   	  char *block_end;
 >   
 >   	  length = read_initial_length (abfd, start, &bytes_read);
 > *************** dwarf2_build_frame_info (struct objfile 
 > *** 1421,1430 ****
 >   	  dwarf64 = (bytes_read == 12);
 >   	  block_end = start + length;
 >   
 >   	  cie_id = read_length (abfd, start, &bytes_read, dwarf64);
 >   	  start += bytes_read;
 >   
 > ! 	  if ((from_eh && cie_id == 0) || is_cie (cie_id, dwarf64))
 >   	    {
 >   	      struct cie_unit *cie = cie_unit_alloc ();
 >   	      char *aug;
 > --- 1424,1439 ----
 >   	  dwarf64 = (bytes_read == 12);
 >   	  block_end = start + length;
 >   
 > + 	  if(length == 0)
 > + 	  {
 > + 		  start = block_end;
 > + 		  continue;
 > + 	  }
 > + 
 >   	  cie_id = read_length (abfd, start, &bytes_read, dwarf64);
 >   	  start += bytes_read;
 >   
 > ! 	  if ((eh_frame && cie_id == 0) || is_cie (cie_id, dwarf64))
 >   	    {
 >   	      struct cie_unit *cie = cie_unit_alloc ();
 >   	      char *aug;
 > *************** dwarf2_build_frame_info (struct objfile 
 > *** 1440,1523 ****
 >   	      start++;		/* version */
 >   
 >   	      cie->augmentation = aug = start;
 > ! 	      while (*start)
 > ! 		start++;
 > ! 	      start++;		/* skip past NUL */
 >   
 >   	      cie->code_align = read_uleb128 (abfd, &start);
 >   	      cie->data_align = read_sleb128 (abfd, &start);
 >   	      cie->ra = read_1u (abfd, &start);
 >   
 >   	      if (*aug == 'z')
 >   		{
 > ! 		  int xtra = read_uleb128 (abfd, &start);
 > ! 		  start += xtra;
 >   		  ++aug;
 >   		}
 >   
 >   	      while (*aug != '\0')
 >   		{
 >   		  if (aug[0] == 'e' && aug[1] == 'h')
 >   		    {
 > ! 		      start += sizeof (void *);
 > ! 		      aug += 2;
 >   		    }
 >   		  else if (aug[0] == 'R')
 >   		    {
 > ! 		      cie->addr_encoding = *start++;
 > ! 		      aug += 1;
 >   		    }
 > ! 		  else if (aug[0] == 'P')
 >   		    {
 > ! 		      CORE_ADDR ptr;
 > ! 		      ptr = read_encoded_pointer (abfd, &start,
 > ! 						  cie->addr_encoding);
 > ! 		      aug += 1;
 >   		    }
 >   		  else
 > ! 		    warning ("%s(): unknown augmentation", __func__);
 >   		}
 >   
 > ! 	      cie->data = start;
 > ! 	      cie->data_length = block_end - start;
 >   	    }
 >   	  else
 >   	    {
 >   	      struct fde_unit *fde;
 >   	      struct cie_unit *cie;
 >   
 > ! 	      fde_chunks_need_space ();
 > ! 	      fde = fde_unit_alloc ();
 > ! 
 > ! 	      fde_chunks.array[fde_chunks.elems++] = fde;
 > ! 	      
 > ! 	      fde->initial_location = read_pointer (abfd, &start)
 > ! 	        + ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
 > ! 	      fde->address_range = read_pointer (abfd, &start);
 > ! 
 > ! 	      cie = cie_chunks;
 > ! 	      while(cie)
 >   	      {
 > ! 	        if (cie->objfile == objfile)
 >   		{
 > ! 		  if (from_eh && (cie->offset == (unit_offset + bytes_read - cie_id)))
 > ! 		      break;
 > ! 		  if (!from_eh && (cie->offset == cie_id))
 > ! 		    break;
 >   		}
 >   
 > ! 		cie = cie->next;
 >   	      }
 > - 	    
 > - 	      if (!cie)
 > - 		error ("%s(): can't find CIE pointer", __func__);
 > - 	      fde->cie_ptr = cie;
 >   
 > ! 	      if (cie->augmentation[0] == 'z')
 > ! 		  read_uleb128 (abfd, &start);
 >   
 > ! 	      fde->data = start;
 > ! 	      fde->data_length = block_end - start;
 >   	    }
 >   	  start = block_end;
 >   	}
 > --- 1449,1626 ----
 >   	      start++;		/* version */
 >   
 >   	      cie->augmentation = aug = start;
 > ! 	      while (*start++);	/* Skips last NULL as well */
 >   
 >   	      cie->code_align = read_uleb128 (abfd, &start);
 >   	      cie->data_align = read_sleb128 (abfd, &start);
 >   	      cie->ra = read_1u (abfd, &start);
 >   
 > + 	      /* Augmentation:
 > + 		 z	Indicates that a uleb128 is present to size the
 > + 		 	augmentation section.
 > + 		 L	Indicates the encoding (and thus presence) of
 > + 			an LSDA pointer in the FDE augmentation.
 > + 		 R	Indicates a non-default pointer encoding for
 > + 			FDE code pointers.
 > + 		 P	Indicates the presence of an encoding + language
 > + 			personality routine in the CIE augmentation.
 > + 	      
 > + 	         [This info comes from GCC's dwarf2out.c]
 > + 		*/
 >   	      if (*aug == 'z')
 >   		{
 > ! 		  aug_len = read_uleb128 (abfd, &start);
 > ! 		  aug_data = start;
 > ! 		  start += aug_len;
 >   		  ++aug;
 >   		}
 >   
 > + 	      cie->data = start;
 > + 	      cie->data_length = block_end - cie->data;
 > + 	      
 >   	      while (*aug != '\0')
 >   		{
 >   		  if (aug[0] == 'e' && aug[1] == 'h')
 >   		    {
 > ! 		      aug_data += sizeof (void *);
 > ! 		      aug++;
 >   		    }
 >   		  else if (aug[0] == 'R')
 > + 		      cie->addr_encoding = *aug_data++;
 > + 		  else if (aug[0] == 'P')
 >   		    {
 > ! 		      CORE_ADDR pers_addr;
 > ! 		      int pers_addr_enc;
 > ! 		      
 > ! 		      pers_addr_enc = *aug_data++;
 > ! 		      /* We use (pers_addr_enc & 0x7f) to avoid 
 > ! 		         warning about indirect relative addressing.
 > ! 			 We don't need pers_addr value anyway and so we 
 > ! 			 don't care if it is correct :-)  */
 > ! 		      pers_addr = read_encoded_pointer (abfd, &aug_data,
 > ! 					pers_addr_enc & 0x7f, NULL);
 >   		    }
 > ! 		  else if (aug[0] == 'L' && eh_frame)
 >   		    {
 > ! 		      int lsda_addr_enc;
 > ! 
 > ! 		      /* Perhaps we should save this to CIE for later use?
 > ! 		         Do we need it for something in GDB?  */
 > ! 		      lsda_addr_enc = *aug_data++;
 >   		    }
 >   		  else
 > ! 		    warning ("CFI warning: unknown augmentation \"%c\""
 > ! 		    	" in \"%s\" of\n"
 > ! 		    	"\t%s", aug[0], curr_section_name, objfile->name);
 > ! 		  aug++;
 >   		}
 >   
 > ! 	      last_cie = cie;
 >   	    }
 >   	  else
 >   	    {
 >   	      struct fde_unit *fde;
 >   	      struct cie_unit *cie;
 > + 	      int dup = 0;
 > + 	      CORE_ADDR	init_loc, base_offset;
 >   
 > ! 	      /* We assume that debug_frame is in order 
 > ! 	         CIE,FDE,CIE,FDE,FDE,...  and thus the CIE for this FDE
 > ! 	         should be stored in last_cie pointer. If not, we'll 
 > ! 	         try to find it by the older way.  */
 > ! 	      if(last_cie)
 > ! 		cie = last_cie;
 > ! 	      else
 >   	      {
 > ! 		warning ("CFI: last_cie == NULL. "
 > ! 			"Perhaps a malformed %s section in '%s'...?\n", 
 > ! 			curr_section_name, objfile->name);
 > ! 
 > ! 		cie = cie_chunks;
 > ! 		while(cie)
 >   		{
 > ! 		  if (cie->objfile == objfile)
 > ! 		  {
 > ! 		    if (eh_frame && 
 > ! 		        (cie->offset == (unit_offset + bytes_read - cie_id)))
 > ! 			   break;
 > ! 		    if (!eh_frame && (cie->offset == cie_id))
 > ! 		       break;
 > ! 		  }
 > ! 
 > ! 		  cie = cie->next;
 >   		}
 > + 		if (!cie)
 > + 		   error ("CFI: can't find CIE pointer"); 
 > + 	      }
 >   
 > ! 	      /* start-frame_buffer gives offset from 
 > ! 	         the beginning of actual section.  */
 > ! 	      base_offset = curr_section_vma + start - frame_buffer;
 > ! 		      
 > ! 	      init_loc = read_encoded_pointer (abfd, &start, 
 > ! 	      			cie->addr_encoding, &flag_pcrel);
 > ! 	      
 > ! 	      if (flag_pcrel)
 > ! 		init_loc += base_offset;
 > ! 	      
 > ! 	      /* For relocatable objects we must add an offset telling
 > ! 	         where the section is actually mapped in the memory.  */
 > ! 	      init_loc += ANOFFSET (objfile->section_offsets, 
 > ! 				SECT_OFF_TEXT (objfile));
 > ! 	
 > ! 	      /* If we have both .debug_frame and .eh_frame present in 
 > ! 	         a file, we must eliminate duplicate FDEs. For now we'll 
 > ! 		 run through all entries in fde_chunks and check it one 
 > ! 		 by one. Perhaps in the future we can implement a faster 
 > ! 		 searching algorithm.  */
 > ! 	      /* eh_frame==2 indicates, that this file has an already 
 > ! 		 parsed .debug_frame too. When eh_frame==1 it means, that no
 > ! 		 .debug_frame is present and thus we don't need to check for
 > ! 		 duplicities. eh_frame==0 means, that we parse .debug_frame
 > ! 		 and don't need to care about duplicate FDEs, because
 > ! 		 .debug_frame is parsed first.  */
 > ! 	      for(i = 0; eh_frame == 2 && i < fde_chunks.elems; i++)
 > ! 	      {
 > ! 		      /* We assume that FDEs in .debug_frame and .eh_frame 
 > ! 		         have the same order (if they are present, of course).
 > ! 			 If we find a duplicate entry for one FDE and save
 > ! 			 it's index to last_dup_fde it's very likely, that 
 > ! 			 we'll find an entry for the following FDE right after 
 > ! 			 the previous one. Thus in many cases we'll run this 
 > ! 			 loop only once.  */
 > ! 		      last_dup_fde = (last_dup_fde + i) % fde_chunks.elems;
 > ! 		      if(fde_chunks.array[last_dup_fde]->initial_location 
 > ! 			      == init_loc)
 > ! 		      {
 > ! 			      dup=1;
 > ! 			      break;
 > ! 		      }
 >   	      }
 >   
 > ! 	      /* Allocate a new entry only if this FDE isn't a duplicate of
 > ! 	         something we have already seen.   */
 > ! 	      if(!dup)
 > ! 	      {
 > ! 		      fde_chunks_need_space ();
 > ! 		      fde = fde_unit_alloc ();
 >   
 > ! 		      fde_chunks.array[fde_chunks.elems++] = fde;
 > ! 		      
 > ! 		      fde->initial_location = init_loc;
 > ! 		      fde->address_range = read_encoded_pointer (abfd, &start,
 > ! 		      	cie->addr_encoding, NULL);
 > ! 
 > ! 		      fde->cie_ptr = cie;
 > ! 
 > ! 		      /* Here we intentionally ignore augmentation data
 > ! 		         from FDE, because we don't need them.  */
 > ! 		      if (cie->augmentation[0] == 'z')
 > ! 			  start += read_uleb128 (abfd, &start);
 > ! 
 > ! 		      fde->data = start;
 > ! 		      fde->data_length = block_end - start;
 > ! 	      }
 >   	    }
 >   	  start = block_end;
 >   	}
 > *************** dwarf2_build_frame_info (struct objfile 
 > *** 1525,1531 ****
 >   	     sizeof (struct fde_unit *), compare_fde_unit);
 >       }
 >   }
 > ! \f
 >   
 >   /* Return the frame address.  */
 >   CORE_ADDR
 > --- 1628,1655 ----
 >   	     sizeof (struct fde_unit *), compare_fde_unit);
 >       }
 >   }
 > ! 
 > ! /* We must parse both .debug_frame section and .eh_frame because 
 > !    not all frames must be present in both of these sections.  */
 > ! void
 > ! dwarf2_build_frame_info (struct objfile *objfile)
 > ! {
 > ! 	int eh_frame;
 > ! 
 > ! 	/* If we have .debug_frame then the parser is called with 
 > ! 	   eh_frame==0 for .debug_frame and eh_frame==2 for .eh_frame,
 > ! 	   otherwise it's only called once for .eh_frame with argument
 > ! 	   eh_frame==1  */
 > ! 	eh_frame = 0;
 > ! 
 > ! 	if (dwarf_frame_offset)
 > ! 		parse_frame_info (objfile, dwarf_frame_offset, 
 > ! 			dwarf_frame_size, eh_frame++);
 > ! 
 > ! 	if (dwarf_eh_frame_offset)
 > ! 		parse_frame_info (objfile, dwarf_eh_frame_offset, 
 > ! 			dwarf_eh_frame_size, eh_frame+1);
 > ! }
 >   
 >   /* Return the frame address.  */
 >   CORE_ADDR

Re: [RFA] dwarf2cfi.c improvements

[Michal Ludvig]

Elena Zannoni wrote:
> Michal Ludvig writes:
>  > 
>  > Well, this is the patch without unrelated message changes. Can I commit 
>  > it? It seems to be quite stable in our everyday use...
>  > 
> 
> Michal, sorry, not as is.  

No problem, I'll rework it.

> There are coding standard violations having to do with spaces before
> '(' and after ')' in casts. Also, it is better to not have multiple
> initializations in a single line, like this:
>  > !   char *start = NULL, *end = NULL, *frame_buffer = NULL;

Changed. I've reindented the whole file. I hope running 'indent 
dwarf2cfi.c' is enough.

> Conditional expressions are also flagged by the ARI.

What does 'AR Index' mean and how do I run the checker?

> Can I suggest that you split the pcrel changes into a separate patch?

Well, I wouldn't like to. When you start parsing .eh_frame you need a 
working pcrel, otherwise it is useless. But without parsing .eh_frame 
you can't verify whether it works, because .debug_frame doesn't use 
relative addressing. May I ask you to approve both at once, please?

 > About pcrel, I don't like the use of the flag like you have below.
 > Could you find some other way to do it w/o passing the address of the
 > flag around? Could you maybe change the function to return 0/1 and
 > have the current return value as an out parameter instead?

That is possible, but it would make read_encoded_pointer() behave 
differently from read_pointer(). I'll better introduce a new function 
like pointer_type() that would return an enum of possible types.

 > Or alternatively, could you have the function read_encoded_pointer
 > emit the warning itself?

In some cases PC-relative addressing is valid, but sometimes it is not 
(or at least is not used). The warning is there only once.

 > After all, none of
 > the other error/warning messages are that detailed, they don't print
 > the PC value. I noticed that in a couple of the calls to
 > read_encoded_pointer the code doesn't care about emitting the warning,
 > why? Or add a pc parameter for the purpose of the warning.

There are four occurences:
- In DW_CFA_set_loc I don't (yet) handle pcrel addressing, because I 
have never seen it in .*_frame. That's why I print a warning - If I'll 
ever see it I'll implement support for it.
- When reading pers_addr I don't care about the return value at all. I 
just need to advance the pointer to data.
- Init_loc reading takes care of pcrel addressing.
- fde->address_range is always an absolute value, but encoded in a 
pointer format. I don't care if it is in pcrel format or not - it's just 
a difference between two pointers.

There are no other calls to read_encoded_pointer() in dwarf2cfi.c

> About the eh_frame part of the patch.
> I don't really like the use of eh_frame as you have in dwarf2_build_frame_info.
> Could you try to figure out a simpler way to deal with the two cases?
> You could use an enumeration for eh_frame.

Well, yes, actually. But then I would have to change:
if(eh_frame) ...
to
if(frame == EH_ONLY || frame == EH_SECOND) ...

What about making local macros IS_DEBUG_FRAME(), IS_EH_FRAME(), 
IS_EH_FRAME_ONLY(), ...?

> Could you do 2 passes, one for each section?

Hm? I don't understand what you mean...

> I am not able to apply your patch, which I usually do, to try a test
> run. Is there anyway you can regenerate the patch? Maybe with a unified
> diff instead? (it's just that I find it easier to read)
 > I would like to give you more intelligent comments about the eh_frame
 > part, but I need to apply the patch first.

The cfi-huge5.diff was made against clean rev 1.7 of dwarf2cfi.c file. 
For me it applies cleanly against this revision.

Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* +420 2 9654 5373   * http://www.suse.cz

Re: [RFA] dwarf2cfi.c improvements

[Daniel Jacobowitz]

On Tue, Jun 11, 2002 at 05:28:27PM +0200, Michal Ludvig wrote:
> Elena Zannoni wrote:
> >Michal Ludvig writes:
> > > 
> > > Well, this is the patch without unrelated message changes. Can I commit 
> > > it? It seems to be quite stable in our everyday use...
> > > 
> >
> >Michal, sorry, not as is.  
> 
> No problem, I'll rework it.
> 
> >There are coding standard violations having to do with spaces before
> >'(' and after ')' in casts. Also, it is better to not have multiple
> >initializations in a single line, like this:
> > > !   char *start = NULL, *end = NULL, *frame_buffer = NULL;
> 
> Changed. I've reindented the whole file. I hope running 'indent 
> dwarf2cfi.c' is enough.

Try gdb_indent.sh in the source directory.


-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] dwarf2cfi.c improvements

[Elena Zannoni]

Michal Ludvig writes:
 > Elena Zannoni wrote:
 > > Michal Ludvig writes:
 > >  > 
 > >  > Well, this is the patch without unrelated message changes. Can I commit 
 > >  > it? It seems to be quite stable in our everyday use...
 > >  > 
 > > 
 > > Michal, sorry, not as is.  
 > 
 > No problem, I'll rework it.
 > 
 > > There are coding standard violations having to do with spaces before
 > > '(' and after ')' in casts. Also, it is better to not have multiple
 > > initializations in a single line, like this:
 > >  > !   char *start = NULL, *end = NULL, *frame_buffer = NULL;
 > 
 > Changed. I've reindented the whole file. I hope running 'indent 
 > dwarf2cfi.c' is enough.
 > 

No, you should run gdb_indent.sh instead. That is the approved format.

 > > Conditional expressions are also flagged by the ARI.
 > 
 > What does 'AR Index' mean and how do I run the checker?
 > 

It is run every day by a script. It detects a set of generally
deprecated conventions and usages. More often than not it also
uncovers bugs.  I don't think you can run it yourself. But you can
look at the categories listed and have a feel for what is deprecated.
http://sources.redhat.com/gdb/ari/

 > > Can I suggest that you split the pcrel changes into a separate patch?
 > 
 > Well, I wouldn't like to. When you start parsing .eh_frame you need a 
 > working pcrel, otherwise it is useless. But without parsing .eh_frame 
 > you can't verify whether it works, because .debug_frame doesn't use 
 > relative addressing. May I ask you to approve both at once, please?

Sigh. OK, but it will take a bit longer, I am afraid. 

 > 
 >  > About pcrel, I don't like the use of the flag like you have below.
 >  > Could you find some other way to do it w/o passing the address of the
 >  > flag around? Could you maybe change the function to return 0/1 and
 >  > have the current return value as an out parameter instead?
 > 
 > That is possible, but it would make read_encoded_pointer() behave 
 > differently from read_pointer(). I'll better introduce a new function 
 > like pointer_type() that would return an enum of possible types.

See below for a different suggestion.

 > 
 >  > Or alternatively, could you have the function read_encoded_pointer
 >  > emit the warning itself?
 > 
 > In some cases PC-relative addressing is valid, but sometimes it is not 
 > (or at least is not used). The warning is there only once.
 > 
 >  > After all, none of
 >  > the other error/warning messages are that detailed, they don't print
 >  > the PC value. I noticed that in a couple of the calls to
 >  > read_encoded_pointer the code doesn't care about emitting the warning,
 >  > why? Or add a pc parameter for the purpose of the warning.
 > 
 > There are four occurences:
 > - In DW_CFA_set_loc I don't (yet) handle pcrel addressing, because I 
 > have never seen it in .*_frame. That's why I print a warning - If I'll 
 > ever see it I'll implement support for it.

OK. But still, the warning should come from inside the function
itself.  I was thinking you needed to do it outside, to have the
fs->pc available to be printed in the warning, but that value actually
comes from inside the function itself, so it doesn't need to be separate.

 > - When reading pers_addr I don't care about the return value at all. I 
 > just need to advance the pointer to data.

Actually, do you need to call this function at all, if all you need to
do is eat up some data? This is really overloading the function.
Could you just call one of the read_* functions?  But I see that gcc does
some similar trick. BTW that function deals with alignment and the gdb
version doesn't (yet?).

 > - Init_loc reading takes care of pcrel addressing.

Yes, the bit that does it though, was inside read_encoded_pointer, and
now it's not, and you also need to do an additional check, so I  would
prefer doing it as it was before. 
Could you instead add a new parmeter *base* instead of the flag?  In
the DW_CFA_set_loc case, base would be NULL while here it would be
base_offset, so you could still distinguish the two cases.

 > - fde->address_range is always an absolute value, but encoded in a 
 > pointer format. I don't care if it is in pcrel format or not - it's just 
 > a difference between two pointers.
 > 

So that would work anyway, right?

 > There are no other calls to read_encoded_pointer() in dwarf2cfi.c
 > 

I don't undersand this change:
*** 530,541 ****
      }
  
    if (ret != 0)
!     switch (encoding & 0xf0)
        {
        case DW_EH_PE_absptr:
        break;
        case DW_EH_PE_pcrel:
!       ret += (CORE_ADDR) *p;
        break;
        case DW_EH_PE_textrel:
        case DW_EH_PE_datarel:
--- 531,543 ----
      }
  
    if (ret != 0)
!   {
!     switch (encoding & 0x70)
        {
        case DW_EH_PE_absptr:
        break;
        case DW_EH_PE_pcrel:
!       if (flag_pcrel) *flag_pcrel = 1;
        break;
        case DW_EH_PE_textrel:
        case DW_EH_PE_datarel:
*************** read_encoded_pointer (bfd *abfd, char **
*** 544,549 ****
--- 546,555 ----
        internal_error (__FILE__, __LINE__,
                        "read_encoded_pointer: unknown pointer encoding");
        }
+     
+     if (encoding & DW_EH_PE_indirect)
+       warning ("CFI: Unsupported pointer encoding: DW_EH_PE_indirect");
+   }
  
    return ret;
  }

Why are you changing the mask from 0xf0 to 0x70 in the switch to avoid
catching DW_EH_PE_indirect? Could you just add it to the case statement?



 > > About the eh_frame part of the patch.
 > > I don't really like the use of eh_frame as you have in dwarf2_build_frame_info.
 > > Could you try to figure out a simpler way to deal with the two cases?
 > > You could use an enumeration for eh_frame.
 > 
 > Well, yes, actually. But then I would have to change:
 > if(eh_frame) ...
 > to
 > if(frame == EH_ONLY || frame == EH_SECOND) ...
 > 

That is the idea, yes. It would be much more understandable to the
reader.  

 > What about making local macros IS_DEBUG_FRAME(), IS_EH_FRAME(), 
 > IS_EH_FRAME_ONLY(), ...?
 > 

How would those behave different from enums? If I am understanding your
suggestion correctly, that is (macros are usually not introduced
unless absolutely necessary).

 > > Could you do 2 passes, one for each section?
 > 
 > Hm? I don't understand what you mean...
 > 

Never mind, there is too much similarity between the 2 cases.

 > > I am not able to apply your patch, which I usually do, to try a test
 > > run. Is there anyway you can regenerate the patch? Maybe with a unified
 > > diff instead? (it's just that I find it easier to read)
 >  > I would like to give you more intelligent comments about the eh_frame
 >  > part, but I need to apply the patch first.
 > 
 > The cfi-huge5.diff was made against clean rev 1.7 of dwarf2cfi.c file. 
 > For me it applies cleanly against this revision.
 > 

Could you send me a unified diff? (don't need to send it to the list
just to me). I'd appreciate that.

thanks
Elena

Re: [RFA] dwarf2cfi.c improvements

[Elena Zannoni]

Michal Ludvig writes:
 > Elena Zannoni wrote:
 > 
 > > Could you send me a unified diff? (don't need to send it to the list
 > > just to me). I'd appreciate that.
 > 
 > Yes. Included. It's against cvs rev 1.9.
 > 

Since the diff had substantial changes from the previous one, I am
copying it to the list as well.

Elena


 > Index: dwarf2cfi.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/dwarf2cfi.c,v
 > retrieving revision 1.9
 > diff -u -r1.9 dwarf2cfi.c
 > --- dwarf2cfi.c	11 Jun 2002 08:45:05 -0000	1.9
 > +++ dwarf2cfi.c	13 Jun 2002 18:27:18 -0000
 > @@ -34,7 +34,7 @@
 >     Frame Descriptors.  */
 >  struct cie_unit
 >  {
 > -  /* Offset of this unit in dwarf_frame_buffer.  */
 > +  /* Offset of this unit in .debug_frame or .eh_frame.  */
 >    ULONGEST offset;
 >  
 >    /* A null-terminated string that identifies the augmentation to this CIE or
 > @@ -176,6 +176,14 @@
 >    struct objfile *objfile;
 >  };
 >  
 > +enum ptr_encoding { 
 > +  PE_absptr = DW_EH_PE_absptr,
 > +  PE_pcrel = DW_EH_PE_pcrel,
 > +  PE_textrel = DW_EH_PE_textrel,
 > +  PE_datarel = DW_EH_PE_datarel,
 > +  PE_funcrel = DW_EH_PE_funcrel
 > +};
 > +
 >  #define UNWIND_CONTEXT(fi) ((struct context *) (fi->context))
 >  \f
 >  
 > @@ -188,8 +196,6 @@
 >  extern unsigned int dwarf_frame_size;
 >  extern file_ptr dwarf_eh_frame_offset;
 >  extern unsigned int dwarf_eh_frame_size;
 > -
 > -static char *dwarf_frame_buffer;
 >  \f
 >  
 >  extern char *dwarf2_read_section (struct objfile *objfile, file_ptr offset,
 > @@ -219,6 +225,7 @@
 >  static CORE_ADDR read_pointer (bfd * abfd, char **p);
 >  static CORE_ADDR read_encoded_pointer (bfd * abfd, char **p,
 >  				       unsigned char encoding);
 > +static enum ptr_encoding pointer_encoding (unsigned char encoding);
 >  
 >  static LONGEST read_initial_length (bfd * abfd, char *buf, int *bytes_read);
 >  static ULONGEST read_length (bfd * abfd, char *buf, int *bytes_read,
 > @@ -494,6 +501,9 @@
 >      }
 >  }
 >  
 > +/* This functions only reads appropriate amount of data from *p 
 > + * and returns the resulting value. Calling function must handle
 > + * different encoding possibilities itself!  */
 >  static CORE_ADDR
 >  read_encoded_pointer (bfd * abfd, char **p, unsigned char encoding)
 >  {
 > @@ -537,22 +547,30 @@
 >  		      "read_encoded_pointer: unknown pointer encoding");
 >      }
 >  
 > -  if (ret != 0)
 > -    switch (encoding & 0xf0)
 > -      {
 > -      case DW_EH_PE_absptr:
 > -	break;
 > -      case DW_EH_PE_pcrel:
 > -	ret += (CORE_ADDR) * p;
 > -	break;
 > -      case DW_EH_PE_textrel:
 > -      case DW_EH_PE_datarel:
 > -      case DW_EH_PE_funcrel:
 > -      default:
 > -	internal_error (__FILE__, __LINE__,
 > -			"read_encoded_pointer: unknown pointer encoding");
 > -      }
 > +  return ret;
 > +}
 > +
 > +enum ptr_encoding
 > +pointer_encoding (unsigned char encoding)
 > +{
 > +  int ret;
 >  
 > +  if (encoding & DW_EH_PE_indirect)
 > +    warning ("CFI: Unsupported pointer encoding: DW_EH_PE_indirect");
 > +  
 > +  switch (encoding & 0x70)
 > +    {
 > +    case DW_EH_PE_absptr:
 > +    case DW_EH_PE_pcrel:
 > +    case DW_EH_PE_textrel:
 > +    case DW_EH_PE_datarel:
 > +    case DW_EH_PE_funcrel:
 > +      ret = encoding & 0x70;
 > +      break;
 > +    default:
 > +      internal_error (__FILE__, __LINE__,
 > +    		  "read_encoded_pointer: unknown pointer encoding");
 > +    }
 >    return ret;
 >  }
 >  
 > @@ -627,6 +645,10 @@
 >  	  case DW_CFA_set_loc:
 >  	    fs->pc = read_encoded_pointer (objfile->obfd, &insn_ptr,
 >  					   fs->addr_encoding);
 > +	    
 > +	    if (pointer_encoding (fs->addr_encoding) != PE_absptr)
 > +		warning ("CFI: DW_CFA_set_loc uses relative addressing");
 > +	    
 >  	    break;
 >  
 >  	  case DW_CFA_advance_loc1:
 > @@ -1381,38 +1403,31 @@
 >  
 >  /*  Build the cie_chunks and fde_chunks tables from informations
 >      in .debug_frame section.  */
 > -void
 > -dwarf2_build_frame_info (struct objfile *objfile)
 > +static void
 > +parse_frame_info (struct objfile *objfile, file_ptr frame_offset,
 > +		  unsigned int frame_size, int eh_frame)
 >  {
 >    bfd *abfd = objfile->obfd;
 > +  asection *curr_section_ptr;
 >    char *start = NULL;
 >    char *end = NULL;
 > -  int from_eh = 0;
 > +  char *frame_buffer = NULL;
 > +  char *curr_section_name, *aug_data;
 > +  struct cie_unit *last_cie = NULL;
 > +  int last_dup_fde = 0, aug_len, i;
 > +  CORE_ADDR curr_section_vma = 0;
 >  
 >    unwind_tmp_obstack_init ();
 >  
 > -  dwarf_frame_buffer = 0;
 > -
 > -  if (dwarf_frame_offset)
 > -    {
 > -      dwarf_frame_buffer = dwarf2_read_section (objfile,
 > -						dwarf_frame_offset,
 > -						dwarf_frame_size);
 > -
 > -      start = dwarf_frame_buffer;
 > -      end = dwarf_frame_buffer + dwarf_frame_size;
 > -    }
 > -  else if (dwarf_eh_frame_offset)
 > -    {
 > -      dwarf_frame_buffer = dwarf2_read_section (objfile,
 > -						dwarf_eh_frame_offset,
 > -						dwarf_eh_frame_size);
 > +  frame_buffer = dwarf2_read_section (objfile, frame_offset, frame_size);
 >  
 > -      start = dwarf_frame_buffer;
 > -      end = dwarf_frame_buffer + dwarf_eh_frame_size;
 > +  start = frame_buffer;
 > +  end = frame_buffer + frame_size;
 >  
 > -      from_eh = 1;
 > -    }
 > +  curr_section_name = eh_frame ? ".eh_frame" : ".debug_frame";
 > +  curr_section_ptr = bfd_get_section_by_name (abfd, curr_section_name);
 > +  if (curr_section_ptr)
 > +    curr_section_vma = curr_section_ptr->vma;
 >  
 >    if (start)
 >      {
 > @@ -1420,9 +1435,8 @@
 >  	{
 >  	  unsigned long length;
 >  	  ULONGEST cie_id;
 > -	  ULONGEST unit_offset = start - dwarf_frame_buffer;
 > -	  int bytes_read;
 > -	  int dwarf64;
 > +	  ULONGEST unit_offset = start - frame_buffer;
 > +	  int bytes_read, dwarf64, flag_pcrel;
 >  	  char *block_end;
 >  
 >  	  length = read_initial_length (abfd, start, &bytes_read);
 > @@ -1430,10 +1444,16 @@
 >  	  dwarf64 = (bytes_read == 12);
 >  	  block_end = start + length;
 >  
 > +	  if (length == 0)
 > +	    {
 > +	      start = block_end;
 > +	      continue;
 > +	    }
 > +
 >  	  cie_id = read_length (abfd, start, &bytes_read, dwarf64);
 >  	  start += bytes_read;
 >  
 > -	  if ((from_eh && cie_id == 0) || is_cie (cie_id, dwarf64))
 > +	  if ((eh_frame && cie_id == 0) || is_cie (cie_id, dwarf64))
 >  	    {
 >  	      struct cie_unit *cie = cie_unit_alloc ();
 >  	      char *aug;
 > @@ -1449,87 +1469,185 @@
 >  	      start++;		/* version */
 >  
 >  	      cie->augmentation = aug = start;
 > -	      while (*start)
 > -		start++;
 > -	      start++;		/* skip past NUL */
 > +	      while (*start++);	/* Skips last NULL as well */
 >  
 >  	      cie->code_align = read_uleb128 (abfd, &start);
 >  	      cie->data_align = read_sleb128 (abfd, &start);
 >  	      cie->ra = read_1u (abfd, &start);
 >  
 > +	      /* Augmentation:
 > +	         z      Indicates that a uleb128 is present to size the
 > +	         augmentation section.
 > +	         L      Indicates the encoding (and thus presence) of
 > +	         an LSDA pointer in the FDE augmentation.
 > +	         R      Indicates a non-default pointer encoding for
 > +	         FDE code pointers.
 > +	         P      Indicates the presence of an encoding + language
 > +	         personality routine in the CIE augmentation.
 > +
 > +	         [This info comes from GCC's dwarf2out.c]
 > +	       */
 >  	      if (*aug == 'z')
 >  		{
 > -		  int xtra = read_uleb128 (abfd, &start);
 > -		  start += xtra;
 > +		  aug_len = read_uleb128 (abfd, &start);
 > +		  aug_data = start;
 > +		  start += aug_len;
 >  		  ++aug;
 >  		}
 >  
 > +	      cie->data = start;
 > +	      cie->data_length = block_end - cie->data;
 > +
 >  	      while (*aug != '\0')
 >  		{
 >  		  if (aug[0] == 'e' && aug[1] == 'h')
 >  		    {
 > -		      start += sizeof (void *);
 > -		      aug += 2;
 > +		      aug_data += sizeof (void *);
 > +		      aug++;
 >  		    }
 >  		  else if (aug[0] == 'R')
 > +		    cie->addr_encoding = *aug_data++;
 > +		  else if (aug[0] == 'P')
 >  		    {
 > -		      cie->addr_encoding = *start++;
 > -		      aug += 1;
 > +		      CORE_ADDR pers_addr;
 > +		      int pers_addr_enc;
 > +
 > +		      pers_addr_enc = *aug_data++;
 > +		      /* We don't need pers_addr value and so we 
 > +		         don't care about it's encoding.  */
 > +		      pers_addr = read_encoded_pointer (abfd, &aug_data,
 > +							pers_addr_enc);
 >  		    }
 > -		  else if (aug[0] == 'P')
 > +		  else if (aug[0] == 'L' && eh_frame)
 >  		    {
 > -		      CORE_ADDR ptr;
 > -		      ptr = read_encoded_pointer (abfd, &start,
 > -						  cie->addr_encoding);
 > -		      aug += 1;
 > +		      int lsda_addr_enc;
 > +
 > +		      /* Perhaps we should save this to CIE for later use?
 > +		         Do we need it for something in GDB?  */
 > +		      lsda_addr_enc = *aug_data++;
 >  		    }
 >  		  else
 > -		    warning ("%s(): unknown augmentation", __func__);
 > +		    warning ("CFI warning: unknown augmentation \"%c\""
 > +			     " in \"%s\" of\n"
 > +			     "\t%s", aug[0], curr_section_name,
 > +			     objfile->name);
 > +		  aug++;
 >  		}
 >  
 > -	      cie->data = start;
 > -	      cie->data_length = block_end - start;
 > +	      last_cie = cie;
 >  	    }
 >  	  else
 >  	    {
 >  	      struct fde_unit *fde;
 >  	      struct cie_unit *cie;
 > +	      int dup = 0;
 > +	      CORE_ADDR init_loc;
 >  
 > -	      fde_chunks_need_space ();
 > -	      fde = fde_unit_alloc ();
 > +	      /* We assume that debug_frame is in order 
 > +	         CIE,FDE,CIE,FDE,FDE,...  and thus the CIE for this FDE
 > +	         should be stored in last_cie pointer. If not, we'll 
 > +	         try to find it by the older way.  */
 > +	      if (last_cie)
 > +		cie = last_cie;
 > +	      else
 > +		{
 > +		  warning ("CFI: last_cie == NULL. "
 > +			   "Perhaps a malformed %s section in '%s'...?\n",
 > +			   curr_section_name, objfile->name);
 > +
 > +		  cie = cie_chunks;
 > +		  while (cie)
 > +		    {
 > +		      if (cie->objfile == objfile)
 > +			{
 > +			  if (eh_frame &&
 > +			      (cie->offset ==
 > +			       (unit_offset + bytes_read - cie_id)))
 > +			    break;
 > +			  if (!eh_frame && (cie->offset == cie_id))
 > +			    break;
 > +			}
 >  
 > -	      fde_chunks.array[fde_chunks.elems++] = fde;
 > +		      cie = cie->next;
 > +		    }
 > +		  if (!cie)
 > +		    error ("CFI: can't find CIE pointer");
 > +		}
 >  
 > -	      fde->initial_location = read_pointer (abfd, &start)
 > -		+ ANOFFSET (objfile->section_offsets,
 > -			    SECT_OFF_TEXT (objfile));
 > -	      fde->address_range = read_pointer (abfd, &start);
 > +	      init_loc = read_encoded_pointer (abfd, &start,
 > +					       cie->addr_encoding);
 >  
 > -	      cie = cie_chunks;
 > -	      while (cie)
 > -		{
 > -		  if (cie->objfile == objfile)
 > -		    {
 > -		      if (from_eh
 > -			  && (cie->offset ==
 > -			      (unit_offset + bytes_read - cie_id)))
 > +	      switch (pointer_encoding (cie->addr_encoding))
 > +	      {
 > +		case PE_absptr:
 >  			break;
 > -		      if (!from_eh && (cie->offset == cie_id))
 > +		case PE_pcrel:
 > +			/* start-frame_buffer gives offset from 
 > +		           the beginning of actual section.  */
 > +			init_loc += curr_section_vma + start - frame_buffer;
 >  			break;
 > -		    }
 > +		default:
 > +			warning ("CFI: Unsupported pointer encoding\n");
 > +	      }
 >  
 > -		  cie = cie->next;
 > +	      /* For relocatable objects we must add an offset telling
 > +	         where the section is actually mapped in the memory.  */
 > +	      init_loc += ANOFFSET (objfile->section_offsets,
 > +				    SECT_OFF_TEXT (objfile));
 > +
 > +	      /* If we have both .debug_frame and .eh_frame present in 
 > +	         a file, we must eliminate duplicate FDEs. For now we'll 
 > +	         run through all entries in fde_chunks and check it one 
 > +	         by one. Perhaps in the future we can implement a faster 
 > +	         searching algorithm.  */
 > +	      /* eh_frame==2 indicates, that this file has an already 
 > +	         parsed .debug_frame too. When eh_frame==1 it means, that no
 > +	         .debug_frame is present and thus we don't need to check for
 > +	         duplicities. eh_frame==0 means, that we parse .debug_frame
 > +	         and don't need to care about duplicate FDEs, because
 > +	         .debug_frame is parsed first.  */
 > +	      for (i = 0; eh_frame == 2 && i < fde_chunks.elems; i++)
 > +		{
 > +		  /* We assume that FDEs in .debug_frame and .eh_frame 
 > +		     have the same order (if they are present, of course).
 > +		     If we find a duplicate entry for one FDE and save
 > +		     it's index to last_dup_fde it's very likely, that 
 > +		     we'll find an entry for the following FDE right after 
 > +		     the previous one. Thus in many cases we'll run this 
 > +		     loop only once.  */
 > +		  last_dup_fde = (last_dup_fde + i) % fde_chunks.elems;
 > +		  if (fde_chunks.array[last_dup_fde]->initial_location
 > +		      == init_loc)
 > +		    {
 > +		      dup = 1;
 > +		      break;
 > +		    }
 >  		}
 >  
 > -	      if (!cie)
 > -		error ("%s(): can't find CIE pointer", __func__);
 > -	      fde->cie_ptr = cie;
 > +	      /* Allocate a new entry only if this FDE isn't a duplicate of
 > +	         something we have already seen.   */
 > +	      if (!dup)
 > +		{
 > +		  fde_chunks_need_space ();
 > +		  fde = fde_unit_alloc ();
 > +
 > +		  fde_chunks.array[fde_chunks.elems++] = fde;
 > +
 > +		  fde->initial_location = init_loc;
 > +		  fde->address_range = read_encoded_pointer (abfd, &start,
 > +							     cie->
 > +							     addr_encoding);
 > +
 > +		  fde->cie_ptr = cie;
 >  
 > -	      if (cie->augmentation[0] == 'z')
 > -		read_uleb128 (abfd, &start);
 > +		  /* Here we intentionally ignore augmentation data
 > +		     from FDE, because we don't need them.  */
 > +		  if (cie->augmentation[0] == 'z')
 > +		    start += read_uleb128 (abfd, &start);
 >  
 > -	      fde->data = start;
 > -	      fde->data_length = block_end - start;
 > +		  fde->data = start;
 > +		  fde->data_length = block_end - start;
 > +		}
 >  	    }
 >  	  start = block_end;
 >  	}
 > @@ -1537,7 +1655,30 @@
 >  	     sizeof (struct fde_unit *), compare_fde_unit);
 >      }
 >  }
 > -\f
 > +
 > +/* We must parse both .debug_frame section and .eh_frame because 
 > +   not all frames must be present in both of these sections.  */
 > +void
 > +dwarf2_build_frame_info (struct objfile *objfile)
 > +{
 > +  int after_debug_frame=0;
 > +
 > +  /* If we have .debug_frame then the parser is called with 
 > +     eh_frame==0 for .debug_frame and eh_frame==2 for .eh_frame,
 > +     otherwise it's only called once for .eh_frame with argument
 > +     eh_frame==1  */
 > +
 > +  if (dwarf_frame_offset)
 > +  {
 > +    parse_frame_info (objfile, dwarf_frame_offset,
 > +		      dwarf_frame_size, 0 /* = debug_frame */);
 > +    after_debug_frame = 1;
 > +  }
 > +
 > +  if (dwarf_eh_frame_offset)
 > +    parse_frame_info (objfile, dwarf_eh_frame_offset, dwarf_eh_frame_size, 
 > +		      1 /* = eh_frame */ + after_debug_frame);
 > +}
 >  
 >  /* Return the frame address.  */
 >  CORE_ADDR

Re: [RFA] dwarf2cfi.c improvements

[Elena Zannoni]

Michal Ludvig writes:
 > Elena Zannoni wrote:
 > > Michal Ludvig writes:
 > >  >  > Or alternatively, could you have the function read_encoded_pointer
 > >  >  > emit the warning itself?
 > >  > 
 > >  > In some cases PC-relative addressing is valid, but sometimes it is not 
 > >  > (or at least is not used). The warning is there only once.
 > >  > 
 > >  >  > After all, none of
 > >  >  > the other error/warning messages are that detailed, they don't print
 > >  >  > the PC value. I noticed that in a couple of the calls to
 > >  >  > read_encoded_pointer the code doesn't care about emitting the warning,
 > >  >  > why? Or add a pc parameter for the purpose of the warning.
 > >  > 
 > >  > There are four occurences:
 > >  > - In DW_CFA_set_loc I don't (yet) handle pcrel addressing, because I 
 > >  > have never seen it in .*_frame. That's why I print a warning - If I'll 
 > >  > ever see it I'll implement support for it.
 > > 
 > > OK. But still, the warning should come from inside the function
 > > itself. 
 > 
 > I have introducend new function pointer_encoding() that the function 
 > calling read_encoded_pointer() may invoke to see what was returned and 
 > how to handle it. I'm convinced, that it's the caller's responsibility 
 > to deal with different PE's (pointer encodings), because sometimes some 
 > of them are valid, sometimes not. If I'd just pass base_addr into the 
 > read_enc_ptr() how would I know whether it was handled correctly. There 
 > might be a situation, that the pointer would be encoded either in pcrel 
 > or in funcrel format - then I should decide afterwards how to compute 
 > the base address, shouldn't I?
 > 

Much better. This makes the code a bit easier to understand. The
previous version of the function was trying to do too much.

 > > I was thinking you needed to do it outside, to have the
 > > fs->pc available to be printed in the warning, but that value actually
 > > comes from inside the function itself, so it doesn't need to be separate.
 > > 
 > >  > - When reading pers_addr I don't care about the return value at all. I 
 > >  > just need to advance the pointer to data.
 > > 
 > > Actually, do you need to call this function at all, if all you need to
 > > do is eat up some data? This is really overloading the function.
 > > Could you just call one of the read_* functions? 
 > 
 > No, because it depends on PE how much data should I eat up. I can have 
 > another function that would give me the number of bytes occupied by a 
 > given PE, but I don't believe it's a good idea. It's not called that 
 > often, that it could speed things up.
 > 

No need anymore, with the pointer_encoding function.

 > > But I see that gcc does
 > > some similar trick. BTW that function deals with alignment and the gdb
 > > version doesn't (yet?).
 > 
 > Hmm I didn't notice. I'll look at it. But it works as it is.
 > 

Keep it in mind if you see strange behaviors.

 > >  > - Init_loc reading takes care of pcrel addressing.
 > > 
 > > Yes, the bit that does it though, was inside read_encoded_pointer, and
 > > now it's not, and you also need to do an additional check, so I  would
 > > prefer doing it as it was before. 
 > 
 > It wasn't correct, anyway. I have written my reasons against this 
 > approach above.
 > 
 > > Could you instead add a new parmeter *base* instead of the flag?  In
 > > the DW_CFA_set_loc case, base would be NULL while here it would be
 > > base_offset, so you could still distinguish the two cases.
 > 
 > How can I know how to compute base_addr when I don't know whether the 
 > encoding is pcrel, textrel, funcrel, ...? I know these aren't used _now_ 
 > but at least hypothetically are possible.
 > 

Right. OK.

 > >  > - fde->address_range is always an absolute value, but encoded in a 
 > >  > pointer format. I don't care if it is in pcrel format or not - it's just 
 > >  > a difference between two pointers.
 > > 
 > > So that would work anyway, right?
 > 
 > I must only know in how many bytes it's encoded so that I advance the 
 > data pointer correctly. The value itself, either if encoded as pcrel, is 
 > always absolute. At least I hope so :-)
 > 
 > 
 > > I don't undersand this change:
 > > *** 530,541 ****
 > >       }
 > >   
 > >     if (ret != 0)
 > > !     switch (encoding & 0xf0)
 > >         {
 > >         case DW_EH_PE_absptr:
 > >         break;
 > >         case DW_EH_PE_pcrel:
 > > !       ret += (CORE_ADDR) *p;
 > >         break;
 > >         case DW_EH_PE_textrel:
 > >         case DW_EH_PE_datarel:
 > > --- 531,543 ----
 > >       }
 > >   
 > >     if (ret != 0)
 > > !   {
 > > !     switch (encoding & 0x70)
 > >         {
 > >         case DW_EH_PE_absptr:
 > >         break;
 > >         case DW_EH_PE_pcrel:
 > > !       if (flag_pcrel) *flag_pcrel = 1;
 > >         break;
 > >         case DW_EH_PE_textrel:
 > >         case DW_EH_PE_datarel:
 > > *************** read_encoded_pointer (bfd *abfd, char **
 > > *** 544,549 ****
 > > --- 546,555 ----
 > >         internal_error (__FILE__, __LINE__,
 > >                         "read_encoded_pointer: unknown pointer encoding");
 > >         }
 > > +     
 > > +     if (encoding & DW_EH_PE_indirect)
 > > +       warning ("CFI: Unsupported pointer encoding: DW_EH_PE_indirect");
 > > +   }
 > >   
 > >     return ret;
 > >   }
 > > 
 > > Why are you changing the mask from 0xf0 to 0x70 in the switch to avoid
 > > catching DW_EH_PE_indirect? Could you just add it to the case statement?
 > 
 > I can't because DW_EH_PE_indirect is independent on DW_EH_PE_*rel. Both 
 > PE_pcrel and PE_pcrel_indirect are possible. In fact, encoding variable 
 > contains three flags: size, encoding and (in)direct flag.
 > 

Ah, I see. 3 masks for 3 different things. Is there any way you can
add a comment about this?

 > All of them must be handled separately.
 > 
 > >  > > About the eh_frame part of the patch.
 > >  > > I don't really like the use of eh_frame as you have in dwarf2_build_frame_info.
 > >  > > Could you try to figure out a simpler way to deal with the two cases?
 > >  > > You could use an enumeration for eh_frame.
 > >  > 
 > >  > Well, yes, actually. But then I would have to change:
 > >  > if(eh_frame) ...
 > >  > to
 > >  > if(frame == EH_ONLY || frame == EH_SECOND) ...
 > >  > 
 > > 
 > > That is the idea, yes. It would be much more understandable to the
 > > reader.  
 > 
 > I have changed computing of this parameter in dwarf2_build_frame_info so 
 > that it is more readable now, but I'd leave 'eh_frame' as it is, because 
 > in most cases I don't care whether it's 1 or 2. The hack with 
 > eh_frame==2 is there just for speeding up things when looking for 
 > duplicate FDEs. I eh_frame is the only section parsed, then I don't have 
 > to be afraid that we already have a given FDE and don't have to check it 
 > at all.
 > 

Could you add a comment explaining the differences between the
eh_frame section and the debug_frame section? Only a couple, as far as
I know.

 > >  > What about making local macros IS_DEBUG_FRAME(), IS_EH_FRAME(), 
 > >  > IS_EH_FRAME_ONLY(), ...?
 > >  > 
 > > 
 > > How would those behave different from enums? If I am understanding your
 > > suggestion correctly, that is (macros are usually not introduced
 > > unless absolutely necessary).
 > 
 > IS_EH_FRAME would be true for both possible cases with eh_frame.
 > As I already said - usually I don't care whether it's parsed first or 
 > after debug_frame.
 > 
 > > Could you send me a unified diff? (don't need to send it to the list
 > > just to me). I'd appreciate that.
 > 
 > Yes. Included. It's against cvs rev 1.9.
 > 

A few little things:

 > +
 > +enum ptr_encoding
 > +pointer_encoding (unsigned char encoding)
 > +{
 > +  int ret;
 >  
 > +  if (encoding & DW_EH_PE_indirect)
 > +    warning ("CFI: Unsupported pointer encoding: DW_EH_PE_indirect");
 > +  
 > +  switch (encoding & 0x70)
 > +    {
 > +    case DW_EH_PE_absptr:
 > +    case DW_EH_PE_pcrel:
 > +    case DW_EH_PE_textrel:
 > +    case DW_EH_PE_datarel:
 > +    case DW_EH_PE_funcrel:
 > +      ret = encoding & 0x70;
 > +      break;
 > +    default:
 > +      internal_error (__FILE__, __LINE__,
 > +    		  "read_encoded_pointer: unknown pointer encoding");


The function name should be pointer_encoding.



 > +static void
 > +parse_frame_info (struct objfile *objfile, file_ptr frame_offset,
 > +		  unsigned int frame_size, int eh_frame)
 >  {
 >    bfd *abfd = objfile->obfd;
 > +  asection *curr_section_ptr;
 >    char *start = NULL;
 >    char *end = NULL;
 > -  int from_eh = 0;
 > +  char *frame_buffer = NULL;
 > +  char *curr_section_name, *aug_data;
 > +  struct cie_unit *last_cie = NULL;
 > +  int last_dup_fde = 0, aug_len, i;

Please put these on separate lines.



 > +			  if (eh_frame &&
 > +			      (cie->offset ==
 > +			       (unit_offset + bytes_read - cie_id)))

Please reformat this line.



 > +	      for (i = 0; eh_frame == 2 && i < fde_chunks.elems; i++)

Could you include the for loop in an if statement, instead of adding the
eh_frame==2 test?


 > +  int after_debug_frame=0;

Spaces.


ChangeLog?

Other than that approved.

Elena

Re: [RFA] dwarf2cfi.c improvements

[Michal Ludvig]

Elena Zannoni wrote:
 > Michal Ludvig writes:
 >  >  > Or alternatively, could you have the function read_encoded_pointer
 >  >  > emit the warning itself?
 >  >
 >  > In some cases PC-relative addressing is valid, but sometimes it is 
not
 >  > (or at least is not used). The warning is there only once.
 >  >
 >  >  > After all, none of
 >  >  > the other error/warning messages are that detailed, they don't 
print
 >  >  > the PC value. I noticed that in a couple of the calls to
 >  >  > read_encoded_pointer the code doesn't care about emitting the 
warning,
 >  >  > why? Or add a pc parameter for the purpose of the warning.
 >  >
 >  > There are four occurences:
 >  > - In DW_CFA_set_loc I don't (yet) handle pcrel addressing, because I
 >  > have never seen it in .*_frame. That's why I print a warning - If 
I'll
 >  > ever see it I'll implement support for it.
 >
 > OK. But still, the warning should come from inside the function
 > itself.

I have introducend new function pointer_encoding() that the function
calling read_encoded_pointer() may invoke to see what was returned and
how to handle it. I'm convinced, that it's the caller's responsibility
to deal with different PE's (pointer encodings), because sometimes some
of them are valid, sometimes not. If I'd just pass base_addr into the
read_enc_ptr() how would I know whether it was handled correctly. There
might be a situation, that the pointer would be encoded either in pcrel
or in funcrel format - then I should decide afterwards how to compute
the base address, shouldn't I?

 > I was thinking you needed to do it outside, to have the
 > fs->pc available to be printed in the warning, but that value actually
 > comes from inside the function itself, so it doesn't need to be separate.
 >
 >  > - When reading pers_addr I don't care about the return value at 
all. I
 >  > just need to advance the pointer to data.
 >
 > Actually, do you need to call this function at all, if all you need to
 > do is eat up some data? This is really overloading the function.
 > Could you just call one of the read_* functions?

No, because it depends on PE how much data should I eat up. I can have
another function that would give me the number of bytes occupied by a
given PE, but I don't believe it's a good idea. It's not called that
often, that it could speed things up.

 > But I see that gcc does
 > some similar trick. BTW that function deals with alignment and the gdb
 > version doesn't (yet?).

Hmm I didn't notice. I'll look at it. But it works as it is.

 >  > - Init_loc reading takes care of pcrel addressing.
 >
 > Yes, the bit that does it though, was inside read_encoded_pointer, and
 > now it's not, and you also need to do an additional check, so I  would
 > prefer doing it as it was before.

It wasn't correct, anyway. I have written my reasons against this
approach above.

 > Could you instead add a new parmeter *base* instead of the flag?  In
 > the DW_CFA_set_loc case, base would be NULL while here it would be
 > base_offset, so you could still distinguish the two cases.

How can I know how to compute base_addr when I don't know whether the
encoding is pcrel, textrel, funcrel, ...? I know these aren't used _now_
but at least hypothetically are possible.

 >  > - fde->address_range is always an absolute value, but encoded in a
 >  > pointer format. I don't care if it is in pcrel format or not - 
it's just
 >  > a difference between two pointers.
 >
 > So that would work anyway, right?

I must only know in how many bytes it's encoded so that I advance the
data pointer correctly. The value itself, either if encoded as pcrel, is
always absolute. At least I hope so :-)


 > I don't undersand this change:
 > *** 530,541 ****
 >       }
 >
 >     if (ret != 0)
 > !     switch (encoding & 0xf0)
 >         {
 >         case DW_EH_PE_absptr:
 >         break;
 >         case DW_EH_PE_pcrel:
 > !       ret += (CORE_ADDR) *p;
 >         break;
 >         case DW_EH_PE_textrel:
 >         case DW_EH_PE_datarel:
 > --- 531,543 ----
 >       }
 >
 >     if (ret != 0)
 > !   {
 > !     switch (encoding & 0x70)
 >         {
 >         case DW_EH_PE_absptr:
 >         break;
 >         case DW_EH_PE_pcrel:
 > !       if (flag_pcrel) *flag_pcrel = 1;
 >         break;
 >         case DW_EH_PE_textrel:
 >         case DW_EH_PE_datarel:
 > *************** read_encoded_pointer (bfd *abfd, char **
 > *** 544,549 ****
 > --- 546,555 ----
 >         internal_error (__FILE__, __LINE__,
 >                         "read_encoded_pointer: unknown pointer 
encoding");
 >         }
 > +
 > +     if (encoding & DW_EH_PE_indirect)
 > +       warning ("CFI: Unsupported pointer encoding: DW_EH_PE_indirect");
 > +   }
 >
 >     return ret;
 >   }
 >
 > Why are you changing the mask from 0xf0 to 0x70 in the switch to avoid
 > catching DW_EH_PE_indirect? Could you just add it to the case statement?

I can't because DW_EH_PE_indirect is independent on DW_EH_PE_*rel. Both
PE_pcrel and PE_pcrel_indirect are possible. In fact, encoding variable
contains three flags: size, encoding and (in)direct flag.

All of them must be handled separately.

 >  > > About the eh_frame part of the patch.
 >  > > I don't really like the use of eh_frame as you have in 
dwarf2_build_frame_info.
 >  > > Could you try to figure out a simpler way to deal with the two 
cases?
 >  > > You could use an enumeration for eh_frame.
 >  >
 >  > Well, yes, actually. But then I would have to change:
 >  > if(eh_frame) ...
 >  > to
 >  > if(frame == EH_ONLY || frame == EH_SECOND) ...
 >  >
 >
 > That is the idea, yes. It would be much more understandable to the
 > reader.

I have changed computing of this parameter in dwarf2_build_frame_info so
that it is more readable now, but I'd leave 'eh_frame' as it is, because
in most cases I don't care whether it's 1 or 2. The hack with
eh_frame==2 is there just for speeding up things when looking for
duplicate FDEs. I eh_frame is the only section parsed, then I don't have
to be afraid that we already have a given FDE and don't have to check it
at all.

 >  > What about making local macros IS_DEBUG_FRAME(), IS_EH_FRAME(),
 >  > IS_EH_FRAME_ONLY(), ...?
 >  >
 >
 > How would those behave different from enums? If I am understanding your
 > suggestion correctly, that is (macros are usually not introduced
 > unless absolutely necessary).

IS_EH_FRAME would be true for both possible cases with eh_frame.
As I already said - usually I don't care whether it's parsed first or
after debug_frame.

 > Could you send me a unified diff? (don't need to send it to the list
 > just to me). I'd appreciate that.

Yes. Included. It's against cvs rev 1.9.

Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* +420 2 9654 5373   * http://www.suse.cz

[RFA] dwarf2cfi.c improvements

[Michal Ludvig]

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

Hi all,
this patch adds an error() call when there is an unknown DW_OP_ value in 
a debug_frame section. Next it initialises return value CFA so that it 
isn't used uninitialised when the switch{} finishes on default label.
Easy. Nothing to be broken in here. OK to commit?

Michal Ludvig
-- 
* SuSE CR, s.r.o     * mludvig@suse.cz
* +420 2 9654 5373   * http://www.suse.cz

[-- Attachment #2: cfi-1.diff --]
[-- Type: text/plain, Size: 973 bytes --]

2002-07-17  Michal Ludvig  <michal@suse.cz>

	* dwarf2cfi.c (execute_stack_op): Complain on unknown DW_OP_ value.
	(update_context): Initialise cfa variable.

Index: dwarf2cfi.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2cfi.c,v
retrieving revision 1.15
diff -u -p -r1.15 dwarf2cfi.c
--- dwarf2cfi.c	15 Jul 2002 16:01:31 -0000	1.15
+++ dwarf2cfi.c	18 Jul 2002 12:35:40 -0000
@@ -1227,7 +1227,8 @@ execute_stack_op (struct objfile *objfil
 	      case DW_OP_ne:
 		result = (LONGEST) first != (LONGEST) second;
 		break;
-	      default:		/* This label is here just to avoid warning.  */
+	      default:
+		error ("execute_stack_op: Unknown DW_OP_ value");
 		break;
 	      }
 	  }
@@ -1271,7 +1272,7 @@ static void
 update_context (struct context *context, struct frame_state *fs, int chain)
 {
   struct context *orig_context;
-  CORE_ADDR cfa;
+  CORE_ADDR cfa = 0;
   long i;
 
   unwind_tmp_obstack_init ();

Re: [RFA] dwarf2cfi.c improvements

[Jim Blandy]

Sure --- looks good to me.

Michal Ludvig <mludvig@suse.cz> writes:

> Hi all,
> this patch adds an error() call when there is an unknown DW_OP_ value
> in a debug_frame section. Next it initialises return value CFA so that
> it isn't used uninitialised when the switch{} finishes on default
> label.
> Easy. Nothing to be broken in here. OK to commit?
> 
> Michal Ludvig
> -- 
> * SuSE CR, s.r.o     * mludvig@suse.cz
> * +420 2 9654 5373   * http://www.suse.cz
> 2002-07-17  Michal Ludvig  <michal@suse.cz>
> 
> 	* dwarf2cfi.c (execute_stack_op): Complain on unknown DW_OP_ value.
> 	(update_context): Initialise cfa variable.
> 
> Index: dwarf2cfi.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/dwarf2cfi.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 dwarf2cfi.c
> --- dwarf2cfi.c	15 Jul 2002 16:01:31 -0000	1.15
> +++ dwarf2cfi.c	18 Jul 2002 12:35:40 -0000
> @@ -1227,7 +1227,8 @@ execute_stack_op (struct objfile *objfil
>  	      case DW_OP_ne:
>  		result = (LONGEST) first != (LONGEST) second;
>  		break;
> -	      default:		/* This label is here just to avoid warning.  */
> +	      default:
> +		error ("execute_stack_op: Unknown DW_OP_ value");
>  		break;
>  	      }
>  	  }
> @@ -1271,7 +1272,7 @@ static void
>  update_context (struct context *context, struct frame_state *fs, int chain)
>  {
>    struct context *orig_context;
> -  CORE_ADDR cfa;
> +  CORE_ADDR cfa = 0;
>    long i;
>  
>    unwind_tmp_obstack_init ();

Re: [RFA] dwarf2cfi.c improvements

[Andrew Cagney]

> -	      default:		/* This label is here just to avoid warning.  */
>> +	      default:
>> +		error ("execute_stack_op: Unknown DW_OP_ value");
>>  		break;
>> 

Just FYI, error messages should not be refering to a gdb internal 
functions.  It should probably also be slightly more descriptive 
indicating that the problem is in the dwarf2 information being read in 
and not in GDB.

enjoy,
Andrew

Re: [RFA] dwarf2cfi.c improvements

[Jim Blandy]

Andrew Cagney <ac131313@ges.redhat.com> writes:
> > -	      default:		/* This label is here just to avoid warning.  */
> >> +	      default:
> >> +		error ("execute_stack_op: Unknown DW_OP_ value");
> >>  		break;
> >>
> 
> Just FYI, error messages should not be refering to a gdb internal
> functions.  It should probably also be slightly more descriptive
> indicating that the problem is in the dwarf2 information being read in
> and not in GDB.

Oops.  I noticed that, and then forgot to say anything about it.
Thanks for catching it.