From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7118 invoked by alias); 11 Jun 2002 03:21:59 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 7052 invoked from network); 11 Jun 2002 03:21:58 -0000 Received: from unknown (HELO cygnus.com) (205.180.83.203) by sources.redhat.com with SMTP; 11 Jun 2002 03:21:58 -0000 Received: from localhost.redhat.com (remus.sfbay.redhat.com [172.16.27.252]) by runyon.cygnus.com (8.8.7-cygnus/8.8.7) with ESMTP id UAA25233; Mon, 10 Jun 2002 20:21:41 -0700 (PDT) Received: by localhost.redhat.com (Postfix, from userid 469) id F417910F74; Mon, 10 Jun 2002 23:21:00 -0400 (EDT) From: Elena Zannoni MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <15621.27804.562409.937859@localhost.redhat.com> Date: Mon, 10 Jun 2002 20:21:00 -0000 To: Michal Ludvig Cc: Andrew Cagney , GDB Patches , Elena Zannoni Subject: Re: [RFA] dwarf2cfi.c improvements In-Reply-To: <3CFCA5ED.7010201@suse.cz> References: <3CF79DA0.9040404@suse.cz> <3CFB727E.10005@cygnus.com> <3CFCA5ED.7010201@suse.cz> X-SW-Source: 2002-06/txt/msg00167.txt.bz2 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 > > * 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; > > > 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); > } > } > ! > > /* 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